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 include directories consistently PRIVATE instead of PUBLIC #201

Closed
wants to merge 1 commit into from

Conversation

cwecht
Copy link

@cwecht cwecht commented Jan 17, 2024

This helps in situations in which:

  1. If the numpy include directory is determined by executing python: and
  2. If the path to your numpy installation may change.

This can happen e.g. if you want to cross-compile ROS2 and want to do it on some CICD setups.

The issue is that right now, e.g. for std_msgs export_std_msgs__rosidl_generator_pyExport.cmake sets the INTERFACE_INCLUDE_DIRECTORIES property of std_msgs::std_msgs__rosidl_generator_py to an absolute path (which causes issues if the folder of your numpy installation changes.

With this patch all include directories are consistently added PRIVATE instead of PUBLIC. There shouldn't be a need to expose them anyways.

Signed-off-by: Christopher Wecht <cwecht@mailbox.org>
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.

I'm going to ask to hold off on this one. This change is also done by the much larger #140, but that one has additional benefits. I'm just waiting for @sloretz to finish reviewing that one.

@clalancette
Copy link
Contributor

I've now merged in #140, which also contains this fix. So I'll close this one out.

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.

2 participants