Merge topic 'set-cache-keep-normal-variable'

d96eb55282 set(CACHE): do not remove normal variable

Acked-by: Kitware Robot <[email protected]>
Reviewed-by: Ben Boeckel <[email protected]>
Merge-request: !6146

Help/command/set.rst

@@ -68,9 +68,13 @@ users.
 
 If the cache entry does not exist prior to the call or the ``FORCE``
 option is given then the cache entry will be set to the given value.
-Furthermore, any normal variable binding in the current scope will
-be removed to expose the newly cached value to any immediately
-following evaluation.
+
+.. note::
+
+  The content of the cache variable will not be directly accessible if a normal
+  variable of the same name already exists (see :ref:`rules of variable
+  evaluation <CMake Language Variables>`). If policy :policy:`CMP0126` is set
+  to ``OLD``, any normal variable binding in the current scope will be removed.
 
 It is possible for the cache entry to exist prior to the call but
 have no type set if it was created on the :manual:`cmake(1)` command

Help/manual/cmake-policies.7.rst

@@ -57,6 +57,7 @@ Policies Introduced by CMake 3.21
 .. toctree::
    :maxdepth: 1
 
+   CMP0126: set(CACHE) does not remove a normal variable of the same name. </policy/CMP0126>
    CMP0125: find_(path|file|library|program) have consistent behavior for cache variables. </policy/CMP0125>
    CMP0124: foreach() loop variables are only available in the loop scope. </policy/CMP0124>
    CMP0123: ARMClang cpu/arch compile and link flags must be set explicitly. </policy/CMP0123>

Help/policy/CMP0126.rst

@@ -0,0 +1,20 @@
+CMP0126
+-------
+
+.. versionadded:: 3.21
+
+The :command:`set(CACHE)` does not remove a normal variable of the same name.
+
+Starting with CMake 3.21, the :command:`set(CACHE)` does not remove, in the
+current scope, any normal variable with the same name.
+
+The ``OLD`` behavior for this policy is to have the :command:`set(CACHE)`
+command removing the normal variable of the same name, if any. The ``NEW``
+behavior for this policy is to keep the normal variable of the same name.
+
+This policy was introduced in CMake version 3.21. Use the
+:command:`cmake_policy` command to set it to ``OLD`` or ``NEW`` explicitly.
+Unlike many policies, CMake version |release| does *not* warn when the policy
+is not set and simply uses ``OLD`` behavior.
+
+.. include:: DEPRECATED.txt

Help/release/dev/set-cache-variable.rst

@@ -0,0 +1,5 @@
+set-cache-variable
+------------------
+
+* The :command:`set(CACHE)` command no longer removes a normal variable of the
+  same name, if any. See policy :policy:`CMP0126`.

Source/cmFindBase.cxx

@@ -357,11 +357,17 @@ void cmFindBase::NormalizeFindResult()
       this->Makefile->GetCMakeInstance()->AddCacheEntry(
         this->VariableName, value.c_str(), this->VariableDocumentation.c_str(),
         this->VariableType);
-      // if there was a definition then remove it
-      // This is required to ensure same behavior as
-      // cmMakefile::AddCacheDefinition.
-      // See #22038 for problems raised by this behavior.
-      this->Makefile->RemoveDefinition(this->VariableName);
+      if (this->Makefile->GetPolicyStatus(cmPolicies::CMP0126) ==
+          cmPolicies::NEW) {
+        if (this->Makefile->IsNormalDefinitionSet(this->VariableName)) {
+          this->Makefile->AddDefinition(this->VariableName, value);
+        }
+      } else {
+        // if there was a definition then remove it
+        // This is required to ensure same behavior as
+        // cmMakefile::AddCacheDefinition.
+        this->Makefile->RemoveDefinition(this->VariableName);
+      }
     }
   } else {
     // If the user specifies the entry on the command line without a
@@ -371,6 +377,14 @@ void cmFindBase::NormalizeFindResult()
       this->Makefile->AddCacheDefinition(this->VariableName, "",
                                          this->VariableDocumentation.c_str(),
                                          this->VariableType);
+      if (this->Makefile->GetPolicyStatus(cmPolicies::CMP0126) ==
+            cmPolicies::NEW &&
+          this->Makefile->IsNormalDefinitionSet(this->VariableName)) {
+        this->Makefile->AddDefinition(
+          this->VariableName,
+          *this->Makefile->GetCMakeInstance()->GetCacheDefinition(
+            this->VariableName));
+      }
     }
   }
 }
@@ -379,17 +393,28 @@ void cmFindBase::StoreFindResult(const std::string& value)
 {
   bool force =
     this->Makefile->GetPolicyStatus(cmPolicies::CMP0125) == cmPolicies::NEW;
+  bool updateNormalVariable =
+    this->Makefile->GetPolicyStatus(cmPolicies::CMP0126) == cmPolicies::NEW;
 
   if (!value.empty()) {
     this->Makefile->AddCacheDefinition(this->VariableName, value,
                                        this->VariableDocumentation.c_str(),
                                        this->VariableType, force);
+    if (updateNormalVariable &&
+        this->Makefile->IsNormalDefinitionSet(this->VariableName)) {
+      this->Makefile->AddDefinition(this->VariableName, value);
+    }
     return;
   }
 
   this->Makefile->AddCacheDefinition(
     this->VariableName, cmStrCat(this->VariableName, "-NOTFOUND"),
     this->VariableDocumentation.c_str(), this->VariableType, force);
+  if (updateNormalVariable &&
+      this->Makefile->IsNormalDefinitionSet(this->VariableName)) {
+    this->Makefile->AddDefinition(this->VariableName,
+                                  cmStrCat(this->VariableName, "-NOTFOUND"));
+  }
 
   if (this->Required) {
     this->Makefile->IssueMessage(

Source/cmFindPackageCommand.cxx

@@ -1138,6 +1138,11 @@ bool cmFindPackageCommand::FindConfig()
   // We force the value since we do not get here if it was already set.
   this->Makefile->AddCacheDefinition(this->Variable, init, help.c_str(),
                                      cmStateEnums::PATH, true);
+  if (this->Makefile->GetPolicyStatus(cmPolicies::CMP0126) ==
+        cmPolicies::NEW &&
+      this->Makefile->IsNormalDefinitionSet(this->Variable)) {
+    this->Makefile->AddDefinition(this->Variable, init);
+  }
 
   return found;
 }

Source/cmMakefile.cxx

@@ -1962,10 +1962,10 @@ void cmMakefile::AddCacheDefinition(const std::string& name, const char* value,
     }
   }
   this->GetCMakeInstance()->AddCacheEntry(name, value, doc, type);
-  // if there was a definition then remove it
-  // The method cmFindBase::NormalizeFindResult also apply same workflow.
-  // See #22038 for problems raised by this behavior.
-  this->StateSnapshot.RemoveDefinition(name);
+  if (this->GetPolicyStatus(cmPolicies::CMP0126) != cmPolicies::NEW) {
+    // if there was a definition then remove it
+    this->StateSnapshot.RemoveDefinition(name);
+  }
 }
 
 void cmMakefile::MarkVariableAsUsed(const std::string& var)

Source/cmOptionCommand.cxx

@@ -68,6 +68,13 @@ bool cmOptionCommand(std::vector<std::string> const& args,
   bool init = cmIsOn(initialValue);
   status.GetMakefile().AddCacheDefinition(args[0], init ? "ON" : "OFF",
                                           args[1].c_str(), cmStateEnums::BOOL);
+  if (status.GetMakefile().GetPolicyStatus(cmPolicies::CMP0077) !=
+        cmPolicies::NEW &&
+      status.GetMakefile().GetPolicyStatus(cmPolicies::CMP0126) ==
+        cmPolicies::NEW) {
+    // if there was a definition then remove it
+    status.GetMakefile().GetStateSnapshot().RemoveDefinition(args[0]);
+  }
 
   if (checkAndWarn) {
     const auto* existsAfterSet =

Source/cmPolicies.h

@@ -376,7 +376,10 @@ class cmMakefile;
   SELECT(POLICY, CMP0125,                                                     \
          "find_(path|file|library|program) have consistent behavior for "     \
          "cache variables.",                                                  \
-         3, 21, 0, cmPolicies::WARN)
+         3, 21, 0, cmPolicies::WARN)                                          \
+  SELECT(POLICY, CMP0126,                                                     \
+         "set(CACHE) does not remove a normal variable of the same name.", 3, \
+         21, 0, cmPolicies::WARN)
 
 #define CM_SELECT_ID(F, A1, A2, A3, A4, A5, A6) F(A1)
 #define CM_FOR_EACH_POLICY_ID(POLICY)                                         \

Tests/RunCMake/CMP0126/CMP0126-NEW.cmake

@@ -0,0 +1,28 @@
+
+# enforce policy CMP0125 to ensure predictable result of find_* commands
+cmake_policy(SET CMP0125 NEW)
+
+cmake_policy(SET CMP0126 NEW)
+
+set(VAR 1)
+set(VAR 2 CACHE STRING "")
+
+if (NOT VAR EQUAL 1)
+  message(FATAL_ERROR "normal variable does not exist anymore.")
+endif()
+
+
+file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/file.txt" "")
+set(VAR file.txt)
+set(VAR "" CACHE STRING "" FORCE)
+set_property(CACHE VAR PROPERTY TYPE UNINITIALIZED)
+
+find_file(VAR NAMES file.txt PATHS "${CMAKE_CURRENT_BINARY_DIR}")
+
+unset(VAR CACHE)
+if (NOT DEFINED VAR)
+  message(FATAL_ERROR "find_file: normal variable does not exist anymore.")
+endif()
+if (NOT VAR STREQUAL "${CMAKE_CURRENT_BINARY_DIR}/file.txt")
+  message(FATAL_ERROR "find_file: failed to set normal variable.")
+endif()

Tests/RunCMake/CMP0126/CMP0126-NEW_CL.cmake

@@ -0,0 +1,9 @@
+
+cmake_policy(SET CMP0126 NEW)
+
+set(VAR 1)
+set(VAR 2 CACHE STRING "")
+
+if (NOT VAR EQUAL 1)
+  message(FATAL_ERROR "normal variable does not exist anymore.")
+endif()

Tests/RunCMake/CMP0126/CMP0126-OLD.cmake

@@ -0,0 +1,25 @@
+
+# enforce policy CMP0125 to ensure predictable result of find_* commands
+cmake_policy(SET CMP0125 NEW)
+
+cmake_policy(SET CMP0126 OLD)
+
+set(VAR 1)
+set(VAR 2 CACHE STRING "")
+
+if (VAR EQUAL 1)
+  message(FATAL_ERROR "normal variable still exist.")
+endif()
+
+
+file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/file.txt" "")
+set(VAR file.txt)
+set(VAR "" CACHE STRING "" FORCE)
+set_property(CACHE VAR PROPERTY TYPE UNINITIALIZED)
+
+find_file(VAR NAMES file.txt PATHS "${CMAKE_CURRENT_BINARY_DIR}")
+
+unset(VAR CACHE)
+if (DEFINED VAR)
+    message(FATAL_ERROR "find_file: normal variable still exist.")
+endif()

Tests/RunCMake/CMP0126/CMP0126-OLD_CL.cmake

@@ -0,0 +1,9 @@
+
+cmake_policy(SET CMP0126 OLD)
+
+set(VAR 1)
+set(VAR 2 CACHE STRING "")
+
+if (NOT VAR EQUAL 3)
+  message(FATAL_ERROR "normal variable still exist.")
+endif()

Tests/RunCMake/CMP0126/CMakeLists.txt

@@ -0,0 +1,3 @@
+cmake_minimum_required(VERSION 3.20)
+project(${RunCMake_TEST} NONE)
+include(${RunCMake_TEST}.cmake)

Tests/RunCMake/CMP0126/RunCMakeTest.cmake

@@ -0,0 +1,6 @@
+include(RunCMake)
+
+run_cmake(CMP0126-OLD)
+run_cmake_with_options(CMP0126-OLD_CL -DVAR=3)
+run_cmake(CMP0126-NEW)
+run_cmake_with_options(CMP0126-NEW_CL -DVAR=3)

Tests/RunCMake/CMakeLists.txt

@@ -140,6 +140,7 @@ if (CMAKE_SYSTEM_NAME MATCHES "(Linux|Darwin)")
   add_RunCMake_test(CMP0125 -DCMAKE_SHARED_LIBRARY_PREFIX=${CMAKE_SHARED_LIBRARY_PREFIX}
                             -DCMAKE_SHARED_LIBRARY_SUFFIX=${CMAKE_SHARED_LIBRARY_SUFFIX})
 endif()
+add_RunCMake_test(CMP0126)
 
 # The test for Policy 65 requires the use of the
 # CMAKE_SHARED_LIBRARY_LINK_CXX_FLAGS variable, which both the VS and Xcode