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

Various improvements to ament_python_install_package #372

Open
wants to merge 7 commits into
base: rolling
Choose a base branch
from

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Jan 6, 2022

This PR implements various improvements to ament_python_install_package:

  • Make SCRIPTS_DESTINATION argument optional as suggested in Make ament_python_install_package() install console_scripts #328 (comment)
    By using install(CODE ...) to directly generate into the target location we can still avoid creating an empty target folder.
    Generating into build folder and (potentially) creating a symlink from this build to the install folder, wasn't good practice anyway.
  • Support multiple SCRIPTS_DESTINATIONs
    This is, for example, required by xacro, which installs to lib/xacro and bin

If this got merged, I will create backport PRs for Foxy and Galactic.

The scripts are generated into a build folder and thus *always* need to be copied.
No need to distinguish between symlink and other installs.

Signed-off-by: Robert Haschke <[email protected]>
This avoids the problem of empty install folders as setup.py
generates the target folder only if needed.

Signed-off-by: Robert Haschke <[email protected]>
Signed-off-by: Robert Haschke <[email protected]>
For example, xacro is install into lib/xacro/xacro and bin/xacro.

Signed-off-by: Robert Haschke <[email protected]>
@rhaschke
Copy link
Contributor Author

@hidmic, @jacobperron, could you please review? This is blocking ros/xacro#304. Thanks a lot in advance.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Need to test this locally, but overall LGTM.

)
if(POLICY CMP0087)
# Allow generator expressions in install(CODE...)
cmake_policy(SET CMP0087 NEW)
Copy link
Contributor

Choose a reason for hiding this comment

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

@rhaschke which generator expressions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't use generator expressions, but cmake complains about the unset CMP otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning is even reported by CI: https://ci.ros2.org/job/ci_linux/16094/cmake/NORMAL

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I wonder if a generator expression is introduced by build_dir. Anyways, we need to cap that warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what I did. How do you trigger Full CI on this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you trigger Full CI on this PR?

Using our ci_launcher with a custom ros2.repos file (see https://ci.ros2.org/job/ci_launcher/9781).

Yes, that's what I did.

What's funny is that I run CI yesterday and there have not been changes to this PR in weeks. Maybe fe6b3a3 was not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm really puzzled. Indeed, the previous builds didn't show the warning - although install(CODE ...) was already used:

# compile Python files
install(CODE

However, locally, the suggested change exactly silences the warning for me. I have no clue, why it fails on CI.
My suggestion is to drop it and retry.

@hidmic
Copy link
Contributor

hidmic commented Feb 7, 2022

Full CI, to be safe:

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

This reverts commit fe6b3a3.

Signed-off-by: Robert Haschke <[email protected]>
@hidmic
Copy link
Contributor

hidmic commented Feb 8, 2022

Alright, let's give it another shot on Linux only for speed:

  • Linux Build Status

@rhaschke
Copy link
Contributor Author

rhaschke commented Feb 8, 2022

The ROS2 build farm confirms that the suggested change is required:
https://build.ros2.org/job/Rpr__ament_cmake__ubuntu_focal_amd64/173

I still have no clue why your full CI fails anyway. How should we proceed now?

@hidmic
Copy link
Contributor

hidmic commented Feb 8, 2022

Hmm, it's just a hunch but I suspect this is an issue with CMake policy scopes.

If you look at the console output for, say, full Linux CI, before cc7ad4e, you'll see that most of the packages that generate warnings are packages that build interfaces.

That's an important detail because ament_python_install_package() gets invoked deep inside the rosidl machinery for Python. None of this will show in the local Rpr job because it only builds the packages in this repository.

Could it be that by the time your install code runs, CMP0087 has been popped off the stack?

@rhaschke
Copy link
Contributor Author

rhaschke commented Feb 9, 2022

Looking into the cmake traces of an old and new build of lifecycle_msgs, I'm even more confused:
Often cmake_policy(VERSION 2.6) is called (push/pop policy), essentially unsetting CMP0087, which was introduced in 3.14 only.
However, cmake doesn't complain about install(code ...) usage in this case. Why??

# generate/install entry-point console scripts
foreach(_dest ${ARG_SCRIPTS_DESTINATION})
get_filename_component(ABS_SCRIPTS_DESTINATION "${_dest}" ABSOLUTE BASE_DIR "${CMAKE_INSTALL_PREFIX}")
install(CODE "execute_process(COMMAND ${python_interpreter} setup.py install_scripts --install-dir \"${ABS_SCRIPTS_DESTINATION}\"
Copy link
Contributor

Choose a reason for hiding this comment

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

${python_interpreter} comes from

get_executable_path(python_interpreter Python3::Interpreter BUILD)

The BUILD argument will result in the path being a generator expression:

set(output_var "$<TARGET_PROPERTY:${target_or_path},LOCATION>")

A workaround to make the install(CODE warnings go away is to call get_executable_path again with the CONFIGURE arugment.

 get_executable_path(python_interpreter_config Python3::Interpreter CONFIGURE)
 # ...
install(CODE "execute_process(COMMAND ${python_interpreter} setup.py ...

The same workaround is already used a little lower in the file

get_executable_path(python_interpreter_config Python3::Interpreter CONFIGURE)
# compile Python files
install(CODE
"execute_process(
COMMAND
\"${python_interpreter_config}\" \"-m\" \"compileall\"
\"${CMAKE_INSTALL_PREFIX}/${ARG_DESTINATION}/${package_name}\"
)"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for this valuable hint. I adapted the code accordingly - sorry for the long delay.
Should be ready for merging now.

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:14
@rhaschke
Copy link
Contributor Author

Is there any chance that this PR will get merged?

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.

3 participants