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

Cmake yaml-cpp package name fix #6325

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

vaishakp
Copy link
Contributor

@vaishakp vaishakp commented Oct 5, 2024

Proposed changes

This PR changes the cmake name of the package in the build system from the default yaml-cpp to YAML_CPP, and sets it as an alias for the original yaml-cpp.

This ensures that an external installation of the library outside of the standard location can be found by cmake, through the default cmake env variable YAML_CPP_ROOT.

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

Currently, the package yaml-cpp is declared as itself in cmake setup files, with the hyphen. While this doesn't cause any issues when the library is installed in standard locations, the hyphen in the cmake package name causes a failure for yaml-cpp to be found when installed in non-standard locations, translating to a failure at the configure stage:

CMake Warning at cmake/SetupYamlCpp.cmake:4 (find_package):
  By not providing "Findyaml-cpp.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by yaml-cpp,
  but CMake did not find one.

  Could not find a package configuration file provided by yaml-cpp with any
  of the following names:

    yaml-cppConfig.cmake
    yaml-cpp-config.cmake

  Add the installation prefix of "yaml-cpp" to CMAKE_PREFIX_PATH or set
  "yaml-cpp_DIR" to a directory containing one of the above files.  If
  "yaml-cpp" provides a separate development package or SDK, be sure it has
  been installed.
Call Stack (most recent call first):
  CMakeLists.txt:162 (include)


CMake Error at cmake/SetupYamlCpp.cmake:8 (message):
  Could not find yaml-cpp.  If you want to fetch missing dependencies
  automatically, set SPECTRE_FETCH_MISSING_DEPS=ON.
Call Stack (most recent call first):
  CMakeLists.txt:162 (include)

-- Configuring incomplete, errors occurred!

This is due to the fact that cmake looks in the path <PackageName>_ROOT, which for this package is yaml-cpp_ROOT. As per IEEE Standards 1003.1-2001, this is not a valid environment variable, at least in UNIX and thus cannot be set.

@prayush prayush added the bugfix label Oct 5, 2024
Hiphens in cmake package name cause `yaml-cpp` to not
be found when in non-standard locations. This is due to
the fact that cmake looks in the path `<PackageName>_ROOT`,
which is not allowed in UNIX.
@vaishakp vaishakp marked this pull request as ready for review October 5, 2024 07:35
@knelli2 knelli2 requested a review from nilsvu October 5, 2024 23:47
Copy link
Member

@nilsvu nilsvu 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 fixing this. It took me a bit of time to verify that this is the right thing to do, because changing names like this has the potential to break installations on other people's machines and clusters. But I'm now conviced that this is a good change 👍

@nilsvu nilsvu merged commit d1ce0f1 into sxs-collaboration:develop Oct 11, 2024
22 of 23 checks passed
@nilsvu nilsvu added the build system CMake build system label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix build system CMake build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants