Skip to content

Make ament_cmake_python_install() handle symlink installs.#325

Closed
hidmic wants to merge 6 commits intomasterfrom
hidmic/fix-ament-cmake-python-for-symlink-installs
Closed

Make ament_cmake_python_install() handle symlink installs.#325
hidmic wants to merge 6 commits intomasterfrom
hidmic/fix-ament-cmake-python-for-symlink-installs

Conversation

@hidmic
Copy link
Copy Markdown

@hidmic hidmic commented Mar 4, 2021

Precisely what the title says. Yet another regression after #316, though this one turned into a rabbit hole. colcon handles Python packages in a very peculiar way, so this patch replicates as much as necessary into ament_cmake_python.

Full CI, just to be sure:

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

@hidmic
Make ament_cmake_python_install() handle symlink installs.
988100d 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic added the bug Something isn't working label Mar 4, 2021
ivanpauno
ivanpauno reviewed Mar 5, 2021
View reviewed changes
@hidmic
Prevent Windows native path escaping
1e0f464 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Copy Markdown
Author

hidmic commented Mar 5, 2021 •
edited
Loading

Running CI for Windows:

  • Windows Build Status

ivanpauno
ivanpauno reviewed Mar 5, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!
It would be great if someone else can also take a look too.

I will also test locally before ✔️

@hidmic
Address peer review comments
f14c59b 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@ivanpauno
Copy link
Copy Markdown
Contributor

ivanpauno commented Mar 5, 2021

I would recommend running CI again with --symlink-install, just to double check everything is fine

@hidmic
Copy link
Copy Markdown
Author

hidmic commented Mar 5, 2021

Test failures in full CI are known and can be found in nightlies as well.

CI using --symlink-install as per request, but up to rclpy (should be enough to reveal any bugs):

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

@ivanpauno
Copy link
Copy Markdown
Contributor

ivanpauno commented Mar 8, 2021

CI using --symlink-install as per request, but up to rclpy (should be enough to reveal any bugs):

Just in case, it would be good to also test something that depends on rclpy.
While using this branch, I had problems with rclpy dependencies not being able to find the _rclpy shared library (I'm not sure way, maybe PYTHONPATH was wrong (?)).

@hidmic
Copy link
Copy Markdown
Author

hidmic commented Mar 8, 2021

Ok, Windows CI is failing because some hard-coded relative path in rosidl_typesupport_connext_c is no longer valid for symlink installs. Unix CIs are trickier. I cannot reproduce locally, which makes me think something's special about their filesystem layout.

@hidmic
Copy link
Copy Markdown
Author

hidmic commented Mar 8, 2021

Alright, it's worst than I thought. There's a lot of packages that rely on Python sources (or a symlink to them) being present in the install space... I'll drop the environment hooks and do plain symlinks. It's not more of a hack than using these ad-hoc hooks.

@hidmic
Add symlink to sources at install dir
d173dab 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
ivanpauno
ivanpauno approved these changes Mar 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM if CI is happy

@hidmic
Fix (but detect) failing setup.py commands
d10a85b 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Copy Markdown
Author

hidmic commented Mar 8, 2021

CI using --symlink-install up to rclpy and test_communication:

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

@hidmic
Copy link
Copy Markdown
Author

hidmic commented Mar 8, 2021 •
edited
Loading

Argh... this is so fragile.

@hidmic
Symlink source files one by one
f5f9768 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Copy Markdown
Author

hidmic commented Mar 8, 2021 •
edited
Loading

This patch is an abomination, but I don't know how to make things better.

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

BTW there's also an uninstall target that this macro does not implement, but it doesn't seem to be working well either way. I have some prototype code implementing it, but it only half-works (I came across pypa/setuptools#175 and other niceties).

@hidmic
Copy link
Copy Markdown
Author

hidmic commented Mar 8, 2021

This doesn't work either... pkg_resources picks up egg links, but importlib.metadata doesn't 🤯

I'm starting to consider rolling back and simply dropping a flat egg in the install space. It's a hack, but Python's packaging ecosystem is not cooperating.

@hidmic hidmic mentioned this pull request Mar 9, 2021
Merged
@hidmic
Copy link
Copy Markdown
Author

hidmic commented Mar 9, 2021

Superseded by #326.

@hidmic hidmic closed this Mar 9, 2021
@hidmic hidmic deleted the hidmic/fix-ament-cmake-python-for-symlink-installs branch March 9, 2021 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ivanpauno ivanpauno ivanpauno approved these changes

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

@sloretz sloretz Awaiting requested review from sloretz

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hidmic @ivanpauno