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

Depend on rosbag2_storage before pluginlib #623

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jan 28, 2021

This fixes #583 for me. It puts the includes for rosbag2_storage before the includes for pluginlib. Because pluginlib is not in the overlay described in #583, the include path it adds first is /opt/ros/include. That causes it to find the includes for rosbag2_storage in /opt/ros/include, not the includes for the one built in install/rosbag2_storage/include.

Maybe this means things in ament_target_dependencies() should not automatically be alphabetized? It's not clear to me what should happen instead. Including packages in the same repo first makes sense, but what if I had an overlay workspace with ros2_storage_default_plugins from the release repo and pluginlib? Then the order added by this PR would be wrong. There probably needs to be a change in ament_target_dependencies() to reorder things so that things get depended on in workspace order no matter what.

@pjreed FYI

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@emersonknapp
Copy link
Collaborator

Maybe this means things in ament_target_dependencies() should not automatically be alphabetized? It's not clear to me what should happen instead. Including packages in the same repo first makes sense, but what if I had an overlay workspace with ros2_storage_default_plugins from the release repo and pluginlib? Then the order added by this PR would be wrong. There probably needs to be a change in ament_target_dependencies() to reorder things so that things get depended on in workspace order no matter what.

I agree that it would be ideal for ament_target_dependencies to handle this for us.

That being said - this seems fine. Could you add a comment to the CMakelists explaining why it's necessary? Otherwise I can see somebody moving it without knowing it was intentional.

@Karsten1987
Copy link
Collaborator

@sloretz friendly ping. Is this still a problem?

@sloretz
Copy link
Contributor Author

sloretz commented Mar 10, 2021

@sloretz friendly ping. Is this still a problem?

IIRC is solved the workspace overlay problem for this one particular situation, but I don't think it's possible to know the right order at the package level because it depends on what packages are in the underlay.

I think it makes sense to close this PR, and document it somewhere as a limitation with overlaying merged workspaces.

@sloretz sloretz closed this Mar 10, 2021
@sloretz sloretz deleted the sloretz__rosbag2_storage_default_plugins__find_rosbag2_storage_first branch March 11, 2021 16:06
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.

Can't build master branch against rolling release
3 participants