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

Remove unused AMENT_CMAKE_ENVIRONMENT_GENERATION option #354

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

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Aug 5, 2021

This removes a CMake option AMENT_CMAKE_ENVIRONMENT_GENERATION that does not appear to be used anywhere, and is using scripts that I can only find uses of in the abandoned ament_tools project.

I came across this while looking at upgrading ament_cmake to use FindPython3 instead of FindPythonInterp. These "prefix_level" scripts reference PYTHON_EXECUTABLE from FindPythonInterp, but as far as I can tell they're not used anywhere, and haven't been since ament_tools.

Part of ros2/python_cmake_module#6
Part of ament/ament_package#133

@sloretz sloretz self-assigned this Aug 5, 2021
@sloretz
Copy link
Contributor Author

sloretz commented Aug 6, 2021

CI lgtm

Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

I'm not sure how much we need to consider "legacy" but I'll share what I recollect about this script option.

All catkin packages generate prefix level environment scripts by default unless CATKIN_BUILD_BINARY_PACKAGE is true or CATKIN_INSTALL_INTO_PREFIX_ROOT is set to false.

In ament the environment generation is off by default and, as you mention, colcon currently provides its own implementation of the prefix-level environment scripts for our recommended build-from-source workflow.

However, the prefix-level scripts for binary packages are provided by the ros_workspace package which calls ament_generate_environment() from ament_cmake_core directly thus skipping the AMENT_CMAKE_ENVIRONMENT_GENERATION option.

With all that in mind, I'm not sure whether the option provides any value but we have to keep the rest of the content around for the ros_workspace package.

@sloretz
Copy link
Contributor Author

sloretz commented Aug 6, 2021

@nuclearsandwich mind taking a second look? I've updated this PR to only remove the CMake option, and the other PR to only remove the isolated_prefix_level scripts which still seem to be unused.

@sloretz sloretz dismissed nuclearsandwich’s stale review August 6, 2021 21:45

Added back still used code

@sloretz sloretz requested a review from nuclearsandwich August 6, 2021 21:47
Signed-off-by: Shane Loretz <[email protected]>
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:14
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.

2 participants