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

Enable CMake policy CMP0077 #2222

Merged
merged 1 commit into from
Jun 27, 2020
Merged

Enable CMake policy CMP0077 #2222

merged 1 commit into from
Jun 27, 2020

Conversation

alexreinking
Copy link
Contributor

@alexreinking alexreinking commented Jun 26, 2020

Projects that import json via FetchContent or add_subdirectory pointed at a git submodule may want to set JSON_BuildTests to "NO". However, this doesn't work without creating an identical option() in the importing project. Enabling CMP0077 in CMake 3.13+ changes the behavior of option() to allow importing projects to set default values for the variables without touching the cache.

See the documentation for CMP0077 here: https://cmake.org/cmake/help/latest/policy/CMP0077.html


Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

Projects that import json via [FetchContent](https://cmake.org/cmake/help/latest/module/FetchContent.html) or `add_subdirectory` pointed at a git submodule may want to set `JSON_BuildTests` to "NO". However, this doesn't work without creating an identical `option()` in the importing project. Enabling CMP0077 in supported versions of CMake changes the behavior of `option()` to allow importing projects to set default values for the variables without touching the cache.

See the documentation for CMP0077 here: https://cmake.org/cmake/help/latest/policy/CMP0077.html
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ec43371 on alexreinking:patch-1 into 635b9a0 on nlohmann:develop.

@t-b
Copy link
Contributor

t-b commented Jun 26, 2020

@alexreinking Can you clarify why that is needed?

I'm doing here

SET(JSON_BuildTests OFF CACHE INTERNAL "")
SET(JSON_Install OFF CACHE INTERNAL "")
ADD_SUBDIRECTORY(json)

and the JSON tests are not built and I also don't define these options. My cmake version is 3.15.2 on windows.

@alexreinking
Copy link
Contributor Author

The key is not touching the cache. The snippet you posted will no longer set JSON_BuildTests to OFF if I explicitly set it to "ON" in the cache. Additionally, option() will not create a cache entry at all with CMP0077 enabled and the normal variable set.

In CMake 3.13 and above the option() command prefers to do nothing when a normal variable of the given name already exists. It does not create or update a cache entry or remove the normal variable. The new behavior is consistent between the first and later runs in a build tree. This policy provides compatibility with projects that have not been updated to expect the new behavior.

Without the policy, the following does not work, but should, and is the default behavior of CMake going forward:

set(JSON_BuildTests OFF)
set(JSON_Install OFF)
add_subdirectory(json)

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann self-assigned this Jun 27, 2020
@nlohmann nlohmann added this to the Release 3.8.1 milestone Jun 27, 2020
@nlohmann nlohmann merged commit 1af4f5f into nlohmann:develop Jun 27, 2020
@nlohmann
Copy link
Owner

Thanks!

@nlohmann
Copy link
Owner


🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

  • ✨ New Feature
  • 🐛 Bug Fix
  • ⚡️ Improvement
  • 🔨 Further Change
  • 🔥 Deprecated function

Description


@t-b
Copy link
Contributor

t-b commented Jun 27, 2020

@alexreinking Thanks for the clarification. Makes sense!

@alexreinking alexreinking deleted the patch-1 branch August 19, 2022 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants