Skip to content
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

Merged
merged 2 commits into from Aug 21, 2023

Conversation

mikepurvis
Copy link
Contributor

@mikepurvis mikepurvis commented Jun 15, 2023

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):

cmake .. -DCMAKE_INSTALL_LIBDIR=$out -DCMAKE_INSTALL_DATADIR=$out -DCMAKE_INSTALL_INCLUDEDIR=$dev [....]

Part of the follow-on to that will be having ament (and catkin) generate variables like fancy_package_DATADIR and fancy_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 the find_package-supplied fancy_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:

foreach(_pkg_name ${rosidl_generate_interfaces_DEPENDENCY_PACKAGE_NAMES})
  foreach(_idl_file ${${_pkg_name}_IDL_FILES})
    set(_abs_idl_file "${${_pkg_name}_DIR}/../${_idl_file}")
    normalize_path(_abs_idl_file "${_abs_idl_file}")
    rosidl_find_package_idl(_abs_idl_file "${_pkg_name}" "${_idl_file}")
    list(APPEND _dependency_files "${_abs_idl_file}")
    list(APPEND _dependencies "${_pkg_name}:${_abs_idl_file}")
  endforeach()
endforeach()

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:

  • Does the problem statement and overall approach seem to make sense here?
  • Is rosidl_cmake the proper place for the suggested helper function?
  • Should the helper be expanded in scope to include either or both of the surrounding loops, so basically just taking in the list of package names and spitting back _dependencies and _dependency_files?
  • Is it likely we can merge something like this in the relatively near term, and then make companion changes to other repos (rosidl_typesupport, rosidl_typesupport_fastrtps, rosidl_dds, rosidl_python, etc) which adopt the new helper function?

FYI @iwanders @guillaumeautran @lopsided98 @gbiggs

Signed-off-by: Mike Purvis <mpurvis@clearpathrobotics.com>
@clalancette clalancette self-assigned this Jun 22, 2023
Copy link
Contributor

@clalancette clalancette left a 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.

Signed-off-by: Mike Purvis <mpurvis@clearpathrobotics.com>
@mikepurvis
Copy link
Contributor Author

mikepurvis commented Jul 4, 2023

@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 lib, include, etc with the CMAKE_INSTALL_<thing> variables, for example stuff like this:

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.

@mikepurvis
Copy link
Contributor Author

@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.

Copy link
Contributor

@clalancette clalancette left a 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.

@clalancette
Copy link
Contributor

clalancette commented Aug 10, 2023

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Windows Debug Build Status
  • RHEL Build Status

@clalancette clalancette merged commit f47565c into ros2:rolling Aug 21, 2023
3 checks passed
@mikepurvis
Copy link
Contributor Author

Fantastic! 🎉

FYI @guillaumeautran

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants