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.

Sign up for GitHub

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

Jump to bottom

Use modern cmake targets to avoid absolute paths to appear in binary archives #160

Merged
merged 6 commits into from
May 13, 2022
Merged

Use modern cmake targets to avoid absolute paths to appear in binary archives #160

merged 6 commits into from
May 13, 2022

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented May 2, 2022 •
edited
Loading

  • Move logic to find Numpy headers to a module and export a target from there.

  • Use the new NumpyHeaders::NumpyHeaders and PythonExtra::PythonExtra to avoid absolute paths to appear in windows binary archives.

This partially fixes ros2/rclcpp#1688.
Depends on ros2/python_cmake_module#11.

@ivanpauno
* Move logic to find Numpy headers to a module and export a target fr…
1142b0a 
…om there.

* Use the new NumpyHeaders::NumpyHeaders and PythonExtra::PythonExtra to avoid absolute paths to appear in windows binary archives.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the bug Something isn't working label May 2, 2022
@ivanpauno ivanpauno self-assigned this May 2, 2022
@ivanpauno
Copy link
Member Author

Windows packaging CI after this change: Build Status

@ivanpauno
Use FindPython3
a59ab9a 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

ivanpauno commented May 2, 2022 •
edited
Loading

Let's see if I have more luck now: Build Status

@ivanpauno
Export dependency on rosidl_generator_py
3d4a24e 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Make link libraries private when possible to avoid having to export d…
dc9b26b 
…ependencies

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

Third time is the charm (?): Build Status

@ivanpauno
Please linters
2aef321 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

I downloaded the binary archive, and it works!!!
Now there're a lot less absolute paths in the provided binaries.

clalancette
clalancette approved these changes May 3, 2022
View reviewed changes
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, this looks good to me with green CI on all platforms (including RHEL and Windows Debug). I'd like to get a second opinion from @sloretz before we merge, though.

That said, this seems pretty risky to me to put into Humble. This is deep in the heart of the message generation, and we don't know the unintended side effects of this. So my suggestion is that we do indeed put it into Rolling, but we do not backport to Humble. I'd be happy to hear other opinions, though.

rosidl_generator_py/cmake/FindNumpyHeaders.cmake Outdated Show resolved Hide resolved
sloretz
sloretz requested changes May 3, 2022
View reviewed changes
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In CMake 3.14 and above, Python3::NumPy can be used, and looking at webos OSE I think we can use that here ros-infrastructure/rep#351

@ivanpauno
Address peer review comment
50b0a3b 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

In CMake 3.14 and above, Python3::NumPy can be used, and looking at webos OSE I think we can use that here ros-infrastructure/rep#351

Thanks, I'm not sure why I didn't see that before.

@ivanpauno
Copy link
Member Author

That said, this seems pretty risky to me to put into Humble. This is deep in the heart of the message generation, and we don't know the unintended side effects of this.

I think it's safe to backport.
The libraries generated here are only loaded from python to bind with the rosidl_generator_c interface, but nothing depends on them, so we actually don't even need to export the cmake targets.

@ivanpauno
Copy link
Member Author

ivanpauno commented May 12, 2022 •
edited
Loading

Windows packaging job: Build Status

@ivanpauno
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@sloretz
Copy link
Contributor

sloretz commented May 12, 2022

  • Linux re-run Build Status

sloretz
sloretz approved these changes May 12, 2022
View reviewed changes
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for Rolling and Humble backport when CI comes back green.

@ivanpauno ivanpauno merged commit a608556 into master May 13, 2022
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/fix-python-and-numpy-absolute-path-win-bin-archive branch May 13, 2022 17:41
@ivanpauno
Copy link
Member Author

@Mergifyio backport humble

mergify bot pushed a commit that referenced this pull request May 13, 2022
@ivanpauno @mergify
Use modern cmake targets to avoid absolute paths to appear in binary …
2b78134 
…archives (#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>
(cherry picked from commit a608556)
@mergify mergify bot mentioned this pull request May 13, 2022
Closed
@mergify
Copy link

mergify bot commented May 13, 2022

backport humble

✅ Backports have been created

This was referenced May 16, 2022
Open
Closed
ivanpauno added a commit that referenced this pull request May 16, 2022
@ivanpauno
Revert "Use modern cmake targets to avoid absolute paths to appear in…
6ad9454 
… binary archives (#160)"

This reverts commit a608556.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno mentioned this pull request May 16, 2022
Merged
ivanpauno added a commit that referenced this pull request May 17, 2022
@ivanpauno
Revert "Use modern cmake targets to avoid absolute paths to appear in…
6675823 
… binary archives (#160)" (#166)

This reverts commit a608556.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@wolfv wolfv mentioned this pull request Jun 26, 2022
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz approved these changes

@clalancette clalancette Awaiting requested review from clalancette

Assignees

@ivanpauno ivanpauno

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

find_package rclcpp adds non-existent include dir
3 participants
@ivanpauno @sloretz @clalancette