-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add rosidl_find_package_idl helper function #754
Add rosidl_find_package_idl helper function #754
Conversation
Signed-off-by: Mike Purvis <mpurvis@clearpathrobotics.com>
9659fb4
to
cbeaba1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think this is an OK thing to do. I've left some feedback inline on things to fix.
I do want to make one comment about the overall goal of switching to GNUInstallDirs
. After discussing with some other people, one thing that came up was that the switch to GNUInstallDirs
will probably have knock-on effects, particularly on RHEL/Fedora. We think that our infrastructure handles this, but we aren't sure about this aspect. So there will need to be careful testing on both our default platforms (Ubuntu and Windows), as well as on RHEL.
rosidl_generator_cpp/cmake/rosidl_generator_cpp_generate_interfaces.cmake
Show resolved
Hide resolved
...pesupport_introspection_c/cmake/rosidl_typesupport_introspection_c_generate_interfaces.cmake
Show resolved
Hide resolved
...pport_introspection_cpp/cmake/rosidl_typesupport_introspection_cpp_generate_interfaces.cmake
Show resolved
Hide resolved
Signed-off-by: Mike Purvis <mpurvis@clearpathrobotics.com>
a0dca0c
to
83de716
Compare
@clalancette Thanks for looking at this. I've added the missing dependencies; let me know if there's anything else I can do. Regarding GNUInstallDirs, my impression on an initial survey is that it would largely be a no-op for current use cases. Basically we'd be replacing what is currently hard-coded ceres-solver/ceres-solver@4998c54 But I'd be glad to participate in a discussion about what are some of the other barriers people are foreseeing. |
@clalancette I see that I made the changes but forgot to hit the button to re-request review, sorry about that. Would love to get this in when you are able. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems OK to me. Let's see how it does in CI.
Fantastic! 🎉 FYI @guillaumeautran |
This is an initial effort meant to spur discussion— it's a first step in making ament and ROS 2 friendlier to split-output building, which is ultimately accomplished by explicitly passing the values of the GNUInstallDirs variables into the configure step (there isn't really an equivalent for Python, since all of a Python package is required at runtime):
Part of the follow-on to that will be having ament (and catkin) generate variables like
fancy_package_DATADIR
andfancy_package_LIBDIR
into their package find modules, and then working through the package tree to break the assumption that runtime files will always be located relative to thefind_package
-suppliedfancy_package_DIR
variable (which points to the location of the CMake module, in the dev tree— which in the Nix case can be disjoint from the runtime/out tree).One of our biggest pain points around this is the finding of IDL files at configure time, since this is done by name, relative to the CMake module location, and occurs in many packages in a cut-and-pasted snippet that looks always like this:
This change seeks to centralize a small part of this blob into a
rosidl_cmake
helper function where it will be more convenient to iterate on the logic used in locating these things, ahead of some more ambitious changes to other parts of ament. In the immediate-term, if you don't want to include the${${pkg_name}_DATADIR}/${idl_file}
part of this, at least adopting the framework makes it possible for us to patch this in in a single location rather than 7+ of them.My initial questions are:
rosidl_cmake
the proper place for the suggested helper function?_dependencies
and_dependency_files
?rosidl_typesupport
,rosidl_typesupport_fastrtps
,rosidl_dds
,rosidl_python
, etc) which adopt the new helper function?FYI @iwanders @guillaumeautran @lopsided98 @gbiggs