Merge pull request #139 from Carter12s/codegen-dependent-files

Abilty to correctly inform cargo of file dependnencies in build.rs
RosLibRust · Oct 8, 2023 · 7c248ea · 7c248ea
2 parents dffd3a0 + b4ddebf
commit 7c248ea
Show file tree

Hide file tree

Showing 6 changed files with 95 additions and 38 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -24,10 +24,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- The build.rs example in example_package now correctly informs cargo of filesystem dependencies
+
 ### Fixed
 
 ### Changed
 
+ - The function interface for top level generation functions in `roslibrust_codegen` have been changed to include the list of dependent
+filesystem paths that should trigger re-running code generation. Note: new files added to the search paths will not be automatically detected.
+
 ## 0.8.0 - October 4th, 2023
 
 ### Added

diff --git a/example_package/build.rs b/example_package/build.rs
@@ -17,7 +17,8 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
     // Actually invoke code generation on our search paths.
     // What we get back is a TokenStream the type normally returned by a proc_macro in rust.
     // For a build.rs we typically want to serialize this data to a file for later import
-    let tokens = roslibrust_codegen::find_and_generate_ros_messages_without_ros_package_path(p)?;
+    let (source, dependent_paths) =
+        roslibrust_codegen::find_and_generate_ros_messages_without_ros_package_path(p)?;
 
     // It is important for build scripts to only output files to OUT_DIR.
     // This guidance can be ignored for end applications. However, crates published and downloaded with cargo
@@ -26,14 +27,19 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
     let dest_path = std::path::Path::new(&out_dir).join("messages.rs");
     // Write the generate code to disk
     // Note: it will not be nicely formatted at this point which can affect readability when debugging
-    std::fs::write(dest_path, tokens.to_string())?;
+    std::fs::write(dest_path, source.to_string())?;
     // Optionally rustfmt could be invoked to format the file at this point
     // Or https://github.com/dtolnay/prettyplease used on the TokenStream ahead of writing to disk
 
     // If we stopped at this point, our code would still work, but Cargo would not know to rebuild
     // our package when a message file changed.
-    // MAJOR TODO need to get codegen methods to return list of dependent files
-    // Also probably want to merge down function names to single function with more args / builder
+    for path in &dependent_paths {
+        let path_str = path
+            .as_os_str()
+            .to_str()
+            .expect(&format!("Failed to convert path into utf8: {path:?}"));
+        cargo_emit::rerun_if_changed!(path_str);
+    }
 
     Ok(())
 }
diff --git a/roslibrust_codegen/src/lib.rs b/roslibrust_codegen/src/lib.rs
@@ -338,33 +338,43 @@ impl PartialEq for ConstantInfo {
 
 /// Searches a list of paths for ROS packages and generates struct definitions
 /// and implementations for message files and service files in packages it finds.
-///
+/// Returns a tuple of the generated source code and list of file system paths that if
+/// modified would trigger re-generation of the source. This function is designed to
+/// be used either in a build.rs file or via the roslibrust_codegen_macro crate.
 /// * `additional_search_paths` - A list of additional paths to search beyond those
 /// found in ROS_PACKAGE_PATH environment variable.
 pub fn find_and_generate_ros_messages(
     additional_search_paths: Vec<PathBuf>,
-) -> Result<TokenStream, Error> {
+) -> Result<(TokenStream, Vec<PathBuf>), Error> {
     let mut ros_package_paths = utils::get_search_paths();
     ros_package_paths.extend(additional_search_paths);
     find_and_generate_ros_messages_without_ros_package_path(ros_package_paths)
 }
 
 /// Searches a list of paths for ROS packages and generates struct definitions
 /// and implementations for message files and service files in packages it finds.
+/// Returns a tuple of the generated source code and list of file system paths that if
+/// modified would trigger re-generation of the source. This function is designed to
+/// be used either in a build.rs file or via the roslibrust_codegen_macro crate.
 ///
 /// * `search_paths` - A list of paths to search for ROS packages.
 pub fn find_and_generate_ros_messages_without_ros_package_path(
     search_paths: Vec<PathBuf>,
-) -> Result<TokenStream, Error> {
-    let (messages, services) = find_and_parse_ros_messages(&search_paths)?;
+) -> Result<(TokenStream, Vec<PathBuf>), Error> {
+    let (messages, services, actions) = find_and_parse_ros_messages(&search_paths)?;
 
     if messages.is_empty() && services.is_empty() {
         // I'm considering this an error for now, but I could see this one being debateable
         // As it stands there is not good way for us to manually produce a warning, so I'd rather fail loud
         bail!("Failed to find any services or messages while generating ROS message definitions, paths searched: {search_paths:?}");
     }
     let (messages, services) = resolve_dependency_graph(messages, services)?;
-    generate_rust_ros_message_definitions(messages, services)
+    let msg_iter = messages.iter().map(|m| m.parsed.path.clone());
+    let srv_iter = services.iter().map(|s| s.parsed.path.clone());
+    let action_iter = actions.iter().map(|a| a.path.clone());
+    let dependent_paths = msg_iter.chain(srv_iter).chain(action_iter).collect();
+    let source = generate_rust_ros_message_definitions(messages, services)?;
+    Ok((source, dependent_paths))
 }
 
 /// Searches a list of paths for ROS packages to find their associated message
@@ -376,7 +386,14 @@ pub fn find_and_generate_ros_messages_without_ros_package_path(
 ///
 pub fn find_and_parse_ros_messages(
     search_paths: &Vec<PathBuf>,
-) -> Result<(Vec<ParsedMessageFile>, Vec<ParsedServiceFile>), Error> {
+) -> Result<
+    (
+        Vec<ParsedMessageFile>,
+        Vec<ParsedServiceFile>,
+        Vec<ParsedActionFile>,
+    ),
+    Error,
+> {
     let search_paths  = search_paths
         .into_iter()
         .map(|path| {
@@ -555,9 +572,17 @@ pub fn resolve_dependency_graph(
 /// * `msg_paths` -- List of tuple (Package, Path to File) for each file to parse
 fn parse_ros_files(
     msg_paths: Vec<(Package, PathBuf)>,
-) -> Result<(Vec<ParsedMessageFile>, Vec<ParsedServiceFile>), Error> {
+) -> Result<
+    (
+        Vec<ParsedMessageFile>,
+        Vec<ParsedServiceFile>,
+        Vec<ParsedActionFile>,
+    ),
+    Error,
+> {
     let mut parsed_messages = Vec::new();
     let mut parsed_services = Vec::new();
+    let mut parsed_actions = Vec::new();
     for (pkg, path) in msg_paths {
         let contents = std::fs::read_to_string(&path).map_err(|e| {
             Error::with(
@@ -579,13 +604,15 @@ fn parse_ros_files(
             "srv" => {
                 let srv_file = parse_ros_service_file(&contents, name, &pkg, &path)?;
                 parsed_services.push(srv_file);
+                // TODO ask shane, shouldn't we be pushing request and response to messages here?
             }
             "msg" => {
                 let msg = parse_ros_message_file(&contents, name, &pkg, &path)?;
                 parsed_messages.push(msg);
             }
             "action" => {
                 let action = parse_ros_action_file(&contents, name, &pkg, &path)?;
+                parsed_actions.push(action.clone());
                 parsed_messages.push(action.action_type);
                 parsed_messages.push(action.action_goal_type);
                 parsed_messages.push(action.goal_type);
@@ -599,7 +626,7 @@ fn parse_ros_files(
             }
         }
     }
-    Ok((parsed_messages, parsed_services))
+    Ok((parsed_messages, parsed_services, parsed_actions))
 }
 
 #[cfg(test)]
@@ -614,9 +641,11 @@ mod test {
             "/../assets/ros1_common_interfaces"
         );
 
-        let gen = find_and_generate_ros_messages(vec![assets_path.into()]);
+        let (source, paths) = find_and_generate_ros_messages(vec![assets_path.into()]).unwrap();
         // Make sure something actually got generated
-        assert!(!gen.unwrap().is_empty())
+        assert!(!source.is_empty());
+        // Make sure we have some paths
+        assert!(!paths.is_empty());
     }
 
     /// Confirms we don't panic on ros2 parsing
@@ -627,9 +656,11 @@ mod test {
             "/../assets/ros2_common_interfaces"
         );
 
-        let gen = find_and_generate_ros_messages(vec![assets_path.into()]);
+        let (source, paths) = find_and_generate_ros_messages(vec![assets_path.into()]).unwrap();
         // Make sure something actually got generated
-        assert!(!gen.unwrap().is_empty())
+        assert!(!source.is_empty());
+        // Make sure we have some paths
+        assert!(!paths.is_empty());
     }
 
     /// Confirms we don't panic on ros1_test_msgs parsing
@@ -643,8 +674,11 @@ mod test {
             env!("CARGO_MANIFEST_DIR"),
             "/../assets/ros1_common_interfaces/std_msgs"
         );
-        let gen = find_and_generate_ros_messages(vec![assets_path.into(), std_msgs.into()]);
-        assert!(!gen.unwrap().is_empty());
+        let (source, paths) =
+            find_and_generate_ros_messages(vec![assets_path.into(), std_msgs.into()]).unwrap();
+        assert!(!source.is_empty());
+        // Make sure we have some paths
+        assert!(!paths.is_empty());
     }
 
     /// Confirms we don't panic on ros2_test_msgs parsing
@@ -653,7 +687,8 @@ mod test {
     fn generate_ok_on_ros2_test_msgs() {
         let assets_path = concat!(env!("CARGO_MANIFEST_DIR"), "/../assets/ros2_test_msgs");
 
-        let gen = find_and_generate_ros_messages(vec![assets_path.into()]);
-        assert!(!gen.unwrap().is_empty());
+        let (source, paths) = find_and_generate_ros_messages(vec![assets_path.into()]).unwrap();
+        assert!(!source.is_empty());
+        assert!(!paths.is_empty());
     }
 }
diff --git a/roslibrust_codegen_macro/src/lib.rs b/roslibrust_codegen_macro/src/lib.rs
@@ -30,10 +30,12 @@ impl Parse for RosLibRustMessagePaths {
 /// variable ROS_PACKAGE_PATH.
 #[proc_macro]
 pub fn find_and_generate_ros_messages(input_stream: TokenStream) -> TokenStream {
+    // Note: there is not currently a way for proc_macros to indicate that they need to be re-generated
+    // We discard the "dependent_paths" part of the response here...
     let RosLibRustMessagePaths { paths } =
         parse_macro_input!(input_stream as RosLibRustMessagePaths);
     match roslibrust_codegen::find_and_generate_ros_messages(paths) {
-        Ok(t) => t.into(),
+        Ok((source, _dependent_paths)) => source.into(),
         Err(e) => {
             let error_msg = e.to_string();
             quote::quote!(compile_error!(#error_msg);).into()
@@ -58,7 +60,9 @@ pub fn find_and_generate_ros_messages_relative_to_manifest_dir(
     }
 
     match roslibrust_codegen::find_and_generate_ros_messages(paths) {
-        Ok(t) => t.into(),
+        // Note: there is not currently a way for proc_macros to indicate that they need to be re-generated
+        // We discard the "dependent_paths" part of the response here...
+        Ok((source, _dependent_paths)) => source.into(),
         Err(e) => {
             let error_msg = e.to_string();
             quote::quote!(compile_error!(#error_msg);).into()
@@ -75,7 +79,9 @@ pub fn find_and_generate_ros_messages_without_ros_package_path(
     let RosLibRustMessagePaths { paths } =
         parse_macro_input!(input_stream as RosLibRustMessagePaths);
     match roslibrust_codegen::find_and_generate_ros_messages_without_ros_package_path(paths) {
-        Ok(t) => t.into(),
+        // Note: there is not currently a way for proc_macros to indicate that they need to be re-generated
+        // We discard the "dependent_paths" part of the response here...
+        Ok((source, _dependent_paths)) => source.into(),
         Err(e) => {
             let error_msg = e.to_string();
             quote::quote!( compile_error!(#error_msg); ).into()

diff --git a/roslibrust_genmsg/src/lib.rs b/roslibrust_genmsg/src/lib.rs
@@ -89,7 +89,7 @@ impl<'a> CodeGeneratorBuilder<'a> {
     pub fn build(self) -> std::io::Result<CodeGenerator<'a>> {
         // Being lazy here and not infecting other error types to far
         // Eventually I think we should move away from io::Result here and remove both of these unwraps
-        let (messages, services) =
+        let (messages, services, _actions) =
             roslibrust_codegen::find_and_parse_ros_messages(&self.msg_paths).unwrap();
         let (messages, services) =
             roslibrust_codegen::resolve_dependency_graph(messages, services).unwrap();

diff --git a/roslibrust_test/src/main.rs b/roslibrust_test/src/main.rs
@@ -25,18 +25,19 @@ lazy_static! {
 /// This main function is used to generate the contents of ros1.rs, ros2.rs
 fn main() -> Result<(), Box<dyn std::error::Error>> {
     env_logger::init();
-    let tokens = roslibrust_codegen::find_and_generate_ros_messages_without_ros_package_path(
-        (*ROS_1_PATHS).clone(),
-    )?;
-    let source = format_rust_source(tokens.to_string().as_str()).to_string();
+    let (source, _paths) =
+        roslibrust_codegen::find_and_generate_ros_messages_without_ros_package_path(
+            (*ROS_1_PATHS).clone(),
+        )?;
+    let source = format_rust_source(source.to_string().as_str()).to_string();
     std::fs::write(concat!(env!("CARGO_MANIFEST_DIR"), "/src/ros1.rs"), source)?;
 
-    let tokens =
+    let (source, _paths) =
         roslibrust_codegen::find_and_generate_ros_messages_without_ros_package_path(vec![
             ROS_2_PATH.into(),
             ROS_2_TEST_PATH.into(),
         ])?;
-    let source = format_rust_source(tokens.to_string().as_str()).to_string();
+    let source = format_rust_source(source.to_string().as_str()).to_string();
     std::fs::write(concat!(env!("CARGO_MANIFEST_DIR"), "/src/ros2.rs"), source)?;
     Ok(())
 }
@@ -73,10 +74,12 @@ mod test {
     /// Confirms that codegen has been run and changes committed
     #[test]
     fn ros1_lib_is_up_to_date() {
-        let tokens = roslibrust_codegen::find_and_generate_ros_messages_without_ros_package_path(
-            (*ROS_1_PATHS).clone(),
-        );
-        let source = format_rust_source(tokens.unwrap().to_string().as_str()).to_string();
+        let (source, _paths) =
+            roslibrust_codegen::find_and_generate_ros_messages_without_ros_package_path(
+                (*ROS_1_PATHS).clone(),
+            )
+            .unwrap();
+        let source = format_rust_source(source.to_string().as_str()).to_string();
         let lib_path = env!("CARGO_MANIFEST_DIR").to_string() + "/src/ros1.rs";
         let lib_contents =
             std::fs::read_to_string(lib_path).expect("Failed to load current ros1.rs contents");
@@ -93,10 +96,12 @@ mod test {
     /// Confirms that codegen has been run and changes committed
     #[test]
     fn ros2_lib_is_up_to_date() {
-        let tokens = roslibrust_codegen::find_and_generate_ros_messages_without_ros_package_path(
-            (*ROS_2_PATHS).clone(),
-        );
-        let source = format_rust_source(tokens.unwrap().to_string().as_str()).to_string();
+        let (source, _paths) =
+            roslibrust_codegen::find_and_generate_ros_messages_without_ros_package_path(
+                (*ROS_2_PATHS).clone(),
+            )
+            .unwrap();
+        let source = format_rust_source(source.to_string().as_str()).to_string();
         let lib_path = env!("CARGO_MANIFEST_DIR").to_string() + "/src/ros2.rs";
         let lib_contents =
             std::fs::read_to_string(lib_path).expect("Failed to load current ros2.rs contents");