From 508a72fc1cec7a69220d542702cc484ef6de0fef Mon Sep 17 00:00:00 2001 From: Daniel Pfeifer Date: Fri, 6 Mar 2026 23:20:37 +0100 Subject: [PATCH] cmCTestRunTest: Don't modify the current environment Instead, pass the environment variables to the child process. --- Source/CTest/cmCTestRunTest.cxx | 67 +++++-------------- Source/CTest/cmCTestRunTest.h | 4 +- Source/CTest/cmProcess.cxx | 26 +++++-- Source/CTest/cmProcess.h | 9 ++- Source/cmEnvironment.cxx | 24 +++++++ Source/cmEnvironment.h | 2 + .../process_count-ctest-s-res-check.cmake | 4 +- .../ctest_test/TestEnvironment-check.cmake | 2 +- 8 files changed, 76 insertions(+), 62 deletions(-) diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx index 97b1d16989..eefd64b282 100644 --- a/Source/CTest/cmCTestRunTest.cxx +++ b/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; - 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 (!this->TestProperties->Environment.empty()) { - cmSystemTools::EnvDiff diff; - diff.AppendEnv(this->TestProperties->Environment); - diff.ApplyToCurrentEnv(&envMeasurement); - } + // Record the original environment before modifying it + auto const originalEnvironment = + cmEnvironment{ cmSystemTools::GetEnvironmentVariables() }; + auto env = originalEnvironment; + env.Update(this->TestProperties->Environment); if (!this->TestProperties->EnvironmentModification.empty()) { - cmSystemTools::EnvDiff diff; - bool env_ok = true; - - for (auto const& envmod : this->TestProperties->EnvironmentModification) { - env_ok &= diff.ParseOperation(envmod); - } - - if (!env_ok) { + auto diff = cmEnvironmentModification{}; + if (!diff.Add(this->TestProperties->EnvironmentModification)) { return false; } - - diff.ApplyToCurrentEnv(&envMeasurement); + diff.ApplyTo(env); } if (this->UseAllocatedResources) { - std::vector envLog; - this->SetupResourcesEnvironment(&envLog); - for (auto const& var : envLog) { - envMeasurement << var << std::endl; - } + this->SetupResourcesEnvironment(env); } else { - cmSystemTools::UnsetEnv("CTEST_RESOURCE_GROUP_COUNT"); - // Signify that this variable is being actively unset - envMeasurement << "#CTEST_RESOURCE_GROUP_COUNT=" << std::endl; + env.UnPutEnv("CTEST_RESOURCE_GROUP_COUNT"); } - this->TestResult.Environment = envMeasurement.str(); - // Remove last newline - this->TestResult.Environment.erase(this->TestResult.Environment.length() - - 1); - + this->TestProcess->SetEnvironment(env.GetVariables()); + this->TestResult.Environment = env.RecordDifference(originalEnvironment); return this->TestProcess->StartProcess(*this->MultiTestHandler.Loop, &this->TestProperties->Affinity); } -void cmCTestRunTest::SetupResourcesEnvironment(std::vector* log) +void cmCTestRunTest::SetupResourcesEnvironment(cmEnvironment& env) { - std::string processCount = - cmStrCat("CTEST_RESOURCE_GROUP_COUNT=", this->AllocatedResources.size()); - cmSystemTools::PutEnv(processCount); - if (log) { - log->emplace_back(std::move(processCount)); - } + env.PutEnv( + cmStrCat("CTEST_RESOURCE_GROUP_COUNT=", this->AllocatedResources.size())); std::size_t i = 0; for (auto const& process : this->AllocatedResources) { @@ -933,15 +908,9 @@ void cmCTestRunTest::SetupResourcesEnvironment(std::vector* log) firstName = false; var += cmStrCat("id:", it2.Id, ",slots:", it2.Slots); } - cmSystemTools::PutEnv(var); - if (log) { - log->push_back(var); - } - } - cmSystemTools::PutEnv(resourceList); - if (log) { - log->push_back(resourceList); + env.PutEnv(var); } + env.PutEnv(resourceList); ++i; } } diff --git a/Source/CTest/cmCTestRunTest.h b/Source/CTest/cmCTestRunTest.h index 3905dab972..37ec24ad66 100644 --- a/Source/CTest/cmCTestRunTest.h +++ b/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* log = nullptr); + void SetupResourcesEnvironment(cmEnvironment& env); // Returns "completed/total Test #Index: " std::string GetTestPrefix(size_t completed, size_t total) const; diff --git a/Source/CTest/cmProcess.cxx b/Source/CTest/cmProcess.cxx index 0610672906..d0e3d0cd14 100644 --- a/Source/CTest/cmProcess.cxx +++ b/Source/CTest/cmProcess.cxx @@ -35,19 +35,24 @@ cmProcess::cmProcess(std::unique_ptr 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 const& args) +void cmProcess::SetCommandArguments(std::vector args) { - this->Arguments = args; + this->Arguments = std::move(args); } -void cmProcess::SetWorkingDirectory(std::string const& dir) +void cmProcess::SetEnvironment(std::vector 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* affinity) @@ -128,6 +133,15 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector* affinity) #else static_cast(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(this->Env.data()); + } status = uv_read_start(pipe_reader, &cmProcess::OnAllocateCB, &cmProcess::OnReadCB); diff --git a/Source/CTest/cmProcess.h b/Source/CTest/cmProcess.h index a4704b42ed..d475dd3602 100644 --- a/Source/CTest/cmProcess.h +++ b/Source/CTest/cmProcess.h @@ -34,9 +34,10 @@ class cmProcess public: explicit cmProcess(std::unique_ptr runner); ~cmProcess(); - void SetCommand(std::string const& command); - void SetCommandArguments(std::vector const& arg); - void SetWorkingDirectory(std::string const& dir); + void SetCommand(std::string command); + void SetCommandArguments(std::vector arg); + void SetEnvironment(std::vector 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 Arguments; std::vector ProcessArgs; + std::vector Environment; + std::vector Env; int Id; int64_t ExitValue; Termination TerminationStyle = Termination::Normal; diff --git a/Source/cmEnvironment.cxx b/Source/cmEnvironment.cxx index 8832c56542..af5a47c295 100644 --- a/Source/cmEnvironment.cxx +++ b/Source/cmEnvironment.cxx @@ -3,6 +3,7 @@ #include "cmEnvironment.h" #include +#include #include #include @@ -69,6 +70,29 @@ std::vector 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{ diff --git a/Source/cmEnvironment.h b/Source/cmEnvironment.h index 700b7ac897..806015f4c3 100644 --- a/Source/cmEnvironment.h +++ b/Source/cmEnvironment.h @@ -46,6 +46,8 @@ public: std::vector GetVariables() const; + std::string RecordDifference(cmEnvironment const& original) const; + protected: struct EnvNameLess { diff --git a/Tests/RunCMake/CTestResourceAllocation/process_count-ctest-s-res-check.cmake b/Tests/RunCMake/CTestResourceAllocation/process_count-ctest-s-res-check.cmake index 585c36bd4c..727d2f4a52 100644 --- a/Tests/RunCMake/CTestResourceAllocation/process_count-ctest-s-res-check.cmake +++ b/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() diff --git a/Tests/RunCMake/ctest_test/TestEnvironment-check.cmake b/Tests/RunCMake/ctest_test/TestEnvironment-check.cmake index 91c9731d8b..7892454422 100644 --- a/Tests/RunCMake/ctest_test/TestEnvironment-check.cmake +++ b/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 "ENV1=env1\nENV2=env2\n#CTEST_RESOURCE_GROUP_COUNT=") +if(NOT _test_contents MATCHES "#CTEST_RESOURCE_GROUP_COUNT=\nENV1=env1\nENV2=env2") string(APPEND RunCMake_TEST_FAILED "Could not find expected environment variables in Test.xml") endif() if(_test_contents MATCHES "BAD_ENVIRONMENT_VARIABLE")