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

[backport] Use modern cmake targets to avoid absolute paths to appear in binary … #214

Closed
wants to merge 1 commit into from

Conversation

xueying4402
Copy link

…archives (#160)

  • Use FindPython3

  • Make link libraries private when possible to avoid having to export dependencies

  • Use Python3::Numpy

…archives (ros2#160)

* Use FindPython3

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

* Make link libraries private when possible to avoid having to export dependencies

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

* Use Python3::Numpy

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
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 have to say that I'm very worried about doing this. I've been poking at this for months in #204 (though that has a different goal), and the whole system here is tricky. For a stable release, I think this is going to be dangerous and may have unwanted side-effects. For those reasons, my opinion is that we should not take this into humble at this time.

@sloretz What's your take on this one?

@sloretz
Copy link
Contributor

sloretz commented Nov 1, 2024

Thank you for the PR, and for your patience! I'm going to close this backport for now because I don't feel confident that it won't break someone's workflow.

@sloretz What's your take on this one?

I think my main concern is that Python3 CMake module could find a different version of NumPy than the current custom logic in rosidl_generator_py. I think that could break a non-standard Python setup like a venv. While ROS 2 doesn't directly support those, I've seen bugs opened by people using different non-standard Python setups for other platforms.

@sloretz sloretz closed this Nov 1, 2024
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.

4 participants