Skip to content

Make ament_python_install_package() install console_scripts#328

Merged
hidmic merged 3 commits intomasterfrom
hidmic/ament-python-install-package-scripts
May 10, 2021
Merged

Make ament_python_install_package() install console_scripts#328
hidmic merged 3 commits intomasterfrom
hidmic/ament-python-install-package-scripts

Conversation

@hidmic
Copy link
Copy Markdown

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

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
Copy Markdown
Contributor

A nice to have, addressing #213.

Nice!

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

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

@LaneaLucy
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown
Contributor

rhaschke commented Dec 8, 2023

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

@LaneaLucy
Copy link
Copy Markdown

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

6 participants