-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add colcon.pkg with gz-cmake4 dependency #142
Conversation
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@@ -0,0 +1,8 @@ | |||
# Configuration file for colcon (https://colcon.readthedocs.io). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also want to mention why we are adding this file here so we remember the original cause to introduce the file and listing only a subset of the versions that the sotfware can depend on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments added in 805d1fd
# - https://colcon.readthedocs.io/en/released/user/configuration.html#colcon-pkg-files | ||
|
||
{ | ||
"dependencies": ["gz-cmake4"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"dependencies": ["gz-cmake4"], | |
"build-dependencies": ["gz-cmake4"], |
Quoting: https://colcon.readthedocs.io/en/released/user/configuration.html#colcon-pkg-files
dependencies (list of strings) to declare additional dependencies on other packages. For more fine grain control it is also possible to set build-dependencies, run-dependencies, and test-dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tricky.
Is gz-cmake4
required when a package which depends on gz-tools
is built? If so, it would also need to be in run-dependencies
. At that point it's probably easier to leave it as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this as is for now and merge to fix CI. We can follow up with other approaches after CI is fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested this locally and it seems to work great! Thanks @scpeters
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
🦟 Bug fix
Fixes
colcon
build ordering since #136 was mergedSummary
Since #128,
gz-tools2
supports building with eithergz-cmake3
orgz-cmake4
, andcolcon
was correctly identifying the dependency relationship due to the cmakefind_package
calls. Apackage.xml
was added in #136 with abuild_depend
only ongz-cmake3
, which now breaks the build-from-source order for Ionic workspaces that includegz-tools2
:This pull request adds a
colcon.pkg
file with an explicit dependency ongz-cmake4
to fix the build orderAlternatives considered
colcon.pkg
to force the build type back tocmake
: f18e584. This also fixes the build (), but it has more side-effects that just addinggz-cmake4
as a dependency.<member_of_group>gz-cmake</member_of_group>
to thepackage.xml
file ingz-cmake3
andgz-cmake4
and a<group_depend>gz-cmake</group_depend>
tag in downstream package.xml files (thanks to @cottsay for the suggestion). This is more elegant but requires more coordination between packages, while thiscolcon.pkg
change can fix CI now.Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.