Reapply "Use system installed yaml-cpp 0.6 if available (#8)" #16
Conversation
…15)" This reverts commit 251be27. Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
ivanpauno
force-pushed
the
reapply/use-system-installed-version
branch
from
c192a7d to
ec8e73b
Compare
|
@seanyen FYI |
|
CI with the new debug message: https://ci.ros2.org/job/ci_osx/9833/. |
|
@seanyen there's something quite weird here, this seems to be finding a previously downloaded version of Maybe we're not cleaning up the workspace in our macOS machines between builds, that would be pretty weird. Maybe there's a way to call |
|
@ivanpauno Hi, is it possible to turn on |
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Enabled in 46ede7b. Maybe this behavior is system independent, and we're only running into the bug on macOS because of not pruning the old workspace.
|
find_package debug output@seanyen FYI |
|
@ivanpauno Thanks for the logs. Ok, the CMake config files are discovered from There is a |
Doing that would make builds more reproducible, but we still need That could be fixed by always installing the environment hook, but I think it's better to only look for the "system installed" yaml-cpp version. I will reread find_package docs and commit a fix. |
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
|
New CI, testing
@seanyen does this make sense to you? |
I don't have any objections adding |
|
@ivanpauno @dirk-thomas Just a friendly ping. Do we have any plans for the macOS build breaks? |
I think the current patch is fine, as it is an improvement over how it used to work and it doesn't introduce any new problems. |
|
I don't see how applying this patch to this single package can be the right thing to do. Imo it either needs to be applied globally or to all affected packages individually (in this case this patch if fine but the other packages needing the same flag should be identified as part of this ticket). |
The focus of the PR isn't adding the usage of
Though we could force colcon to use Here's a complete list of packages that would need a similar change:
All other vendor packages aren't using the "try find_package then build" approach. I will create a ticket with this list in |
I wouldn't call this an "unrelated detail" if the whole point about I just raised the concern that the actual question how we should handle this hasn't been answered yet. Please feel free to merge before that is the case if you think that is fine. |
I have created a ticket to follow-up in the discussion ros2/ros2#1021.
I think I have answered how I think it should be handled in #16 (comment), #16 (comment). I understand that we would like to solve the problem everywhere, but I think that a clear action item for the original author was missing.
I think I have satisfied the only missing request of opening a ticket to follow-up in the |
👍 we can follow-up in how we want to handle this in ros2/ros2#1021. |
delete-merged-branch
bot
deleted the
reapply/use-system-installed-version
branch
|
Thanks @seanyen ! |
|
@ivanpauno This ticket is on the Foxy and Dashing release boards, can you take care of backporting this change (if applicable). Thanks! |
I don't think we need to backport this. |
Reapplies #8.
I will try to see why it was failing on macOS, by adding some extra debug messages.