Merge topic 'option-normal-variable'

2a5f5c0e31 option: respect existing normal variable
12e6f83319 Option: Add a test that verifies interaction with normal variables
5bb3d40a28 cmOption: Remove VTK 4.0 workarounds

Acked-by: Kitware Robot <kwrobot@kitware.com>
Merge-request: !2155

Help/command/option.rst

@@ -9,7 +9,9 @@ Provides an option that the user can optionally select.
          [initial value])
 
 Provide an option for the user to select as ``ON`` or ``OFF``.  If no
-initial value is provided, ``OFF`` is used.
+initial value is provided, ``OFF`` is used.  If the option is already
+set as a normal variable then the command does nothing
+(see policy :policy:`CMP0077`).
 
 If you have options that depend on the values of other options, see
 the module help for :module:`CMakeDependentOption`.

Help/manual/cmake-policies.7.rst

@@ -57,6 +57,7 @@ Policies Introduced by CMake 3.13
 .. toctree::
    :maxdepth: 1
 
+   CMP0077: option() honors normal variables. </policy/CMP0077>
    CMP0076: target_sources() command converts relative paths to absolute. </policy/CMP0076>
 
 Policies Introduced by CMake 3.12

Help/policy/CMP0077.rst

@@ -0,0 +1,16 @@
+CMP0077
+-------
+
+:command:`option` honors normal variables.
+
+The ``OLD`` behavior for this policy is to clear any existing normal variables
+with the same name.  The ``NEW`` behavior for this policy is to not create
+a cache entry or modify any existing normal variables if a normal variable
+with the same name already exists.
+
+This policy was introduced in CMake version 3.13.  CMake version
+|release| warns when the policy is not set and uses ``OLD`` behavior.
+Use the :command:`cmake_policy` command to set it to ``OLD`` or ``NEW``
+explicitly.
+
+.. include:: DEPRECATED.txt

Help/release/dev/option-normal-variable.rst

@@ -0,0 +1,5 @@
+option-normal-variable
+----------------------
+
+* The :command:`option` command now honors existing normal variables instead
+  of replacing them with a cache entry. See policy :policy:`CMP0077`.

Source/cmOptionCommand.cxx

@@ -2,11 +2,16 @@
    file Copyright.txt or https://cmake.org/licensing for details.  */
 #include "cmOptionCommand.h"
 
+#include <sstream>
+
 #include "cmAlgorithms.h"
 #include "cmMakefile.h"
+#include "cmPolicies.h"
 #include "cmState.h"
+#include "cmStateSnapshot.h"
 #include "cmStateTypes.h"
 #include "cmSystemTools.h"
+#include "cmake.h"
 
 class cmExecutionStatus;
 
@@ -14,19 +19,7 @@ class cmExecutionStatus;
 bool cmOptionCommand::InitialPass(std::vector<std::string> const& args,
                                   cmExecutionStatus&)
 {
-  bool argError = false;
-  if (args.size() < 2) {
-    argError = true;
-  }
-  // for VTK 4.0 we have to support the option command with more than 3
-  // arguments if CMAKE_MINIMUM_REQUIRED_VERSION is not defined, if
-  // CMAKE_MINIMUM_REQUIRED_VERSION is defined, then we can have stricter
-  // checking.
-  if (this->Makefile->GetDefinition("CMAKE_MINIMUM_REQUIRED_VERSION")) {
-    if (args.size() > 3) {
-      argError = true;
-    }
-  }
+  const bool argError = (args.size() < 2) || (args.size() > 3);
   if (argError) {
     std::string m = "called with incorrect number of arguments: ";
     m += cmJoin(args, " ");
@@ -34,18 +27,47 @@ bool cmOptionCommand::InitialPass(std::vector<std::string> const& args,
     return false;
   }
 
-  std::string initialValue = "Off";
-  // Now check and see if the value has been stored in the cache
-  // already, if so use that value and don't look for the program
+  // Determine the state of the option policy
+  auto status = this->Makefile->GetPolicyStatus(cmPolicies::CMP0077);
+  const char* exists =
+    this->Makefile->GetStateSnapshot().GetDefinition(args[0]);
+  switch (status) {
+    case cmPolicies::WARN:
+      if (exists) {
+        std::ostringstream w;
+        w << cmPolicies::GetPolicyWarning(cmPolicies::CMP0077)
+          << "\n"
+             "For compatibility with older versions of CMake, option "
+             "is clearing the normal variable '"
+          << args[0] << "'.";
+        this->Makefile->IssueMessage(cmake::AUTHOR_WARNING, w.str());
+      }
+    case cmPolicies::OLD:
+      // OLD behavior does not warn.
+      break;
+    case cmPolicies::REQUIRED_ALWAYS:
+    case cmPolicies::REQUIRED_IF_USED:
+    case cmPolicies::NEW: {
+      // See if a local variable with this name already exists.
+      // If so we ignore the option command.
+      if (exists) {
+        return true;
+      }
+    } break;
+  }
+
+  // See if a cache variable with this name already exists
+  // If so just make sure the doc state is correct
   cmState* state = this->Makefile->GetState();
   const char* existingValue = state->GetCacheEntryValue(args[0]);
-  if (existingValue) {
-    if (state->GetCacheEntryType(args[0]) != cmStateEnums::UNINITIALIZED) {
-      state->SetCacheEntryProperty(args[0], "HELPSTRING", args[1]);
-      return true;
-    }
-    initialValue = existingValue;
+  if (existingValue &&
+      (state->GetCacheEntryType(args[0]) != cmStateEnums::UNINITIALIZED)) {
+    state->SetCacheEntryProperty(args[0], "HELPSTRING", args[1]);
+    return true;
   }
+
+  // Nothing in the cache so add it
+  std::string initialValue = existingValue ? existingValue : "Off";
   if (args.size() == 3) {
     initialValue = args[2];
   }

Source/cmPolicies.h

@@ -226,7 +226,9 @@ class cmMakefile;
          0, cmPolicies::WARN)                                                 \
   SELECT(POLICY, CMP0076,                                                     \
          "target_sources() command converts relative paths to absolute.", 3,  \
-         13, 0, cmPolicies::WARN)
+         13, 0, cmPolicies::WARN)                                             \
+  SELECT(POLICY, CMP0077, "option() honors normal variables.", 3, 13, 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/CMakeLists.txt

@@ -240,6 +240,7 @@ add_RunCMake_test(include_guard)
 add_RunCMake_test(list)
 add_RunCMake_test(math)
 add_RunCMake_test(message)
+add_RunCMake_test(option)
 add_RunCMake_test(project -DCMake_TEST_RESOURCES=${CMake_TEST_RESOURCES})
 add_RunCMake_test(return)
 add_RunCMake_test(separate_arguments)

Tests/RunCMake/option/CMP0077-NEW.cmake

@@ -0,0 +1,14 @@
+
+#Verify that option DOESN'T overwrite existing normal variable when the policy
+#is set to NEW
+cmake_policy(SET CMP0077 NEW)
+set(OPT_LOCAL_VAR FALSE)
+option(OPT_LOCAL_VAR "TEST_VAR" ON)
+if(OPT_LOCAL_VAR)
+  message(FATAL_ERROR "option failed to overwrite existing normal variable")
+endif()
+
+get_property(_exists_in_cache CACHE OPT_LOCAL_VAR PROPERTY VALUE SET)
+if(_exists_in_cache)
+  message(FATAL_ERROR "value should not exist in cache as it was already a local variable")
+endif()

Tests/RunCMake/option/CMP0077-OLD.cmake

@@ -0,0 +1,9 @@
+
+#Verify that option overwrites existing normal variable when the policy
+#is set to OLD
+cmake_policy(SET CMP0077 OLD)
+set(OPT_LOCAL_VAR FALSE)
+option(OPT_LOCAL_VAR "TEST_VAR" ON)
+if(NOT OPT_LOCAL_VAR)
+  message(FATAL_ERROR "option failed to overwrite existing normal variable")
+endif()

Tests/RunCMake/option/CMP0077-WARN-stderr.txt

@@ -0,0 +1,7 @@
+CMake Warning \(dev\) at CMP0077-WARN.cmake:5 \(option\):
+  Policy CMP0077 is not set: option\(\) honors normal variables.  Run "cmake
+  --help-policy CMP0077" for policy details.  Use the cmake_policy command to
+  set the policy and suppress this warning.
+
+  For compatibility with older versions of CMake, option is clearing the
+  normal variable 'OPT_LOCAL_VAR'.

Tests/RunCMake/option/CMP0077-WARN.cmake

@@ -0,0 +1,5 @@
+
+#Verify that option overwrites existing normal variable when the policy
+#is set to OLD
+set(OPT_LOCAL_VAR FALSE)
+option(OPT_LOCAL_VAR "TEST_VAR" ON)

Tests/RunCMake/option/CMakeLists.txt

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

Tests/RunCMake/option/RunCMakeTest.cmake

@@ -0,0 +1,5 @@
+include(RunCMake)
+
+run_cmake(CMP0077-OLD)
+run_cmake(CMP0077-NEW)
+run_cmake(CMP0077-WARN)