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_python_install_package() install console_scripts #328

Merged
merged 3 commits into from May 10, 2021

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Mar 11, 2021

Precisely what the title says. A nice to have, addressing #213.

Preliminary CI up to ament_cmake_python (no core packages exercises the feature):

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

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

what is the result of this if there is no script to install?
do you get an empty <build_dir>/scripts and nothing happens in the install folder?

If that's the case (which seems like it), lgtm!
if something get's installed in the install folder even if no script exist, doesn't lgtm 😂

@ivanpauno
Copy link
Contributor

A nice to have, addressing #213.

Nice!

@hidmic
Copy link
Contributor Author

hidmic commented Mar 11, 2021

if something get's installed in the install folder even if no script exist, doesn't lgtm

Good point. Yeah, empty directory. Ugh. Will fix.

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

hidmic commented Mar 11, 2021

b6cb63b does the trick, but it requires explicit request. I had to add DESTINATION, I hated the asymmetry.

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 this would be used somewhere so we can check it actually works, but that's something that it can be done later.

@hidmic
Copy link
Contributor Author

hidmic commented Mar 12, 2021

ros-simulation/gazebo_ros_pkgs#1252 does, but I don't think we can run a joint CI. After a Rolling release, perhaps.

@hidmic
Copy link
Contributor Author

hidmic commented Apr 5, 2021

I'll get this one merged after the freeze. It's a nice to have and I don't want to break anything 😅

@hidmic
Copy link
Contributor Author

hidmic commented May 5, 2021

Full CI:

  • Linux Build Status (unrelated warning)
  • Linux-aarch64 Build Status (unrelated warning)
  • macOS Build Status
  • Windows Build Status

@hidmic hidmic requested a review from clalancette May 6, 2021 15:27
@hidmic
Copy link
Contributor Author

hidmic commented May 10, 2021

Alright, this is ready to go in. I do not expect any fallout, but I will keep an eye on it for the next few days. If it gives us trouble, I'll revert it right away.

@hidmic hidmic merged commit 8551eec into master May 10, 2021
@delete-merged-branch delete-merged-branch bot deleted the hidmic/ament-python-install-package-scripts branch May 10, 2021 16:10
wep21 pushed a commit to wep21/ament_cmake that referenced this pull request Nov 26, 2021
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
hidmic added a commit that referenced this pull request Dec 7, 2021
)

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>
@rhaschke
Copy link
Contributor

It feels somewhat awkward that one needs to specify SCRIPTS_DESTINATION lib/${PROJECT_NAME} in order to generate and install console scripts. Obviously, this argument is used for two independent purposes:

  1. specifying the destination for those scripts (which is always lib/${PROJECT_NAME} by default)
  2. enabling the script generation at all

IMHO, only the first point is justified. The generation should be triggered in any case. To ensure that script files are only installed if really needed, one could easily use cmake's file(GLOB ...) to check for the existence of generated script files.

@hidmic, would you accept a PR to make SCRIPTS_DESTINATION optional, using lib/${PROJECT_NAME} by default?
Do you plan to backport this feature to Foxy as well (I have seen a backport to Galactic only)?

@hidmic
Copy link
Contributor Author

hidmic commented Jan 3, 2022

would you accept a PR to make SCRIPTS_DESTINATION optional, using lib/${PROJECT_NAME} by default?

@rhaschke IIRC we force explicit request so as to not populate the install space with empty directories (in case no scripts are declared in the setup.py file). If you have a proposal that works around that defect, it's more than welcome!

Do you plan to backport this feature to Foxy as well (I have seen a backport to Galactic only)?

@rhaschke as a general rule of thumb, we don't backport features. Having said that, it should be a fairly innocuous backport. I'm onboard if @jacobperron is.

@jacobperron
Copy link
Contributor

I have no objection is folks want to backport this to Foxy.

@LaneaLucy
Copy link

hi. trying to compile foxy on raspberry pi zero. but sadly my project needs xacro which only has one ros2 branch which uses the new "ament_python_install_package" syntax, but because this is not backported to foxy, i get

ament_python_install_package() called with unused arguments:
  SCRIPTS_DESTINATION;lib/xacro

fatal error and it stops to compile.
so i would really appreciate the backport.
thx in advance

@clalancette
Copy link
Contributor

so i would really appreciate the backport.

Foxy is now End-of-Life, so we don't have a way to do backports there. I encourage you to try out Humble, where this bug should already be fixed.

@LaneaLucy
Copy link

But would this run on raspberry pi zero 1? Because compiling on it takes days, and after days compiling to find out, it don't work, wouldn't be great...

@rhaschke
Copy link
Contributor

rhaschke commented Dec 8, 2023

@LaneaLucy, you could simply revert the corresponding xacro commit. No need to rebuilt in this case ;-)

@LaneaLucy
Copy link

@rhaschke I'm now using the last tag of xacro that don't use the new format. It's compiling now, but this will take at least some hours before it gets to the xacro part

@rhaschke
Copy link
Contributor

rhaschke commented Dec 8, 2023

You don't need to rebuilt packages if you just changed xacro.

@LaneaLucy
Copy link

@rhaschke I'm compiling ROS2 from source and it don't re compile packages without change, but on a raspi zero it still takes some time (15s-2min) for colcon to figure out if it needs to compile. And I'm still very new to ros and that's my first ros2 build from source

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.

None yet

6 participants