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

Reapply "Use system installed yaml-cpp 0.6 if available (#8)" #16

Merged
merged 4 commits into from
Sep 17, 2020

Conversation

ivanpauno
Copy link
Member

Reapplies #8.

I will try to see why it was failing on macOS, by adding some extra debug messages.

@ivanpauno ivanpauno added the enhancement New feature or request label Aug 25, 2020
…15)"

This reverts commit 251be27.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the reapply/use-system-installed-version branch from c192a7d to ec8e73b Compare August 25, 2020 14:08
@ivanpauno
Copy link
Member Author

@seanyen FYI

@ivanpauno ivanpauno added the help wanted Extra attention is needed label Aug 25, 2020
@ivanpauno
Copy link
Member Author

CI with the new debug message: https://ci.ros2.org/job/ci_osx/9833/.

@ivanpauno
Copy link
Member Author

@seanyen there's something quite weird here, this seems to be finding a previously downloaded version of yaml-cpp in the same workspace:

Found yaml-cpp 0.6.2 in path /Users/osrf/jenkins-agent/workspace/nightly_osx_extra_rmw_release/ws/build/yaml_cpp_vendor/yaml_cpp-0f9a586-prefix/src/yaml_cpp-0f9a586-build/yaml-cpp-config.cmake

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 find_package that avoid this ... (NO_CMAKE_BUILDS_PATH ?)

@seanyen
Copy link
Contributor

seanyen commented Aug 25, 2020

@ivanpauno Hi, is it possible to turn on CMAKE_FIND_DEBUG_MODE? I think it would be helpful to understand the CMake search order (in macOS).

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

@ivanpauno Hi, is it possible to turn on CMAKE_FIND_DEBUG_MODE? I think it would be helpful to understand the CMake search order (in macOS).

Enabled in 46ede7b.
new CI: https://ci.ros2.org/job/ci_osx/9840/


Maybe this behavior is system independent, and we're only running into the bug on macOS because of not pruning the old workspace.
I imagine this can be tested in the following way:

  • Build without system yaml
  • Modify something in the CMakelists.txt (e.g. add a debug message), so that cmake does something in next build
  • Check if find_package founds the previously downloaded version or not

@ivanpauno
Copy link
Member Author

find_package debug output
17:54:29   find_package considered the following paths for yaml-cpp.cmake
17:54:29 
17:54:29     /usr/local/Cellar/cmake/3.17.2/share/cmake/Modules/Findyaml-cpp.cmake
17:54:29 
17:54:29   The file was not found.
17:54:29 
17:54:29   <PackageName>_ROOT CMake variable [CMAKE_FIND_USE_PACKAGE_ROOT_PATH].
17:54:29 
17:54:29     none
17:54:29 
17:54:29   CMAKE_PREFIX_PATH variable [CMAKE_FIND_USE_CMAKE_PATH].
17:54:29 
17:54:29     none
17:54:29 
17:54:29   CMAKE_FRAMEWORK_PATH and CMAKE_APPBUNDLE_PATH variables
17:54:29   [CMAKE_FIND_USE_CMAKE_PATH].
17:54:29 
17:54:29     none
17:54:29 
17:54:29   Env variable yaml-cpp_DIR [CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH].
17:54:29 
17:54:29     none
17:54:29 
17:54:29   CMAKE_PREFIX_PATH env variable [CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH].
17:54:29 
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_test
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_target_dependencies
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_targets
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_interfaces
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_dependencies
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_version
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_python
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_libraries
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_include_directories
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_link_flags
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_libraries
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_include_directories
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_definitions
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_core
17:54:29     /usr/local/opt/qt
17:54:29 
17:54:29   
17:54:29 
17:54:29   CMAKE_FRAMEWORK_PATH and CMAKE_APPBUNDLE_PATH env variables
17:54:29   [CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH].
17:54:29 
17:54:29   
17:54:29 
17:54:29   Paths specified by the find_package HINTS option.
17:54:29 
17:54:29     none
17:54:29 
17:54:29   Standard system environment variables
17:54:29   [CMAKE_FIND_USE_SYSTEM_ENVIRONMENT_PATH].
17:54:29 
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/venv
17:54:29     /usr/local/opt/ccache/libexec
17:54:29     /usr/local
17:54:29     /usr
17:54:29     /
17:54:29 
17:54:29   
17:54:29 
17:54:29   CMake User Package Registry [CMAKE_FIND_USE_PACKAGE_REGISTRY].
17:54:29 
17:54:29     /Users/osrf/jenkins-agent/workspace/packaging_osx/ws/build/yaml_cpp_vendor/yaml_cpp-0f9a586-prefix/src/yaml_cpp-0f9a586-build
17:54:29 
17:54:29   
17:54:29 
17:54:29   CMake variables defined in the Platform file
17:54:29   [CMAKE_FIND_USE_CMAKE_SYSTEM_PATH].
17:54:29 
17:54:29     /usr/local/Cellar/cmake/3.17.2
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/yaml_cpp_vendor
17:54:29     /usr/X11R6
17:54:29     /usr/pkg
17:54:29     /opt
17:54:29     /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr
17:54:29     /sw
17:54:29     /opt/local
17:54:29     /Users/osrf/Library/Frameworks
17:54:29     /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/Library/Frameworks
17:54:29     /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/Network/Library/Frameworks
17:54:29     /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks
17:54:29     /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/Library/Frameworks
17:54:29     /Applications/Xcode.app/Contents/Developer/Library/Frameworks
17:54:29     /Library/Frameworks
17:54:29     /Network/Library/Frameworks
17:54:29     /System/Library/Frameworks
17:54:29     /Applications
17:54:29     /Applications/Xcode.app/Contents/Applications
17:54:29     /Applications/Xcode.app/Contents/Developer/Applications
17:54:29 
17:54:29   
17:54:29 
17:54:29   CMake System Package Registry
17:54:29   [CMAKE_FIND_PACKAGE_NO_SYSTEM_PACKAGE_REGISTRY].
17:54:29 
17:54:29     none
17:54:29 
17:54:29   Paths specified by the find_package PATHS option.
17:54:29 
17:54:29     none
17:54:29 
17:54:29   find_package considered the following locations for the Config module:
17:54:29 
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_test/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_test/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_target_dependencies/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_target_dependencies/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_targets/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_targets/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_interfaces/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_interfaces/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_dependencies/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_dependencies/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_version/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_version/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_python/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_python/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_libraries/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_libraries/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_include_directories/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_include_directories/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_link_flags/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_link_flags/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_libraries/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_libraries/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_include_directories/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_include_directories/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_definitions/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_export_definitions/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_core/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/ament_cmake_core/yaml-cpp-config.cmake
17:54:29     /usr/local/opt/qt/yaml-cppConfig.cmake
17:54:29     /usr/local/opt/qt/yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/venv/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/ci_osx/venv/yaml-cpp-config.cmake
17:54:29     /usr/local/opt/ccache/libexec/yaml-cppConfig.cmake
17:54:29     /usr/local/opt/ccache/libexec/yaml-cpp-config.cmake
17:54:29     /usr/local/yaml-cppConfig.cmake
17:54:29     /usr/local/yaml-cpp-config.cmake
17:54:29     /usr/yaml-cppConfig.cmake
17:54:29     /usr/yaml-cpp-config.cmake
17:54:29     /yaml-cppConfig.cmake
17:54:29     /yaml-cpp-config.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/packaging_osx/ws/build/yaml_cpp_vendor/yaml_cpp-0f9a586-prefix/src/yaml_cpp-0f9a586-build/yaml-cppConfig.cmake
17:54:29     /Users/osrf/jenkins-agent/workspace/packaging_osx/ws/build/yaml_cpp_vendor/yaml_cpp-0f9a586-prefix/src/yaml_cpp-0f9a586-build/yaml-cpp-config.cmake
17:54:29 
17:54:29   The file was found at
17:54:29 
17:54:29     /Users/osrf/jenkins-agent/workspace/packaging_osx/ws/build/yaml_cpp_vendor/yaml_cpp-0f9a586-prefix/src/yaml_cpp-0f9a586-build/yaml-cpp-config.cmake
17:54:29 
17:54:29 
17:54:29 
17:54:29 -- Found yaml-cpp 0.6.2 in path /Users/osrf/jenkins-agent/workspace/packaging_osx/ws/build/yaml_cpp_vendor/yaml_cpp-0f9a586-prefix/src/yaml_cpp-0f9a586-build/yaml-cpp-config.cmake

@seanyen FYI

@seanyen
Copy link
Contributor

seanyen commented Aug 25, 2020

@ivanpauno Thanks for the logs. Ok, the CMake config files are discovered from CMake User Package Registry.

CMake User Package Registry [CMAKE_FIND_USE_PACKAGE_REGISTRY].
/Users/osrf/jenkins-agent/workspace/packaging_osx/ws/build/yaml_cpp_vendor/yaml_cpp-0f9a586-prefix/src/yaml_cpp-0f9a586-build

There is a NO_CMAKE_PACKAGE_REGISTRY option to skip the discovery, but I think for CI service, it makes more sense to nuke ~/.cmake/packages/ for reproducible builds.

@ivanpauno
Copy link
Member Author

There is a NO_CMAKE_PACKAGE_REGISTRY option to skip the discovery, but I think for CI service, it makes more sense to nuke ~/.cmake/packages/ for reproducible builds.

Doing that would make builds more reproducible, but we still need NO_CMAKE_PACKAGE_REGISTRY (and maybe some of the other restrictions).
Without that, workspace A can find the library downloaded in workspace B, and people will run into this same issue.

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 <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

New CI, testing rosbag2_tests:

  • macOS Build Status

@seanyen does this make sense to you?

@ivanpauno ivanpauno self-assigned this Aug 26, 2020
@ivanpauno ivanpauno removed the help wanted Extra attention is needed label Aug 26, 2020
@seanyen
Copy link
Contributor

seanyen commented Aug 26, 2020

Doing that would make builds more reproducible, but we still need NO_CMAKE_PACKAGE_REGISTRY (and maybe some of the other restrictions).
Without that, workspace A can find the library downloaded in workspace B, and people will run into this same issue.

I don't have any objections adding NO_CMAKE_PACKAGE_REGISTRY (Personally I am not a fan for CMake package registry). As a user building ROS from sources heavily, I think it would be helpful to mention this problem in the ROS2 docs and point people to Disabling the Package Registry, in case he/she wants a isolated build.

@seanyen
Copy link
Contributor

seanyen commented Sep 3, 2020

@ivanpauno @dirk-thomas Just a friendly ping. Do we have any plans for the macOS build breaks?

@ivanpauno
Copy link
Member Author

@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.
@dirk-thomas can give you feedback of what's missing.

@ivanpauno ivanpauno removed their assignment Sep 3, 2020
@dirk-thomas
Copy link
Member

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).

@ivanpauno
Copy link
Member Author

I don't see how applying this patch to this single package can be the right thing to do

The focus of the PR isn't adding the usage of NO_CMAKE_PACKAGE_REGISTRY, it is to avoid building yaml_cpp_vendor if you already have a system installed version or if you have already downloaded it in an underlay workspace.
IMHO we shouldn't block a PR because of an unrelated detail.

Imo it either needs to be applied globally or to all affected packages individually

Though we could force colcon to use NO_CMAKE_PACKAGE_REGISTRY globally by default, there might be some people that want to use the package registry. Adding NO_CMAKE_PACKAGE_REGISTRY in the find_package calls where using the package registry is a problem seems appropriate to me.
Particularly, I think that all vendor packages using the "try find_package, build if it doesn't succeed" approach should be using that option.

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 ros2/ros2 after this gets merged.

@dirk-thomas
Copy link
Member

IMHO we shouldn't block a PR because of an unrelated detail.

I wouldn't call this an "unrelated detail" if the whole point about NO_CMAKE_PACKAGE_REGISTRY is that without it this patch doesn't work as intended since it shows cross talk between unrelated workspaces.

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.

@ivanpauno
Copy link
Member Author

I wouldn't call this an "unrelated detail" if the whole point about NO_CMAKE_PACKAGE_REGISTRY is that without it this patch doesn't work as intended since it shows cross talk between unrelated workspaces.

I have created a ticket to follow-up in the discussion ros2/ros2#1021.
One of the subitems of that ticket is revisiting the usage of NO_CMAKE_PACKAGE_REGISTRY here if we later decide to do something different.

I just raised the concern that the actual question how we should handle this hasn't been answered yet

I think I have answered how I think it should be handled in #16 (comment), #16 (comment).
I think that globally disabling the package registry might not be an option to everybody, and locally disabling it in the places it's known to cause problem seems correct.

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 that asking a contributor to check every vendor package in the project when they are proposing an enhancement in one is a bit to much (though I opened this PR, @seanyen is the author).

Please feel free to merge before that is the case if you think that is fine.

I think I have satisfied the only missing request of opening a ticket to follow-up in the NO_CMAKE_PACKAGE_REGISTRY discussion and listing every package where this is a problem, so I would like to either get an approval or a comment stating what else is missing.

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Repeating my previous comment:

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.

@ivanpauno
Copy link
Member Author

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.

👍 we can follow-up in how we want to handle this in ros2/ros2#1021.

@ivanpauno ivanpauno merged commit 40ad34b into master Sep 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the reapply/use-system-installed-version branch September 17, 2020 17:43
@ivanpauno
Copy link
Member Author

Thanks @seanyen !

@dirk-thomas dirk-thomas added this to Proposed in Foxy Patch Release 4 via automation Oct 23, 2020
@dirk-thomas dirk-thomas added this to Needs Backport in Dashing Patch Release 8 Oct 23, 2020
@dirk-thomas dirk-thomas moved this from Proposed to Needs backport in Foxy Patch Release 4 Oct 23, 2020
@jacobperron
Copy link
Member

@ivanpauno This ticket is on the Foxy and Dashing release boards, can you take care of backporting this change (if applicable). Thanks!

@ivanpauno ivanpauno removed this from Needs Backport in Dashing Patch Release 8 Nov 11, 2020
@ivanpauno ivanpauno removed this from Needs backport in Foxy Patch Release 4 Nov 11, 2020
@ivanpauno
Copy link
Member Author

@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.
It was only an improvement to use the system yaml-cpp if available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants