Merge topic 'refactor-environment-modification'

a5d45e685f Tests: Add case for ENVIRONMENT_MODIFICATION property OP=reset behavior
e2854b4fa2 cmCTestRunTest: Implement the ENVIRONMENT test property with EnvDiff too
bfa1c5285b cmSystemTools: Add EnvDiff class to hold ENVIRONMENT_MODIFICATION logic
a0b1c4ee90 cmCTestRunTest: Simplify by using GetSystemPathlistSeparator
4e6cbb1f13 cmCTestRunTest: Remove unnecessary CMAKE_BOOTSTRAP guard

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

10 files changed, 208 insertions, 134 deletions

diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx
index 2a2cb1c..5efe69f 100644
--- a/Source/CTest/cmCTestRunTest.cxx
+++ b/Source/CTest/cmCTestRunTest.cxx
@@ -8,16 +8,12 @@
 #include <cstdint>
 #include <cstdio>
 #include <cstring>
-#include <functional>
 #include <iomanip>
 #include <ratio>
 #include <sstream>
 #include <utility>
 
 #include <cm/memory>
-#include <cm/optional>
-#include <cm/string_view>
-#include <cmext/string_view>
 
 #include "cmsys/RegularExpression.hxx"
 
@@ -787,149 +783,31 @@ bool cmCTestRunTest::ForkProcess(
 
   this->TestProcess->SetTimeout(timeout);
 
-#ifndef CMAKE_BOOTSTRAP
   cmSystemTools::SaveRestoreEnvironment sre;
-#endif
-
   std::ostringstream envMeasurement;
+
+  // We split processing ENVIRONMENT and ENVIRONMENT_MODIFICATION into two
+  // phases to ensure that MYVAR=reset: in the latter phase resets to the
+  // former phase's settings, rather than to the original environment.
   if (environment && !environment->empty()) {
-    // Environment modification works on the assumption that the environment is
-    // actually modified here. If another strategy is used, there will need to
-    // be updates below in `apply_diff`.
-    cmSystemTools::AppendEnv(*environment);
-    for (auto const& var : *environment) {
-      envMeasurement << var << std::endl;
-    }
+    cmSystemTools::EnvDiff diff;
+    diff.AppendEnv(*environment);
+    diff.ApplyToCurrentEnv(&envMeasurement);
   }
 
   if (environment_modification && !environment_modification->empty()) {
-    std::map<std::string, cm::optional<std::string>> env_application;
-
-#ifdef _WIN32
-    char path_sep = ';';
-#else
-    char path_sep = ':';
-#endif
-
-    auto apply_diff =
-      [&env_application](const std::string& name,
-                         std::function<void(std::string&)> const& apply) {
-        cm::optional<std::string> old_value = env_application[name];
-        std::string output;
-        if (old_value) {
-          output = *old_value;
-        } else {
-          // This only works because the environment is actually modified above
-          // (`AppendEnv`). If CTest ever just creates an environment block
-          // directly, that block will need to be queried for the subprocess'
-          // value instead.
-          const char* curval = cmSystemTools::GetEnv(name);
-          if (curval) {
-            output = curval;
-          }
-        }
-        apply(output);
-        env_application[name] = output;
-      };
-
-    bool err_occurred = false;
+    cmSystemTools::EnvDiff diff;
+    bool env_ok = true;
 
     for (auto const& envmod : *environment_modification) {
-      // Split on `=`
-      auto const eq_loc = envmod.find_first_of('=');
-      if (eq_loc == std::string::npos) {
-        cmCTestLog(this->CTest, ERROR_MESSAGE,
-                   "Error: Missing `=` after the variable name in: "
-                     << envmod << std::endl);
-        err_occurred = true;
-        continue;
-      }
-      auto const name = envmod.substr(0, eq_loc);
-
-      // Split value on `:`
-      auto const op_value_start = eq_loc + 1;
-      auto const colon_loc = envmod.find_first_of(':', op_value_start);
-      if (colon_loc == std::string::npos) {
-        cmCTestLog(this->CTest, ERROR_MESSAGE,
-                   "Error: Missing `:` after the operation in: " << envmod
-                                                                 << std::endl);
-        err_occurred = true;
-        continue;
-      }
-      auto const op =
-        envmod.substr(op_value_start, colon_loc - op_value_start);
-
-      auto const value_start = colon_loc + 1;
-      auto const value = envmod.substr(value_start);
-
-      // Determine what to do with the operation.
-      if (op == "reset"_s) {
-        auto entry = env_application.find(name);
-        if (entry != env_application.end()) {
-          env_application.erase(entry);
-        }
-      } else if (op == "set"_s) {
-        env_application[name] = value;
-      } else if (op == "unset"_s) {
-        env_application[name] = {};
-      } else if (op == "string_append"_s) {
-        apply_diff(name, [&value](std::string& output) { output += value; });
-      } else if (op == "string_prepend"_s) {
-        apply_diff(name,
-                   [&value](std::string& output) { output.insert(0, value); });
-      } else if (op == "path_list_append"_s) {
-        apply_diff(name, [&value, path_sep](std::string& output) {
-          if (!output.empty()) {
-            output += path_sep;
-          }
-          output += value;
-        });
-      } else if (op == "path_list_prepend"_s) {
-        apply_diff(name, [&value, path_sep](std::string& output) {
-          if (!output.empty()) {
-            output.insert(output.begin(), path_sep);
-          }
-          output.insert(0, value);
-        });
-      } else if (op == "cmake_list_append"_s) {
-        apply_diff(name, [&value](std::string& output) {
-          if (!output.empty()) {
-            output += ';';
-          }
-          output += value;
-        });
-      } else if (op == "cmake_list_prepend"_s) {
-        apply_diff(name, [&value](std::string& output) {
-          if (!output.empty()) {
-            output.insert(output.begin(), ';');
-          }
-          output.insert(0, value);
-        });
-      } else {
-        cmCTestLog(this->CTest, ERROR_MESSAGE,
-                   "Error: Unrecognized environment manipulation argument: "
-                     << op << std::endl);
-        err_occurred = true;
-        continue;
-      }
+      env_ok &= diff.ParseOperation(envmod);
     }
 
-    if (err_occurred) {
+    if (!env_ok) {
       return false;
     }
 
-    for (auto const& env_apply : env_application) {
-      if (env_apply.second) {
-        auto const env_update =
-          cmStrCat(env_apply.first, '=', *env_apply.second);
-        cmSystemTools::PutEnv(env_update);
-        envMeasurement << env_update << std::endl;
-      } else {
-        cmSystemTools::UnsetEnv(env_apply.first.c_str());
-        // Signify that this variable is being actively unset
-        envMeasurement << "#" << env_apply.first << "=" << std::endl;
-      }
-    }
+    diff.ApplyToCurrentEnv(&envMeasurement);
   }
 
   if (this->UseAllocatedResources) {
diff --git a/Source/cmSystemTools.cxx b/Source/cmSystemTools.cxx
index 672cdc7..2c32f09 100644
--- a/Source/cmSystemTools.cxx
+++ b/Source/cmSystemTools.cxx
@@ -16,6 +16,7 @@
 
 #include <cm/optional>
 #include <cmext/algorithm>
+#include <cmext/string_view>
 
 #include <cm3p/uv.h>
 
@@ -1563,6 +1564,139 @@ void cmSystemTools::AppendEnv(std::vector<std::string> const& env)
   }
 }
 
+void cmSystemTools::EnvDiff::AppendEnv(std::vector<std::string> const& env)
+{
+  for (std::string const& eit : env) {
+    this->PutEnv(eit);
+  }
+}
+
+void cmSystemTools::EnvDiff::PutEnv(const std::string& env)
+{
+  auto const eq_loc = env.find('=');
+  if (eq_loc != std::string::npos) {
+    std::string name = env.substr(0, eq_loc);
+    diff[name] = env.substr(eq_loc + 1);
+  } else {
+    diff[env] = {};
+  }
+}
+
+bool cmSystemTools::EnvDiff::ParseOperation(const std::string& envmod)
+{
+  char path_sep = GetSystemPathlistSeparator();
+
+  auto apply_diff = [this](const std::string& name,
+                           std::function<void(std::string&)> const& apply) {
+    cm::optional<std::string> old_value = diff[name];
+    std::string output;
+    if (old_value) {
+      output = *old_value;
+    } else {
+      const char* curval = cmSystemTools::GetEnv(name);
+      if (curval) {
+        output = curval;
+      }
+    }
+    apply(output);
+    diff[name] = output;
+  };
+
+  // Split on `=`
+  auto const eq_loc = envmod.find_first_of('=');
+  if (eq_loc == std::string::npos) {
+    cmSystemTools::Error(cmStrCat(
+      "Error: Missing `=` after the variable name in: ", envmod, '\n'));
+    return false;
+  }
+
+  auto const name = envmod.substr(0, eq_loc);
+
+  // Split value on `:`
+  auto const op_value_start = eq_loc + 1;
+  auto const colon_loc = envmod.find_first_of(':', op_value_start);
+  if (colon_loc == std::string::npos) {
+    cmSystemTools::Error(
+      cmStrCat("Error: Missing `:` after the operation in: ", envmod, '\n'));
+    return false;
+  }
+  auto const op = envmod.substr(op_value_start, colon_loc - op_value_start);
+
+  auto const value_start = colon_loc + 1;
+  auto const value = envmod.substr(value_start);
+
+  // Determine what to do with the operation.
+  if (op == "reset"_s) {
+    auto entry = diff.find(name);
+    if (entry != diff.end()) {
+      diff.erase(entry);
+    }
+  } else if (op == "set"_s) {
+    diff[name] = value;
+  } else if (op == "unset"_s) {
+    diff[name] = {};
+  } else if (op == "string_append"_s) {
+    apply_diff(name, [&value](std::string& output) { output += value; });
+  } else if (op == "string_prepend"_s) {
+    apply_diff(name,
+               [&value](std::string& output) { output.insert(0, value); });
+  } else if (op == "path_list_append"_s) {
+    apply_diff(name, [&value, path_sep](std::string& output) {
+      if (!output.empty()) {
+        output += path_sep;
+      }
+      output += value;
+    });
+  } else if (op == "path_list_prepend"_s) {
+    apply_diff(name, [&value, path_sep](std::string& output) {
+      if (!output.empty()) {
+        output.insert(output.begin(), path_sep);
+      }
+      output.insert(0, value);
+    });
+  } else if (op == "cmake_list_append"_s) {
+    apply_diff(name, [&value](std::string& output) {
+      if (!output.empty()) {
+        output += ';';
+      }
+      output += value;
+    });
+  } else if (op == "cmake_list_prepend"_s) {
+    apply_diff(name, [&value](std::string& output) {
+      if (!output.empty()) {
+        output.insert(output.begin(), ';');
+      }
+      output.insert(0, value);
+    });
+  } else {
+    cmSystemTools::Error(cmStrCat(
+      "Error: Unrecognized environment manipulation argument: ", op, '\n'));
+    return false;
+  }
+
+  return true;
+}
+
+void cmSystemTools::EnvDiff::ApplyToCurrentEnv(std::ostringstream* measurement)
+{
+  for (auto const& env_apply : diff) {
+    if (env_apply.second) {
+      auto const env_update =
+        cmStrCat(env_apply.first, '=', *env_apply.second);
+      cmSystemTools::PutEnv(env_update);
+      if (measurement) {
+        *measurement << env_update << std::endl;
+      }
+    } else {
+      cmSystemTools::UnsetEnv(env_apply.first.c_str());
+      if (measurement) {
+        // Signify that this variable is being actively unset
+        *measurement << '#' << env_apply.first << "=\n";
+      }
+    }
+  }
+}
+
 cmSystemTools::SaveRestoreEnvironment::SaveRestoreEnvironment()
 {
   this->Env = cmSystemTools::GetEnvironmentVariables();
diff --git a/Source/cmSystemTools.h b/Source/cmSystemTools.h
index ec650f7..e8c04b1 100644
--- a/Source/cmSystemTools.h
+++ b/Source/cmSystemTools.h
@@ -6,9 +6,12 @@
 
 #include <cstddef>
 #include <functional>
+#include <map>
+#include <sstream>
 #include <string>
 #include <vector>
 
+#include <cm/optional>
 #include <cm/string_view>
 
 #include "cmsys/Process.h"
@@ -377,6 +380,39 @@ public:
   /** Append multiple variables to the current environment. */
   static void AppendEnv(std::vector<std::string> const& env);
 
+  /**
+   * Helper class to represent an environment diff directly. This is to avoid
+   * repeated in-place environment modification (i.e. via setenv/putenv), which
+   * could be slow.
+   */
+  class EnvDiff
+  {
+  public:
+    /** Append multiple variables to the current environment diff */
+    void AppendEnv(std::vector<std::string> const& env);
+
+    /**
+     * Add a single variable (or remove if no = sign) to the current
+     * environment diff.
+     */
+    void PutEnv(const std::string& env);
+
+    /**
+     * Apply an ENVIRONMENT_MODIFICATION operation to this diff. Returns
+     * false and issues an error on parse failure.
+     */
+    bool ParseOperation(const std::string& envmod);
+
+    /**
+     * Apply this diff to the actual environment, optionally writing out the
+     * modifications to a CTest-compatible measurement stream.
+     */
+    void ApplyToCurrentEnv(std::ostringstream* measurement = nullptr);
+
+  private:
+    std::map<std::string, cm::optional<std::string>> diff;
+  };
+
   /** Helper class to save and restore the environment.
       Instantiate this class as an automatic variable on
       the stack. Its constructor saves a copy of the current
diff --git a/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-prop-result.txt b/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-prop-result.txt
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-prop-result.txt
@@ -0,0 +1 @@
+0
diff --git a/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-prop-stdout.txt b/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-prop-stdout.txt
new file mode 100644
index 0000000..a7eae05
--- /dev/null
+++ b/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-prop-stdout.txt
@@ -0,0 +1 @@
+CTEST_TEST_VAR=set-via-ENVIRONMENT-property
diff --git a/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-prop.cmake b/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-prop.cmake
new file mode 100644
index 0000000..51e7f2a
--- /dev/null
+++ b/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-prop.cmake
@@ -0,0 +1,9 @@
+include(CTest)
+
+add_test(NAME cmake_environment COMMAND "${CMAKE_COMMAND}" -E environment)
+set_tests_properties(
+    cmake_environment
+    PROPERTIES
+    ENVIRONMENT "CTEST_TEST_VAR=set-via-ENVIRONMENT-property"
+    ENVIRONMENT_MODIFICATION "CTEST_TEST_VAR=set:set-via-ENVIRONMENT_MODIFICATION;CTEST_TEST_VAR=reset:"
+)
diff --git a/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-system-result.txt b/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-system-result.txt
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-system-result.txt
@@ -0,0 +1 @@
+0
diff --git a/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-system-stdout.txt b/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-system-stdout.txt
new file mode 100644
index 0000000..beaf133
--- /dev/null
+++ b/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-system-stdout.txt
@@ -0,0 +1 @@
+CTEST_TEST_VAR=set-via-system-environment
diff --git a/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-system.cmake b/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-system.cmake
new file mode 100644
index 0000000..23268aa
--- /dev/null
+++ b/Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-system.cmake
@@ -0,0 +1,9 @@
+include(CTest)
+
+add_test(NAME cmake_environment COMMAND "${CMAKE_COMMAND}" -E environment)
+set_tests_properties(
+    cmake_environment
+    PROPERTIES
+    # ENVIRONMENT "CTEST_TEST_VAR=set-via-ENVIRONMENT-property"
+    ENVIRONMENT_MODIFICATION "CTEST_TEST_VAR=set:set-via-ENVIRONMENT_MODIFICATION;CTEST_TEST_VAR=reset:"
+)
diff --git a/Tests/RunCMake/ctest_environment/RunCMakeTest.cmake b/Tests/RunCMake/ctest_environment/RunCMakeTest.cmake
index 3447779..365c9e8 100644
--- a/Tests/RunCMake/ctest_environment/RunCMakeTest.cmake
+++ b/Tests/RunCMake/ctest_environment/RunCMakeTest.cmake
@@ -10,3 +10,7 @@ set(RunCTest_VERBOSE_FLAG "-VV")
 run_ctest(ENVIRONMENT_MODIFICATION-invalid-op)
 run_ctest(ENVIRONMENT_MODIFICATION-no-colon)
 run_ctest(ENVIRONMENT_MODIFICATION-no-equals)
+
+set(ENV{CTEST_TEST_VAR} set-via-system-environment)
+run_ctest(ENVIRONMENT_MODIFICATION-reset-to-prop)
+run_ctest(ENVIRONMENT_MODIFICATION-reset-to-system)