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

Simplify ament_python_install_package() macro. #326

Merged
merged 1 commit into from
Mar 9, 2021

Conversation

hidmic
Copy link
Contributor

@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

Do not delegate to setuptools, install egg-info manually.

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

Sounds reasonable to me

@hidmic
Copy link
Contributor Author

hidmic commented Mar 9, 2021

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

@@ -4,31 +4,6 @@ project(ament_cmake_python NONE)

find_package(ament_cmake_core REQUIRED)

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

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

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
Follow-up after #326. Should've been removed then.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
hidmic added a commit that referenced this pull request May 28, 2021
Follow-up after #326. Should've been removed then.

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

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

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

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

3 participants