From ea8e3107ec31bfc92b0aaa6717b8785d65fa8ba4 Mon Sep 17 00:00:00 2001 From: Martin Duffy Date: Wed, 13 Nov 2024 10:25:23 -0500 Subject: [PATCH] cmake --install: Respect CMAKE_DEFAULT_CONFIGS When `cmake --install` is run with no `--config` passed, use `CMAKE_DEFAULT_CONFIGS` to determine which config(s) to install. Fixes: #21475 --- Source/cmGlobalGenerator.cxx | 22 +++--- Source/cmGlobalGenerator.h | 6 ++ Source/cmGlobalNinjaGenerator.h | 2 +- Source/cmInstallScriptHandler.cxx | 67 ++++++++++++++----- Source/cmInstallScriptHandler.h | 10 ++- Source/cmake.cxx | 2 +- Source/cmakemain.cxx | 63 ++++++++--------- Tests/RunCMake/CMakeLists.txt | 1 + .../no-parallel-install-stdout.txt | 2 - .../parallel-install-stdout.txt | 25 ++----- .../NinjaMultiConfig/RunCMakeTest.cmake | 13 ++++ .../cmake--install-install-check.cmake | 4 ++ .../NinjaMultiConfig/cmake--install.cmake | 1 + 13 files changed, 133 insertions(+), 85 deletions(-) create mode 100644 Tests/RunCMake/NinjaMultiConfig/cmake--install-install-check.cmake create mode 100644 Tests/RunCMake/NinjaMultiConfig/cmake--install.cmake diff --git a/Source/cmGlobalGenerator.cxx b/Source/cmGlobalGenerator.cxx index 8c553ee1e1..1d1d932c88 100644 --- a/Source/cmGlobalGenerator.cxx +++ b/Source/cmGlobalGenerator.cxx @@ -1796,15 +1796,21 @@ void cmGlobalGenerator::WriteJsonContent(const std::string& path, void cmGlobalGenerator::WriteInstallJson() const { - if (this->GetCMakeInstance()->GetState()->GetGlobalPropertyAsBool( - "INSTALL_PARALLEL")) { - Json::Value index(Json::objectValue); - index["InstallScripts"] = Json::arrayValue; - for (const auto& file : this->InstallScripts) { - index["InstallScripts"].append(file); - } - this->WriteJsonContent("CMakeFiles/InstallScripts.json", index); + Json::Value index(Json::objectValue); + index["InstallScripts"] = Json::arrayValue; + for (const auto& file : this->InstallScripts) { + index["InstallScripts"].append(file); } + index["Parallel"] = + this->GetCMakeInstance()->GetState()->GetGlobalPropertyAsBool( + "INSTALL_PARALLEL"); + if (this->SupportsDefaultConfigs()) { + index["Configs"] = Json::arrayValue; + for (auto const& config : this->GetDefaultConfigs()) { + index["Configs"].append(config); + } + } + this->WriteJsonContent("CMakeFiles/InstallScripts.json", index); } #endif diff --git a/Source/cmGlobalGenerator.h b/Source/cmGlobalGenerator.h index 92e974c9d0..6b97a508b7 100644 --- a/Source/cmGlobalGenerator.h +++ b/Source/cmGlobalGenerator.h @@ -678,6 +678,12 @@ public: void AddTestFile(std::string const& file); void AddCMakeFilesToRebuild(std::vector& files) const; + virtual const std::set& GetDefaultConfigs() const + { + static std::set configs; + return configs; + } + protected: // for a project collect all its targets by following depend // information, and also collect all the targets diff --git a/Source/cmGlobalNinjaGenerator.h b/Source/cmGlobalNinjaGenerator.h index 7bd7206a21..3af5c1ad1c 100644 --- a/Source/cmGlobalNinjaGenerator.h +++ b/Source/cmGlobalNinjaGenerator.h @@ -466,7 +466,7 @@ public: std::set GetCrossConfigs(const std::string& config) const; - const std::set& GetDefaultConfigs() const + const std::set& GetDefaultConfigs() const override { return this->DefaultConfigs; } diff --git a/Source/cmInstallScriptHandler.cxx b/Source/cmInstallScriptHandler.cxx index a7dfb0c0f4..e4620d1e3d 100644 --- a/Source/cmInstallScriptHandler.cxx +++ b/Source/cmInstallScriptHandler.cxx @@ -33,41 +33,76 @@ using InstallScript = cmInstallScriptHandler::InstallScript; cmInstallScriptHandler::cmInstallScriptHandler(std::string _binaryDir, std::string _component, + std::string _config, std::vector& args) : binaryDir(std::move(_binaryDir)) , component(std::move(_component)) { const std::string& file = cmStrCat(this->binaryDir, "/CMakeFiles/InstallScripts.json"); + this->parallel = false; + + auto addScript = [this, &args](std::string script, + std::string config) -> void { + this->commands.push_back(args); + if (!config.empty()) { + this->commands.back().insert( + this->commands.back().end() - 1, + cmStrCat("-DCMAKE_INSTALL_CONFIG_NAME=", config)); + } + this->commands.back().emplace_back(script); + this->directories.push_back(cmSystemTools::GetFilenamePath(script)); + }; + + int compare = 1; if (cmSystemTools::FileExists(file)) { - int compare; cmSystemTools::FileTimeCompare( cmStrCat(this->binaryDir, "/CMakeFiles/cmake.check_cache"), file, &compare); - if (compare < 1) { + } + if (compare < 1) { + Json::CharReaderBuilder rbuilder; + auto JsonReader = + std::unique_ptr(rbuilder.newCharReader()); + std::vector content; + Json::Value value; + cmJSONState state(file, &value); + this->parallel = value["Parallel"].asBool(); + if (this->parallel) { args.insert(args.end() - 1, "-DCMAKE_INSTALL_LOCAL_ONLY=1"); - Json::CharReaderBuilder rbuilder; - auto JsonReader = - std::unique_ptr(rbuilder.newCharReader()); - std::vector content; - Json::Value value; - cmJSONState state(file, &value); - for (auto const& script : value["InstallScripts"]) { - this->commands.push_back(args); - this->commands.back().emplace_back(script.asCString()); - this->directories.push_back( - cmSystemTools::GetFilenamePath(script.asCString())); + } + if (_config.empty() && value.isMember("Configs")) { + for (auto const& config : value["Configs"]) { + this->configs.push_back(config.asCString()); + } + } else { + this->configs.push_back(_config); + } + for (auto const& script : value["InstallScripts"]) { + for (auto const& config : configs) { + addScript(script.asCString(), config); + } + if (!this->parallel) { + break; } } + } else { + addScript(cmStrCat(this->binaryDir, "/cmake_install.cmake"), _config); } } -bool cmInstallScriptHandler::isParallel() +bool cmInstallScriptHandler::IsParallel() { - return !this->commands.empty(); + return this->parallel; } -int cmInstallScriptHandler::install(unsigned int j) +std::vector> cmInstallScriptHandler::GetCommands() + const +{ + return this->commands; +} + +int cmInstallScriptHandler::Install(unsigned int j) { cm::uv_loop_ptr loop; loop.init(); diff --git a/Source/cmInstallScriptHandler.h b/Source/cmInstallScriptHandler.h index 414304425f..a904dc9cc6 100644 --- a/Source/cmInstallScriptHandler.h +++ b/Source/cmInstallScriptHandler.h @@ -16,9 +16,11 @@ class cmInstallScriptHandler { public: cmInstallScriptHandler() = default; - cmInstallScriptHandler(std::string, std::string, std::vector&); - bool isParallel(); - int install(unsigned int j); + cmInstallScriptHandler(std::string, std::string, std::string, + std::vector&); + bool IsParallel(); + int Install(unsigned int j); + std::vector> GetCommands() const; class InstallScript { public: @@ -38,6 +40,8 @@ public: private: std::vector> commands; std::vector directories; + std::vector configs; std::string binaryDir; std::string component; + bool parallel; }; diff --git a/Source/cmake.cxx b/Source/cmake.cxx index 7d85db4791..728974036e 100644 --- a/Source/cmake.cxx +++ b/Source/cmake.cxx @@ -2933,7 +2933,7 @@ int cmake::Generate() this->SaveCache(this->GetHomeOutputDirectory()); #if !defined(CMAKE_BOOTSTRAP) - this->GetGlobalGenerator()->WriteInstallJson(); + this->GlobalGenerator->WriteInstallJson(); this->FileAPI->WriteReplies(); #endif diff --git a/Source/cmakemain.cxx b/Source/cmakemain.cxx index 6febc97bae..d8072374eb 100644 --- a/Source/cmakemain.cxx +++ b/Source/cmakemain.cxx @@ -924,20 +924,6 @@ int do_install(int ac, char const* const* av) return 1; } - cmake cm(cmake::RoleScript, cmState::Script); - - cmSystemTools::SetMessageCallback( - [&cm](const std::string& msg, const cmMessageMetadata& md) { - cmakemainMessageCallback(msg, md, &cm); - }); - cm.SetProgressCallback([&cm](const std::string& msg, float prog) { - cmakemainProgressCallback(msg, prog, &cm); - }); - cm.SetHomeDirectory(""); - cm.SetHomeOutputDirectory(""); - cm.SetDebugOutputOn(verbose); - cm.SetWorkingMode(cmake::SCRIPT_MODE); - std::vector args{ av[0] }; if (!prefix.empty()) { @@ -952,10 +938,6 @@ int do_install(int ac, char const* const* av) args.emplace_back("-DCMAKE_INSTALL_DO_STRIP=1"); } - if (!config.empty()) { - args.emplace_back("-DCMAKE_INSTALL_CONFIG_NAME=" + config); - } - if (!defaultDirectoryPermissions.empty()) { std::string parsedPermissionsVar; if (!parse_default_directory_permissions(defaultDirectoryPermissions, @@ -970,25 +952,38 @@ int do_install(int ac, char const* const* av) args.emplace_back("-P"); - auto handler = cmInstallScriptHandler(dir, component, args); + auto handler = cmInstallScriptHandler(dir, component, config, args); int ret = 0; - if (!handler.isParallel()) { - args.emplace_back(cmStrCat(dir, "/cmake_install.cmake")); - ret = int(bool(cm.Run(args))); - } else { - if (!jobs) { - jobs = 1; - auto envvar = cmSystemTools::GetEnvVar("CMAKE_INSTALL_PARALLEL_LEVEL"); - if (envvar.has_value()) { - jobs = extract_job_number("", envvar.value()); - if (jobs < 1) { - std::cerr << "Value of CMAKE_INSTALL_PARALLEL_LEVEL environment" - " variable must be a positive integer.\n"; - return 1; - } + if (!jobs && handler.IsParallel()) { + jobs = 1; + auto envvar = cmSystemTools::GetEnvVar("CMAKE_INSTALL_PARALLEL_LEVEL"); + if (envvar.has_value()) { + jobs = extract_job_number("", envvar.value()); + if (jobs < 1) { + std::cerr << "Value of CMAKE_INSTALL_PARALLEL_LEVEL environment" + " variable must be a positive integer.\n"; + return 1; } } - ret = handler.install(jobs); + } + if (handler.IsParallel()) { + ret = handler.Install(jobs); + } else { + for (auto const& cmd : handler.GetCommands()) { + cmake cm(cmake::RoleScript, cmState::Script); + cmSystemTools::SetMessageCallback( + [&cm](const std::string& msg, const cmMessageMetadata& md) { + cmakemainMessageCallback(msg, md, &cm); + }); + cm.SetProgressCallback([&cm](const std::string& msg, float prog) { + cmakemainProgressCallback(msg, prog, &cm); + }); + cm.SetHomeDirectory(""); + cm.SetHomeOutputDirectory(""); + cm.SetDebugOutputOn(verbose); + cm.SetWorkingMode(cmake::SCRIPT_MODE); + ret = int(bool(cm.Run(cmd))); + } } return int(ret > 0); diff --git a/Tests/RunCMake/CMakeLists.txt b/Tests/RunCMake/CMakeLists.txt index 7f001241c7..1174cdba46 100644 --- a/Tests/RunCMake/CMakeLists.txt +++ b/Tests/RunCMake/CMakeLists.txt @@ -240,6 +240,7 @@ if(CMAKE_GENERATOR MATCHES "Ninja") set_property(TEST RunCMake.Ninja APPEND PROPERTY LABELS "Fortran") set(NinjaMultiConfig_ARGS -DCYGWIN=${CYGWIN} -DMSYS=${MSYS} + -DCMAKE_EXECUTABLE_SUFFIX=${CMAKE_EXECUTABLE_SUFFIX} ) if(ninja_test_with_qt_version) list(APPEND NinjaMultiConfig_ARGS diff --git a/Tests/RunCMake/InstallParallel/no-parallel-install-stdout.txt b/Tests/RunCMake/InstallParallel/no-parallel-install-stdout.txt index aa0a9d3f36..4a30a041bc 100644 --- a/Tests/RunCMake/InstallParallel/no-parallel-install-stdout.txt +++ b/Tests/RunCMake/InstallParallel/no-parallel-install-stdout.txt @@ -1,5 +1,3 @@ -\-\- Install configuration:[^ -]* \-\- Installing:[^ ]* \-\- Installing:[^ diff --git a/Tests/RunCMake/InstallParallel/parallel-install-stdout.txt b/Tests/RunCMake/InstallParallel/parallel-install-stdout.txt index 61a52b0520..9c059b0f20 100644 --- a/Tests/RunCMake/InstallParallel/parallel-install-stdout.txt +++ b/Tests/RunCMake/InstallParallel/parallel-install-stdout.txt @@ -1,30 +1,15 @@ -\[1\/5\] [^ -]* -\-\- Install configuration:[^ -]* +\[1\/5\] .* \-\- Installing:[^ ]* -\[2\/5\] [^ -]* -\-\- Install configuration:[^ -]* +\[2\/5\] .* \-\- Installing:[^ ]* -\[3\/5\] [^ -]* -\-\- Install configuration:[^ -]* +\[3\/5\] .* \-\- Installing:[^ ]* -\[4\/5\] [^ -]* -\-\- Install configuration:[^ -]* +\[4\/5\] .* \-\- Installing:[^ ]* -\[5\/5\] [^ -]* -\-\- Install configuration:[^ -]* +\[5\/5\] .* \-\- Installing:[^ ]* diff --git a/Tests/RunCMake/NinjaMultiConfig/RunCMakeTest.cmake b/Tests/RunCMake/NinjaMultiConfig/RunCMakeTest.cmake index f1e8b3028d..aef8870c59 100644 --- a/Tests/RunCMake/NinjaMultiConfig/RunCMakeTest.cmake +++ b/Tests/RunCMake/NinjaMultiConfig/RunCMakeTest.cmake @@ -470,6 +470,19 @@ run_cmake_command(OutputPathPrefix-clean-ninja "${RunCMake_MAKE_PROGRAM}" -f Out unset(RunCMake_TEST_NO_CLEAN) unset(RunCMake_TEST_BINARY_DIR) +# cmake --install test +block() + set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/cmake--install-build) + run_cmake_with_options(cmake--install + -DCMAKE_INSTALL_PREFIX=${RunCMake_TEST_BINARY_DIR}/install + -DCMAKE_CROSS_CONFIGS=all + -DCMAKE_DEFAULT_CONFIGS=Debug\\\\;Release + ) + set(RunCMake_TEST_NO_CLEAN 1) + run_cmake_command(cmake--install-build ${CMAKE_COMMAND} --build .) + run_cmake_command(cmake--install-install ${CMAKE_COMMAND} --install .) +endblock() + # CudaSimple uses separable compilation, which is currently only supported on NVCC. if(CMake_TEST_CUDA) set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/CudaSimple-build) diff --git a/Tests/RunCMake/NinjaMultiConfig/cmake--install-install-check.cmake b/Tests/RunCMake/NinjaMultiConfig/cmake--install-install-check.cmake new file mode 100644 index 0000000000..e6a9f95327 --- /dev/null +++ b/Tests/RunCMake/NinjaMultiConfig/cmake--install-install-check.cmake @@ -0,0 +1,4 @@ +if (NOT EXISTS ${RunCMake_TEST_BINARY_DIR}/install/bin/Debug/exe${CMAKE_EXECUTABLE_SUFFIX} OR + NOT EXISTS ${RunCMake_TEST_BINARY_DIR}/install/bin/Release/exe${CMAKE_EXECUTABLE_SUFFIX}) + set(RunCMake_TEST_FAILED "Multi-Config Install with CMAKE_DEFAULT_CONFIGS set did not install all specified configs by default") +endif() diff --git a/Tests/RunCMake/NinjaMultiConfig/cmake--install.cmake b/Tests/RunCMake/NinjaMultiConfig/cmake--install.cmake new file mode 100644 index 0000000000..e0c31b5293 --- /dev/null +++ b/Tests/RunCMake/NinjaMultiConfig/cmake--install.cmake @@ -0,0 +1 @@ +include(${CMAKE_CURRENT_LIST_DIR}/Install.cmake)