From 4e6cbb1f132540cb1a283d99aad8c1890f06145d Mon Sep 17 00:00:00 2001 From: Alex Reinking Date: Sun, 14 Aug 2022 02:04:49 -0400 Subject: cmCTestRunTest: Remove unnecessary CMAKE_BOOTSTRAP guard CTest is not compiled during CMake's `bootstrap` build. --- Source/CTest/cmCTestRunTest.cxx | 2 -- 1 file changed, 2 deletions(-) diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx index 2a2cb1c..e19739d 100644 --- a/Source/CTest/cmCTestRunTest.cxx +++ b/Source/CTest/cmCTestRunTest.cxx @@ -787,9 +787,7 @@ bool cmCTestRunTest::ForkProcess( this->TestProcess->SetTimeout(timeout); -#ifndef CMAKE_BOOTSTRAP cmSystemTools::SaveRestoreEnvironment sre; -#endif std::ostringstream envMeasurement; if (environment && !environment->empty()) { -- cgit v0.12 From a0b1c4ee907ee135fdb5727084fc90a07f525470 Mon Sep 17 00:00:00 2001 From: Alex Reinking Date: Sun, 14 Aug 2022 02:08:09 -0400 Subject: cmCTestRunTest: Simplify by using GetSystemPathlistSeparator Part of the implementation of `ENVIRONMENT_MODIFICATION` replicated the logic in this function. Using it here de-duplicates code and will be useful during the upcoming refactoring. --- Source/CTest/cmCTestRunTest.cxx | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx index e19739d..88b45df 100644 --- a/Source/CTest/cmCTestRunTest.cxx +++ b/Source/CTest/cmCTestRunTest.cxx @@ -803,11 +803,7 @@ bool cmCTestRunTest::ForkProcess( if (environment_modification && !environment_modification->empty()) { std::map> env_application; -#ifdef _WIN32 - char path_sep = ';'; -#else - char path_sep = ':'; -#endif + char path_sep = cmSystemTools::GetSystemPathlistSeparator(); auto apply_diff = [&env_application](const std::string& name, -- cgit v0.12 From bfa1c5285b0db8f7dfe8e7c5ac546312723f59db Mon Sep 17 00:00:00 2001 From: Alex Reinking Date: Sun, 14 Aug 2022 02:28:24 -0400 Subject: cmSystemTools: Add EnvDiff class to hold ENVIRONMENT_MODIFICATION logic Prepare to re-use this logic when enhancing `cmake -E env`. --- Source/CTest/cmCTestRunTest.cxx | 127 ++-------------------------------------- Source/cmSystemTools.cxx | 121 ++++++++++++++++++++++++++++++++++++++ Source/cmSystemTools.h | 27 +++++++++ 3 files changed, 154 insertions(+), 121 deletions(-) diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx index 88b45df..11be132 100644 --- a/Source/CTest/cmCTestRunTest.cxx +++ b/Source/CTest/cmCTestRunTest.cxx @@ -8,16 +8,12 @@ #include #include #include -#include #include #include #include #include #include -#include -#include -#include #include "cmsys/RegularExpression.hxx" @@ -793,7 +789,7 @@ bool cmCTestRunTest::ForkProcess( 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`. + // be updates in `EnvDiff::ParseOperation`. cmSystemTools::AppendEnv(*environment); for (auto const& var : *environment) { envMeasurement << var << std::endl; @@ -801,129 +797,18 @@ bool cmCTestRunTest::ForkProcess( } if (environment_modification && !environment_modification->empty()) { - std::map> env_application; - - char path_sep = cmSystemTools::GetSystemPathlistSeparator(); - - auto apply_diff = - [&env_application](const std::string& name, - std::function const& apply) { - cm::optional 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..c35c1e7 100644 --- a/Source/cmSystemTools.cxx +++ b/Source/cmSystemTools.cxx @@ -16,6 +16,7 @@ #include #include +#include #include @@ -1563,6 +1564,126 @@ void cmSystemTools::AppendEnv(std::vector const& env) } } +bool cmSystemTools::EnvDiff::ParseOperation(const std::string& envmod) +{ + char path_sep = GetSystemPathlistSeparator(); + + auto apply_diff = [this](const std::string& name, + std::function const& apply) { + cm::optional old_value = diff[name]; + std::string output; + if (old_value) { + output = *old_value; + } else { + // This only works because the environment is actually modified when + // processing the ENVIRONMENT property in CTest and cmake -E env + // (`AppendEnv`). If either one 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); + 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..3c5c28c 100644 --- a/Source/cmSystemTools.h +++ b/Source/cmSystemTools.h @@ -6,9 +6,12 @@ #include #include +#include +#include #include #include +#include #include #include "cmsys/Process.h" @@ -377,6 +380,30 @@ public: /** Append multiple variables to the current environment. */ static void AppendEnv(std::vector 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: + /** + * 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> 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 -- cgit v0.12 From e2854b4fa25778b66473a75a0bf3ca4bfdec5a54 Mon Sep 17 00:00:00 2001 From: Alex Reinking Date: Sun, 14 Aug 2022 04:11:35 -0400 Subject: cmCTestRunTest: Implement the ENVIRONMENT test property with EnvDiff too Going through the same internal API for both `ENVIRONMENT` and `ENVIRONMENT_MODIFICATION` properties will make it easier to implement checkpointing for `MYVAR=reset:` more efficiently if the need ever presents itself. It also makes the two-stage nature of the environment mutation clearer in the code itself. --- Source/CTest/cmCTestRunTest.cxx | 15 +++++++-------- Source/cmSystemTools.cxx | 23 ++++++++++++++++++----- Source/cmSystemTools.h | 9 +++++++++ 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx index 11be132..5efe69f 100644 --- a/Source/CTest/cmCTestRunTest.cxx +++ b/Source/CTest/cmCTestRunTest.cxx @@ -784,16 +784,15 @@ bool cmCTestRunTest::ForkProcess( this->TestProcess->SetTimeout(timeout); 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 (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 in `EnvDiff::ParseOperation`. - 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()) { diff --git a/Source/cmSystemTools.cxx b/Source/cmSystemTools.cxx index c35c1e7..2c32f09 100644 --- a/Source/cmSystemTools.cxx +++ b/Source/cmSystemTools.cxx @@ -1564,6 +1564,24 @@ void cmSystemTools::AppendEnv(std::vector const& env) } } +void cmSystemTools::EnvDiff::AppendEnv(std::vector 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(); @@ -1575,11 +1593,6 @@ bool cmSystemTools::EnvDiff::ParseOperation(const std::string& envmod) if (old_value) { output = *old_value; } else { - // This only works because the environment is actually modified when - // processing the ENVIRONMENT property in CTest and cmake -E env - // (`AppendEnv`). If either one 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; diff --git a/Source/cmSystemTools.h b/Source/cmSystemTools.h index 3c5c28c..e8c04b1 100644 --- a/Source/cmSystemTools.h +++ b/Source/cmSystemTools.h @@ -388,6 +388,15 @@ public: class EnvDiff { public: + /** Append multiple variables to the current environment diff */ + void AppendEnv(std::vector 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. -- cgit v0.12 From a5d45e685f193bca3c9506d50141c459df6662b0 Mon Sep 17 00:00:00 2001 From: Alex Reinking Date: Sun, 14 Aug 2022 04:57:40 -0400 Subject: Tests: Add case for ENVIRONMENT_MODIFICATION property OP=reset behavior When processing the reset operation in the context of a CTest `ENVIRONMENT_MODIFICATION` property, the value the variable is reset to is the one it had after `ENVIRONMENT` was processed, not before. This was broken once during refactoring and is subtle enough that it should be tested. --- .../ENVIRONMENT_MODIFICATION-reset-to-prop-result.txt | 1 + .../ENVIRONMENT_MODIFICATION-reset-to-prop-stdout.txt | 1 + .../ENVIRONMENT_MODIFICATION-reset-to-prop.cmake | 9 +++++++++ .../ENVIRONMENT_MODIFICATION-reset-to-system-result.txt | 1 + .../ENVIRONMENT_MODIFICATION-reset-to-system-stdout.txt | 1 + .../ENVIRONMENT_MODIFICATION-reset-to-system.cmake | 9 +++++++++ Tests/RunCMake/ctest_environment/RunCMakeTest.cmake | 4 ++++ 7 files changed, 26 insertions(+) create mode 100644 Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-prop-result.txt create mode 100644 Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-prop-stdout.txt create mode 100644 Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-prop.cmake create mode 100644 Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-system-result.txt create mode 100644 Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-system-stdout.txt create mode 100644 Tests/RunCMake/ctest_environment/ENVIRONMENT_MODIFICATION-reset-to-system.cmake 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) -- cgit v0.12