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

AMENT_PREFIX_PATH: Environment variable vs CMake variable #713

Open
furushchev opened this issue Dec 19, 2023 · 3 comments
Open

AMENT_PREFIX_PATH: Environment variable vs CMake variable #713

furushchev opened this issue Dec 19, 2023 · 3 comments

Comments

@furushchev
Copy link

I found the CMake variable AMENT_PREFIX_PATH is set in the following files in this repository:

-DAMENT_PREFIX_PATH="@(InstallationPrefix)" \

-DAMENT_PREFIX_PATH="@(InstallationPrefix)" \

However, the variable is not assumed to be the CMake variable but the environment variable in the ament/ament_cmake repository.
for instance:
https://github.com/ament/ament_cmake/blob/dc576abb40986f9d1ce8e534eb17770a625014cd/ament_cmake_core/cmake/environment/ament_cmake_environment_package_hook.cmake#L33

I want to clarify if there is any background to have this variable set as the environment variable or if it just needs to be fixed.
(I'll make a PR if it's the latter case.)

@furushchev furushchev changed the title AMENT_PREFIX_PATH: Environment variables vs CMake variables AMENT_PREFIX_PATH: Environment variable vs CMake variable Dec 19, 2023
@furushchev
Copy link
Author

@cottsay Hi! Do you have any insights on this topic? (I mentioned you because the change above was committed by you)
0dc2646

@meetgandhi-dev
Copy link

@furushchev
I believe, you're correct.
Do you have a fix for the same?

@cottsay
Copy link
Member

cottsay commented Nov 19, 2024

Hmm, I think these lines can be removed from the templates you referenced. From what I can tell, AMENT_PREFIX_PATH is only ever set and only ever used as an environment variable and never as a CMake variable. In this context, sourcing the setup scripts sets the variable appropriately.

We should survey the ROS 2 codebase to be absolutely sure nobody is relying on this variable being set in this way before merging a change to drop it.

Keep in mind that defining the variable shouldn't cause any misbehavior, it's just misleading.

@nuclearsandwich originally added the flag to the ament_cmake templates - do you have any context to add?

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

No branches or pull requests

3 participants