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

Revert install of .so files into python path #211

Merged
merged 1 commit into from Apr 12, 2024
Merged

Revert install of .so files into python path #211

merged 1 commit into from Apr 12, 2024

Conversation

ghost
Copy link

@ghost ghost commented Apr 9, 2024

There seems that some regression might have happened after #195. When removing those 2 lines, we avoid to install the .so files in lib and python path.

Revert install of .so files into python path
2cddd1c 
There seems that some regression might have happened after #195.
When removing those 2 lines, we avoid to install the .so files
in lib *and* python path.

Signed-off-by: Matthias Schoepfer <m.schoepfer@rethinkrobotics.com>
@ghost ghost requested review from quarkytale and sloretz as code owners April 9, 2024 13:08
@clalancette
Copy link
Contributor

There seems that some regression might have happened after #195. When removing those 2 lines, we avoid to install the .so files in lib and python path.

Can you explain the regression? We haven't been seeing that in our testing, so I'm curious what is happening.

@ghost
Copy link
Author

ghost commented Apr 9, 2024 •
edited by ghost
Loading

Hi @clalancette ,

sure. When you build for example builtin-interfaces, you will get
/usr/lib/libbuiltin_interfaces__rosidl_generator_py.so
and
/usr/lib/python3.12/site-packages/builtin_interfaces/libbuiltin_interfaces__rosidl_generator_py.so
which are essentially providing the same stuff. The later one is not needed and just bloats the packages.

@ghost
Copy link
Author

ghost commented Apr 9, 2024

Note that it was removed in #195 but sneaked in again later with 1505ac0

@robwoolley robwoolley mentioned this pull request Apr 10, 2024
Closed
@clalancette
Copy link
Contributor

Note that it was removed in #195 but sneaked in again later with 1505ac0

Ah, thanks for the explanation. You are right, it did sneak back in. Here is CI to see if this works everywhere:

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

@clalancette
Copy link
Contributor

Both of the failing tests here are known issues, so I'm going to go ahead and merge this one. Thanks for fixing this again.

@clalancette clalancette merged commit 75c10df into ros2:rolling Apr 12, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quarkytale quarkytale Awaiting requested review from quarkytale quarkytale is a code owner

@sloretz sloretz Awaiting requested review from sloretz sloretz is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@clalancette