Skip to content

Simplify ament_python_install_package() macro.#326

Merged
hidmic merged 1 commit intomasterfrom
hidmic/install-flat-egg-manually
Mar 9, 2021
Merged

Simplify ament_python_install_package() macro.#326
hidmic merged 1 commit intomasterfrom
hidmic/install-flat-egg-manually

Conversation

@hidmic
Copy link
Copy Markdown

@hidmic hidmic commented Mar 9, 2021

Alternative to #316, #323, #324, #325. This patch no longer delegates to setuptools, it simply builds and installs an .egg-info directory manually. It's a bit hacky, but it's less fragile than the original approach.

CI up to test_communication (to exercise rclpy and rosidl packages):

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

CI up to test_communication with --symlink-install:

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

@hidmic
Simplify ament_python_install_package() macro.
d17f2b7 
Do not delegate to setuptools, install egg-info manually.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic requested review from ivanpauno and sloretz March 9, 2021 14:29
ivanpauno
ivanpauno approved these changes Mar 9, 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.

Sounds reasonable to me

@hidmic
Copy link
Copy Markdown
Author

hidmic commented Mar 9, 2021

Alright, CI is green, reviewer's happy. Going in !

@hidmic hidmic merged commit 0cdd601 into master Mar 9, 2021
@delete-merged-branch delete-merged-branch bot deleted the hidmic/install-flat-egg-manually branch March 9, 2021 17:39
This was referenced Mar 9, 2021
Closed
Merged
Closed
ivanpauno
ivanpauno reviewed May 27, 2021
View reviewed changes
ament_cmake_python/CMakeLists.txt

find_package(ament_cmake_core REQUIRED)

set(ament_cmake_python_DIR "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
Copy link
Copy Markdown
Contributor

@ivanpauno ivanpauno May 27, 2021

Choose a reason for hiding this comment

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

@hidmic I think we should've deleted the test folder, right?

Copy link
Copy Markdown
Author

@hidmic hidmic May 27, 2021

Choose a reason for hiding this comment

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

Yeap. I'll send a PR.

hidmic added a commit that referenced this pull request May 28, 2021
@hidmic
Drop ament_cmake_python outdated tests.
feedacd 
Follow-up after #326. Should've been removed then.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic mentioned this pull request May 28, 2021
Merged
hidmic added a commit that referenced this pull request May 28, 2021
@hidmic
Drop ament_cmake_python outdated tests. (#340)
18d1515 
Follow-up after #326. Should've been removed then.

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

lukicdarkoo commented Aug 14, 2021

@hidmic It seems this PR breaks ability to create Python subpackages. Before (in Foxy) I was able to:

ament_python_install_package(${PROJECT_NAME}/subpackage
  PACKAGE_DIR src/mypackage)

and it would create a subpackage. In Rolling it shows the following error:

09:18:20 CMake Error at /opt/ros/rolling/share/ament_cmake_python/cmake/ament_python_install_package.cmake:98 (add_custom_target):
09:18:20   add_custom_target called with invalid target name
09:18:20   "ament_cmake_python_symlink_webots_ros2_driver/webots".  Target names may
09:18:20   not contain a slash.  Use ADD_CUSTOM_COMMAND to generate files.
09:18:20 Call Stack (most recent call first):
09:18:20   /opt/ros/rolling/share/ament_cmake_python/cmake/ament_python_install_package.cmake:33 (_ament_cmake_python_install_package)
09:18:20   CMakeLists.txt:70 (ament_python_install_package)
09:18:20 
09:18:20 
09:18:20 -- Configuring incomplete, errors occurred!

Is there a workaround or it is broken on purpose?

@ivanpauno
Copy link
Copy Markdown
Contributor

ivanpauno commented Aug 18, 2021

@hidmic It seems this PR breaks ability to create Python subpackages. Before (in Foxy) I was able to:

IIRC the first argument was always expected to be a package name, which shouldn't contain any / character.
The fact that it accepted / characters at some point was accidental (I think we found a similar issue in a package in the core, but I cannot find the PR).

If you have many package you can still use ament_python_install_package() many times, no need to use / in the package name.
If you have one package with many subpackages you should be using ament_python_install_package() once.

@rkent rkent mentioned this pull request Jun 27, 2025
Open
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

@sloretz sloretz Awaiting requested review from sloretz

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hidmic @lukicdarkoo @ivanpauno