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

Make ament_cmake_python_install() handle symlink installs. #325

Closed

Conversation

hidmic
Copy link
Contributor

@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

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic added the bug Something isn't working label Mar 4, 2021
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Mar 5, 2021

Running CI for Windows:

  • Windows Build Status

Copy link
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 ✔️

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@ivanpauno
Copy link
Contributor

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

@hidmic
Copy link
Contributor 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
Contributor

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
Contributor 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
Contributor 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.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Copy link
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

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor 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
Contributor Author

hidmic commented Mar 8, 2021

Argh... this is so fragile.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Mar 8, 2021

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
Contributor 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
Copy link
Contributor 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
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants