Skip to content

Conversation

nwn
Copy link

@nwn nwn commented Oct 17, 2024

For systems not using colcon's or ament's .dsv files for initializing the environment, having only "dummy" .sh hooks means that the GZ_CONFIG_PATH environment variable does not get set correctly. The proposed change is to define a "real" .sh hook with the proper logic to initialize the environment.

This generates the exact same output as was proposed in gazebo-release/gz_common_vendor#5.

This correctly sets the GZ_CONFIG_PATH in the same way as the .dsv file.

Signed-off-by: Nathan Wiebe Neufeldt <nwiebeneufeldt@clearpath.ai>
@nwn
Copy link
Author

nwn commented Nov 7, 2024

Ping @azeey for review

1 similar comment
@nwn
Copy link
Author

nwn commented Nov 29, 2024

Ping @azeey for review

@azeey
Copy link
Collaborator

azeey commented Dec 9, 2024

Sorry for the delay. I'll plan to review this this week.

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

This mostly looks good, and I've tested that it doesn't break any existing behavior. But I'm not sure how to test that it'll actually work as intended.

@@ -0,0 +1,3 @@
if [ -d "$AMENT_CURRENT_PREFIX/opt/@PROJECT_NAME@/share/gz" ]; then
ament_prepend_unique_value GZ_CONFIG_PATH "$AMENT_CURRENT_PREFIX/opt/@PROJECT_NAME@/share/gz"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tested this by building a colcon workspace with the vendor packages and deleting the .dsv files in the install directories. What I found is that since $AMENT_CURRENT_PREFIX was /opt/ros/rolling, the condition here is never satisfied and GZ_CONFIG_PATH is never set. So I guess this would only work if you're installing the vendor packages to /opt/ros/<ros-version>?

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