cmCTestRunTest: Don't modify the current environment

Instead, pass the environment variables to the child process.

Source/CTest/cmCTestRunTest.cxx

@@ -21,6 +21,7 @@
 #include "cmCTestMemCheckHandler.h"
 #include "cmCTestMultiProcessHandler.h"
 #include "cmDuration.h"
+#include "cmEnvironment.h"
 #include "cmInstrumentation.h"
 #include "cmProcess.h"
 #include "cmStringAlgorithms.h"
@@ -853,62 +854,36 @@ bool cmCTestRunTest::ForkProcess()
                        this->TestHandler->GetQuiet());
   }
 
-  cmSystemTools::SaveRestoreEnvironment sre;
+  // Record the original environment before modifying it
-  std::ostringstream envMeasurement;
+  auto const originalEnvironment =
-
+    cmEnvironment{ cmSystemTools::GetEnvironmentVariables() };
-  // 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 (!this->TestProperties->Environment.empty()) {
-    cmSystemTools::EnvDiff diff;
-    diff.AppendEnv(this->TestProperties->Environment);
-    diff.ApplyToCurrentEnv(&envMeasurement);
-  }
 
+  auto env = originalEnvironment;
+  env.Update(this->TestProperties->Environment);
   if (!this->TestProperties->EnvironmentModification.empty()) {
-    cmSystemTools::EnvDiff diff;
+    auto diff = cmEnvironmentModification{};
-    bool env_ok = true;
+    if (!diff.Add(this->TestProperties->EnvironmentModification)) {
-
-    for (auto const& envmod : this->TestProperties->EnvironmentModification) {
-      env_ok &= diff.ParseOperation(envmod);
-    }
-
-    if (!env_ok) {
       return false;
     }
-
+    diff.ApplyTo(env);
-    diff.ApplyToCurrentEnv(&envMeasurement);
   }
 
   if (this->UseAllocatedResources) {
-    std::vector<std::string> envLog;
+    this->SetupResourcesEnvironment(env);
-    this->SetupResourcesEnvironment(&envLog);
-    for (auto const& var : envLog) {
-      envMeasurement << var << std::endl;
-    }
   } else {
-    cmSystemTools::UnsetEnv("CTEST_RESOURCE_GROUP_COUNT");
+    env.UnPutEnv("CTEST_RESOURCE_GROUP_COUNT");
-    // Signify that this variable is being actively unset
-    envMeasurement << "#CTEST_RESOURCE_GROUP_COUNT=" << std::endl;
   }
 
-  this->TestResult.Environment = envMeasurement.str();
+  this->TestProcess->SetEnvironment(env.GetVariables());
-  // Remove last newline
+  this->TestResult.Environment = env.RecordDifference(originalEnvironment);
-  this->TestResult.Environment.erase(this->TestResult.Environment.length() -
-                                     1);
-
   return this->TestProcess->StartProcess(*this->MultiTestHandler.Loop,
                                          &this->TestProperties->Affinity);
 }
 
-void cmCTestRunTest::SetupResourcesEnvironment(std::vector<std::string>* log)
+void cmCTestRunTest::SetupResourcesEnvironment(cmEnvironment& env)
 {
-  std::string processCount =
+  env.PutEnv(
-    cmStrCat("CTEST_RESOURCE_GROUP_COUNT=", this->AllocatedResources.size());
+    cmStrCat("CTEST_RESOURCE_GROUP_COUNT=", this->AllocatedResources.size()));
-  cmSystemTools::PutEnv(processCount);
-  if (log) {
-    log->emplace_back(std::move(processCount));
-  }
 
   std::size_t i = 0;
   for (auto const& process : this->AllocatedResources) {
@@ -933,15 +908,9 @@ void cmCTestRunTest::SetupResourcesEnvironment(std::vector<std::string>* log)
         firstName = false;
         var += cmStrCat("id:", it2.Id, ",slots:", it2.Slots);
       }
-      cmSystemTools::PutEnv(var);
+      env.PutEnv(var);
-      if (log) {
-        log->push_back(var);
-      }
-    }
-    cmSystemTools::PutEnv(resourceList);
-    if (log) {
-      log->push_back(resourceList);
     }
+    env.PutEnv(resourceList);
     ++i;
   }
 }

Source/CTest/cmCTestRunTest.h

@@ -16,6 +16,8 @@
 #include "cmCTestTestHandler.h"
 #include "cmProcess.h"
 
+class cmEnvironment;
+
 /** \class cmRunTest
  * \brief represents a single test to be run
  *
@@ -113,7 +115,7 @@ private:
   // Run post processing of the process output for MemCheck
   void MemCheckPostProcess();
 
-  void SetupResourcesEnvironment(std::vector<std::string>* log = nullptr);
+  void SetupResourcesEnvironment(cmEnvironment& env);
 
   // Returns "completed/total Test #Index: "
   std::string GetTestPrefix(size_t completed, size_t total) const;

Source/CTest/cmProcess.cxx

@@ -35,19 +35,24 @@ cmProcess::cmProcess(std::unique_ptr<cmCTestRunTest> runner)
 
 cmProcess::~cmProcess() = default;
 
-void cmProcess::SetCommand(std::string const& command)
+void cmProcess::SetCommand(std::string command)
 {
-  this->Command = command;
+  this->Command = std::move(command);
 }
 
-void cmProcess::SetCommandArguments(std::vector<std::string> const& args)
+void cmProcess::SetCommandArguments(std::vector<std::string> args)
 {
-  this->Arguments = args;
+  this->Arguments = std::move(args);
 }
 
-void cmProcess::SetWorkingDirectory(std::string const& dir)
+void cmProcess::SetEnvironment(std::vector<std::string> env)
 {
-  this->WorkingDirectory = dir;
+  this->Environment = std::move(env);
+}
+
+void cmProcess::SetWorkingDirectory(std::string dir)
+{
+  this->WorkingDirectory = std::move(dir);
 }
 
 bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
@@ -128,6 +133,15 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
 #else
   static_cast<void>(affinity);
 #endif
+  if (!this->Environment.empty()) {
+    this->Env.clear();
+    this->Env.reserve(this->Environment.size() + 1);
+    for (auto const& var : this->Environment) {
+      this->Env.push_back(var.c_str());
+    }
+    this->Env.push_back(nullptr);
+    options.env = const_cast<char**>(this->Env.data());
+  }
 
   status =
     uv_read_start(pipe_reader, &cmProcess::OnAllocateCB, &cmProcess::OnReadCB);

Source/CTest/cmProcess.h

@@ -34,9 +34,10 @@ class cmProcess
 public:
   explicit cmProcess(std::unique_ptr<cmCTestRunTest> runner);
   ~cmProcess();
-  void SetCommand(std::string const& command);
+  void SetCommand(std::string command);
-  void SetCommandArguments(std::vector<std::string> const& arg);
+  void SetCommandArguments(std::vector<std::string> arg);
-  void SetWorkingDirectory(std::string const& dir);
+  void SetEnvironment(std::vector<std::string> env);
+  void SetWorkingDirectory(std::string dir);
   void SetStopTimeout(cmDuration t) { this->StopTimeout = t; }
   void SetTimeout(cmDuration t) { this->Timeout = t; }
   void ChangeTimeout(cmDuration t);
@@ -161,6 +162,8 @@ private:
   std::string WorkingDirectory;
   std::vector<std::string> Arguments;
   std::vector<char const*> ProcessArgs;
+  std::vector<std::string> Environment;
+  std::vector<char const*> Env;
   int Id;
   int64_t ExitValue;
   Termination TerminationStyle = Termination::Normal;

Source/cmEnvironment.cxx

@@ -3,6 +3,7 @@
 #include "cmEnvironment.h"
 
 #include <set>
+#include <sstream>
 #include <utility>
 
 #include <cm/string_view>
@@ -69,6 +70,29 @@ std::vector<std::string> cmEnvironment::GetVariables() const
   return result;
 }
 
+std::string cmEnvironment::RecordDifference(
+  cmEnvironment const& original) const
+{
+  cm::string_view nl;
+  std::ostringstream os;
+  for (auto const& elem : this->Map) {
+    if (!elem.second) {
+      // Signify that this variable is being actively unset
+      os << nl << '#' << elem.first << '=';
+      nl = "\n";
+      continue;
+    }
+    auto const it = original.Map.find(elem.first);
+    if (it != original.Map.end() && *elem.second == it->second) {
+      // Skip variables that are unchanged
+      continue;
+    }
+    os << nl << elem.first << '=' << *elem.second;
+    nl = "\n";
+  }
+  return os.str();
+}
+
 namespace {
 
 auto const ValidOperators = std::set<cm::string_view>{

Source/cmEnvironment.h

@@ -46,6 +46,8 @@ public:
 
   std::vector<std::string> GetVariables() const;
 
+  std::string RecordDifference(cmEnvironment const& original) const;
+
 protected:
   struct EnvNameLess
   {

Tests/RunCMake/CTestResourceAllocation/process_count-ctest-s-res-check.cmake

@@ -1,9 +1,9 @@
 verify_ctest_resources()
 
 read_testing_file("Test.xml" _test_contents)
-if(NOT _test_contents MATCHES "\nCTEST_RESOURCE_GROUP_0=widgets")
+if(NOT _test_contents MATCHES "(>|\n)CTEST_RESOURCE_GROUP_0=widgets")
   string(APPEND RunCMake_TEST_FAILED "Could not find variable CTEST_RESOURCE_GROUP_0 in test measurements\n")
 endif()
-if(NOT _test_contents MATCHES "\nCTEST_RESOURCE_GROUP_0_WIDGETS=id:")
+if(NOT _test_contents MATCHES "(>|\n)CTEST_RESOURCE_GROUP_0_WIDGETS=id:")
   string(APPEND RunCMake_TEST_FAILED "Could not find variable CTEST_RESOURCE_GROUP_0_WIDGETS in test measurements\n")
 endif()

Tests/RunCMake/ctest_test/TestEnvironment-check.cmake

@@ -2,7 +2,7 @@ file(READ "${RunCMake_TEST_BINARY_DIR}/Testing/TAG" _tag)
 string(REGEX REPLACE "^([^\n]*)\n.*$" "\\1" _date "${_tag}")
 file(READ "${RunCMake_TEST_BINARY_DIR}/Testing/${_date}/Test.xml" _test_contents)
 
-if(NOT _test_contents MATCHES "<Value>ENV1=env1\nENV2=env2\n#CTEST_RESOURCE_GROUP_COUNT=</Value>")
+if(NOT _test_contents MATCHES "<Value>#CTEST_RESOURCE_GROUP_COUNT=\nENV1=env1\nENV2=env2</Value>")
   string(APPEND RunCMake_TEST_FAILED "Could not find expected environment variables in Test.xml")
 endif()
 if(_test_contents MATCHES "BAD_ENVIRONMENT_VARIABLE")