From b4ddebf011ecdc96d038c10c7a90f6f80274c218 Mon Sep 17 00:00:00 2001 From: Carter Date: Thu, 5 Oct 2023 12:22:29 -0600 Subject: [PATCH] Abilty to correctly inform cargo of file dependnencies in build.rs --- CHANGELOG.md | 5 +++ example_package/build.rs | 14 ++++-- roslibrust_codegen/src/lib.rs | 67 ++++++++++++++++++++++------- roslibrust_codegen_macro/src/lib.rs | 12 ++++-- roslibrust_genmsg/src/lib.rs | 2 +- roslibrust_test/src/main.rs | 33 ++++++++------ 6 files changed, 95 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 401351b..8653e2e 100644 --- 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 index 9678a37..92939b0 100644 --- a/example_package/build.rs +++ b/example_package/build.rs @@ -17,7 +17,8 @@ fn main() -> Result<(), Box> { // 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> { 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 index 9439190..c059e19 100644 --- a/roslibrust_codegen/src/lib.rs +++ b/roslibrust_codegen/src/lib.rs @@ -338,12 +338,14 @@ 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, -) -> Result { +) -> Result<(TokenStream, Vec), 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) @@ -351,12 +353,15 @@ pub fn find_and_generate_ros_messages( /// 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, -) -> Result { - let (messages, services) = find_and_parse_ros_messages(&search_paths)?; +) -> Result<(TokenStream, Vec), 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 @@ -364,7 +369,12 @@ pub fn find_and_generate_ros_messages_without_ros_package_path( 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, -) -> Result<(Vec, Vec), Error> { +) -> Result< + ( + Vec, + Vec, + Vec, + ), + 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, Vec), Error> { +) -> Result< + ( + Vec, + Vec, + Vec, + ), + 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,6 +604,7 @@ 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)?; @@ -586,6 +612,7 @@ fn parse_ros_files( } "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 index 63134f0..d227f3d 100644 --- 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 index 0ab22d7..6d8efd7 100644 --- 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> { // 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 index d92fbba..b4c5938 100644 --- 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> { 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");