From ae5f5069061b1ddfd5f0609fd99204ef5be4fb77 Mon Sep 17 00:00:00 2001 From: Daksh Mamodiya Date: Mon, 29 Jun 2026 17:48:16 +0200 Subject: [PATCH] Install: Exit non-zero when an install script fails `cmake --install` did not propagate per-script failures into its process exit code. In parallel mode, cmInstallScriptHandler::Install spawned each install script as a child process but never inspected the child exit status or termination signal, and always returned 0. It now reads each child's status after the event loop, prints the failing script's exit code or signal, and returns non-zero if any script failed. A failed parallel install also no longer writes the combined install_manifest.txt, so a partial manifest is not mistaken for a complete install. In serial mode, GetScripts() returns the top-level cmake_install.cmake once per component and per configuration, so multiple scripts run only when installing several components or configurations at once. The loop overwrote its result on every iteration, so an earlier script that failed via cmake_language(EXIT) was masked by a later one that succeeded. The loop now stops at the first failure. This is a no-op for a single component and configuration; it changes only installs of multiple components or configurations, which now stop at the first failed script and report it instead of attempting the rest. Fixes: #27906 --- .../dev/install-exit-code-fidelity.rst | 9 ++ Source/cmInstallScriptHandler.cxx | 124 +++++++++++++----- Source/cmInstallScriptHandler.h | 4 +- Source/cmakemain.cxx | 5 + .../InstallParallel/RunCMakeTest.cmake | 42 ++++++ .../fail-subdir/CMakeLists.txt | 5 + .../install-component-mask.cmake | 3 + .../InstallParallel/install-fail.cmake | 6 + .../InstallParallel/ok-subdir/CMakeLists.txt | 1 + .../InstallParallel/parallel-exit-result.txt | 1 + .../InstallParallel/parallel-exit-stdout.txt | 1 + .../parallel-fatal-check.cmake | 4 + .../InstallParallel/parallel-fatal-result.txt | 1 + .../InstallParallel/serial-fatal-result.txt | 1 + .../InstallParallel/serial-mask-check.cmake | 5 + .../InstallParallel/serial-mask-result.txt | 1 + 16 files changed, 179 insertions(+), 34 deletions(-) create mode 100644 Help/release/dev/install-exit-code-fidelity.rst create mode 100644 Tests/RunCMake/InstallParallel/fail-subdir/CMakeLists.txt create mode 100644 Tests/RunCMake/InstallParallel/install-component-mask.cmake create mode 100644 Tests/RunCMake/InstallParallel/install-fail.cmake create mode 100644 Tests/RunCMake/InstallParallel/ok-subdir/CMakeLists.txt create mode 100644 Tests/RunCMake/InstallParallel/parallel-exit-result.txt create mode 100644 Tests/RunCMake/InstallParallel/parallel-exit-stdout.txt create mode 100644 Tests/RunCMake/InstallParallel/parallel-fatal-check.cmake create mode 100644 Tests/RunCMake/InstallParallel/parallel-fatal-result.txt create mode 100644 Tests/RunCMake/InstallParallel/serial-fatal-result.txt create mode 100644 Tests/RunCMake/InstallParallel/serial-mask-check.cmake create mode 100644 Tests/RunCMake/InstallParallel/serial-mask-result.txt diff --git a/Help/release/dev/install-exit-code-fidelity.rst b/Help/release/dev/install-exit-code-fidelity.rst new file mode 100644 index 0000000000..a49758bbc2 --- /dev/null +++ b/Help/release/dev/install-exit-code-fidelity.rst @@ -0,0 +1,9 @@ +install-exit-code-fidelity +--------------------------- + +* The :option:`cmake --install` command now reports a non-zero exit code + when an install script fails. Previously, a parallel install (enabled by + the :prop_gbl:`INSTALL_PARALLEL` global property) always reported success, + and a serial install could lose the failure of an earlier component that + exited via :command:`cmake_language(EXIT) ` when a later + component succeeded. diff --git a/Source/cmInstallScriptHandler.cxx b/Source/cmInstallScriptHandler.cxx index 99a0694f78..d1fb45fa86 100644 --- a/Source/cmInstallScriptHandler.cxx +++ b/Source/cmInstallScriptHandler.cxx @@ -155,49 +155,67 @@ int cmInstallScriptHandler::Install(unsigned int j, for (auto queue = std::min(j - working, runners.size() - i); queue > 0; --queue) { ++working; - runners[i].start(loop, - [&runners, &working, &installed, i, &queueScripts]() { - runners[i].printResult(++installed, runners.size()); - --working; - queueScripts(); - }); + bool started = runners[i].start( + loop, [&runners, &working, &installed, i, &queueScripts]() { + runners[i].printResult(++installed, runners.size()); + --working; + queueScripts(); + }); + if (!started) { + // The child could not be spawned. Release its scheduling slot so the + // remaining scripts still run; it is counted as a failure after the + // event loop completes. + --working; + } ++i; } }; queueScripts(); uv_run(loop, UV_RUN_DEFAULT); - // Write install manifest - std::string installManifest; - for (auto const& component : this->Components) { - if (component.empty()) { - installManifest = "install_manifest.txt"; - } else { - cmsys::RegularExpression regEntry; - if (regEntry.compile("^[a-zA-Z0-9_.+-]+$") && regEntry.find(component)) { - installManifest = cmStrCat("install_manifest_", component, ".txt"); - } else { - cmCryptoHash md5(cmCryptoHash::AlgoMD5); - md5.Initialize(); - installManifest = - cmStrCat("install_manifest_", md5.HashString(component), ".txt"); - } + int result = 0; + for (auto& runner : runners) { + if (runner.Failed()) { + runner.printFailure(); + result = 1; } - cmGeneratedFileStream fout( - cmStrCat(this->BinaryDir, '/', installManifest)); - fout.SetCopyIfDifferent(true); - for (auto const& dir : this->Directories) { - auto localManifest = cmStrCat(dir, "/install_local_manifest.txt"); - if (cmSystemTools::FileExists(localManifest)) { - cmsys::ifstream fin(localManifest.c_str()); - std::string line; - while (std::getline(fin, line)) { - fout << line << "\n"; + } + + // Write the install manifest only when every script succeeded. A failed + // install must not leave behind a manifest that looks complete. + if (result == 0) { + std::string installManifest; + for (auto const& component : this->Components) { + if (component.empty()) { + installManifest = "install_manifest.txt"; + } else { + cmsys::RegularExpression regEntry; + if (regEntry.compile("^[a-zA-Z0-9_.+-]+$") && + regEntry.find(component)) { + installManifest = cmStrCat("install_manifest_", component, ".txt"); + } else { + cmCryptoHash md5(cmCryptoHash::AlgoMD5); + md5.Initialize(); + installManifest = + cmStrCat("install_manifest_", md5.HashString(component), ".txt"); + } + } + cmGeneratedFileStream fout( + cmStrCat(this->BinaryDir, '/', installManifest)); + fout.SetCopyIfDifferent(true); + for (auto const& dir : this->Directories) { + auto localManifest = cmStrCat(dir, "/install_local_manifest.txt"); + if (cmSystemTools::FileExists(localManifest)) { + cmsys::ifstream fin(localManifest.c_str()); + std::string line; + while (std::getline(fin, line)) { + fout << line << "\n"; + } } } } } - return 0; + return result; } InstallScriptRunner::InstallScriptRunner(InstallScript const& script) @@ -207,7 +225,7 @@ InstallScriptRunner::InstallScriptRunner(InstallScript const& script) this->Command = script.command; } -void InstallScriptRunner::start(cm::uv_loop_ptr& loop, +bool InstallScriptRunner::start(cm::uv_loop_ptr& loop, std::function callback) { cmUVProcessChainBuilder builder; @@ -215,6 +233,9 @@ void InstallScriptRunner::start(cm::uv_loop_ptr& loop, .SetExternalLoop(*loop) .SetMergedBuiltinStreams(); this->Chain = cm::make_unique(builder.Start()); + if (!this->Chain->Valid()) { + return false; + } this->StreamHandler = cmUVStreamRead( this->Chain->OutputStream(), [this](std::vector data) { @@ -224,6 +245,7 @@ void InstallScriptRunner::start(cm::uv_loop_ptr& loop, this->Output.push_back(strdata); }, std::move(callback)); + return true; } void InstallScriptRunner::printResult(std::size_t n, std::size_t total) @@ -233,3 +255,39 @@ void InstallScriptRunner::printResult(std::size_t n, std::size_t total) cmSystemTools::Stdout(line); } } + +bool InstallScriptRunner::Failed() const +{ + if (!this->Chain || !this->Chain->Valid()) { + return true; + } + auto const& status = this->Chain->GetStatus(0); + return status.SpawnResult != 0 || status.TermSignal != 0 || + status.ExitStatus != 0; +} + +void InstallScriptRunner::printFailure() +{ + std::string detail; + if (!this->Chain || !this->Chain->Valid()) { + // The chain never spawned a process (e.g. pipe/loop setup failed). + detail = "failed to start"; + } else { + auto const& status = this->Chain->GetStatus(0); + auto exception = status.GetException(); + switch (exception.first) { + case cmUVProcessChain::ExceptionCode::None: + detail = cmStrCat("exited with code ", status.ExitStatus); + break; + case cmUVProcessChain::ExceptionCode::Spawn: + // Prepared, but the process could not be executed. + detail = cmStrCat("failed to start: ", exception.second); + break; + default: + detail = exception.second; + break; + } + } + cmSystemTools::Stderr( + cmStrCat("CMake Error: install script '", this->Name, "' ", detail, '\n')); +} diff --git a/Source/cmInstallScriptHandler.h b/Source/cmInstallScriptHandler.h index 9ab8f8e0fc..180c5d2538 100644 --- a/Source/cmInstallScriptHandler.h +++ b/Source/cmInstallScriptHandler.h @@ -36,8 +36,10 @@ public: { public: InstallScriptRunner(InstallScript const&); - void start(cm::uv_loop_ptr&, std::function); + bool start(cm::uv_loop_ptr&, std::function); void printResult(std::size_t n, std::size_t total); + bool Failed() const; + void printFailure(); private: std::vector Command; diff --git a/Source/cmakemain.cxx b/Source/cmakemain.cxx index b9545c9ba0..97b1712097 100644 --- a/Source/cmakemain.cxx +++ b/Source/cmakemain.cxx @@ -969,6 +969,11 @@ int do_install(int ac, char const* const* av) }); cm.SetDebugOutputOn(verbose); ret_ = int(bool(cm.Run(cmd))); + if (ret_ != 0) { + // Serial install is fail-fast: stop at the first failed script + // instead of attempting the remaining components or configs. + break; + } } } return int(ret_ > 0); diff --git a/Tests/RunCMake/InstallParallel/RunCMakeTest.cmake b/Tests/RunCMake/InstallParallel/RunCMakeTest.cmake index d491b66546..63fb07f7fe 100644 --- a/Tests/RunCMake/InstallParallel/RunCMakeTest.cmake +++ b/Tests/RunCMake/InstallParallel/RunCMakeTest.cmake @@ -58,3 +58,45 @@ if(RunCMake_GENERATOR MATCHES "Ninja") install_test(ninja-parallel ARGS "-t install/parallel" NINJA PARALLEL) install_test(ninja-no-parallel ARGS "-t install" NINJA) endif() + +# Exit-code fidelity: a failing install must report a non-zero exit code. +function(install_fail_test test fixture) + cmake_parse_arguments(ARG "PARALLEL" "FAIL_MODE" "INSTALL_ARGS" ${ARGN}) + set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/${test}-build) + set(RunCMake_TEST_OPTIONS + -DINSTALL_PARALLEL=${ARG_PARALLEL} + -DCMAKE_INSTALL_PREFIX=install) + if (ARG_FAIL_MODE) + list(APPEND RunCMake_TEST_OPTIONS -DFAIL_MODE=${ARG_FAIL_MODE}) + endif() + set(RunCMake_TEST_OUTPUT_MERGE 1) + if (NOT RunCMake_GENERATOR_IS_MULTI_CONFIG) + list(APPEND RunCMake_TEST_OPTIONS -DCMAKE_BUILD_TYPE=Debug) + endif() + run_cmake(${fixture}) + set(RunCMake_TEST_NO_CLEAN 1) + if (ARG_PARALLEL) + # The install runs in parallel only when CMakeFiles/InstallScripts.json is + # at least as new as CMakeFiles/cmake.check_cache; otherwise the handler + # falls back to running the top-level cmake_install.cmake serially. Both + # files are written during the same configuration, and their relative + # modification times are not reliable on every filesystem, so make the JSON + # newest explicitly to keep this test from intermittently exercising the + # serial fallback (which omits the per-script parallel diagnostics). + file(TOUCH_NOCREATE + ${RunCMake_TEST_BINARY_DIR}/CMakeFiles/InstallScripts.json) + endif() + run_cmake_command(${test} + ${CMAKE_COMMAND} -E env --unset=NINJA_STATUS + ${CMAKE_COMMAND} --install . ${ARG_INSTALL_ARGS}) +endfunction() + +# Parallel: any failing child must make the install exit non-zero. +install_fail_test(parallel-fatal install-fail PARALLEL FAIL_MODE fatal INSTALL_ARGS -j 4) +install_fail_test(parallel-exit install-fail PARALLEL FAIL_MODE exit INSTALL_ARGS -j 4) +# Serial: fail-fast is preserved on a fatal error (regression guard) ... +install_fail_test(serial-fatal install-fail FAIL_MODE fatal) +# ... and a status-only failure of an earlier component is no longer masked +# by a later success (and the later component is not installed). +install_fail_test(serial-mask install-component-mask + INSTALL_ARGS --component comp_fail --component comp_ok) diff --git a/Tests/RunCMake/InstallParallel/fail-subdir/CMakeLists.txt b/Tests/RunCMake/InstallParallel/fail-subdir/CMakeLists.txt new file mode 100644 index 0000000000..5ed1c04759 --- /dev/null +++ b/Tests/RunCMake/InstallParallel/fail-subdir/CMakeLists.txt @@ -0,0 +1,5 @@ +if (FAIL_MODE STREQUAL "exit") + install(CODE [[cmake_language(EXIT 7)]]) +else () + install(CODE [[message(FATAL_ERROR "intentional install failure")]]) +endif () diff --git a/Tests/RunCMake/InstallParallel/install-component-mask.cmake b/Tests/RunCMake/InstallParallel/install-component-mask.cmake new file mode 100644 index 0000000000..d1dd236d76 --- /dev/null +++ b/Tests/RunCMake/InstallParallel/install-component-mask.cmake @@ -0,0 +1,3 @@ +install(CODE [[cmake_language(EXIT 7)]] COMPONENT comp_fail) +install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/CMakeLists.txt TYPE DATA RENAME ok.txt + COMPONENT comp_ok) diff --git a/Tests/RunCMake/InstallParallel/install-fail.cmake b/Tests/RunCMake/InstallParallel/install-fail.cmake new file mode 100644 index 0000000000..d582ada3c8 --- /dev/null +++ b/Tests/RunCMake/InstallParallel/install-fail.cmake @@ -0,0 +1,6 @@ +install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/CMakeLists.txt TYPE DATA RENAME root.txt) +if (INSTALL_PARALLEL) + set_property(GLOBAL PROPERTY INSTALL_PARALLEL ON) +endif() +add_subdirectory(fail-subdir) +add_subdirectory(ok-subdir) diff --git a/Tests/RunCMake/InstallParallel/ok-subdir/CMakeLists.txt b/Tests/RunCMake/InstallParallel/ok-subdir/CMakeLists.txt new file mode 100644 index 0000000000..5c9d7fa78c --- /dev/null +++ b/Tests/RunCMake/InstallParallel/ok-subdir/CMakeLists.txt @@ -0,0 +1 @@ +install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/CMakeLists.txt TYPE DATA RENAME ok.txt) diff --git a/Tests/RunCMake/InstallParallel/parallel-exit-result.txt b/Tests/RunCMake/InstallParallel/parallel-exit-result.txt new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/Tests/RunCMake/InstallParallel/parallel-exit-result.txt @@ -0,0 +1 @@ +1 diff --git a/Tests/RunCMake/InstallParallel/parallel-exit-stdout.txt b/Tests/RunCMake/InstallParallel/parallel-exit-stdout.txt new file mode 100644 index 0000000000..5c72e6a592 --- /dev/null +++ b/Tests/RunCMake/InstallParallel/parallel-exit-stdout.txt @@ -0,0 +1 @@ +install script '[^']*' exited with code 7 diff --git a/Tests/RunCMake/InstallParallel/parallel-fatal-check.cmake b/Tests/RunCMake/InstallParallel/parallel-fatal-check.cmake new file mode 100644 index 0000000000..1e8a4c23a7 --- /dev/null +++ b/Tests/RunCMake/InstallParallel/parallel-fatal-check.cmake @@ -0,0 +1,4 @@ +if (EXISTS "${RunCMake_TEST_BINARY_DIR}/install_manifest.txt") + set(RunCMake_TEST_FAILED + "install_manifest.txt was written despite a failed parallel install") +endif () diff --git a/Tests/RunCMake/InstallParallel/parallel-fatal-result.txt b/Tests/RunCMake/InstallParallel/parallel-fatal-result.txt new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/Tests/RunCMake/InstallParallel/parallel-fatal-result.txt @@ -0,0 +1 @@ +1 diff --git a/Tests/RunCMake/InstallParallel/serial-fatal-result.txt b/Tests/RunCMake/InstallParallel/serial-fatal-result.txt new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/Tests/RunCMake/InstallParallel/serial-fatal-result.txt @@ -0,0 +1 @@ +1 diff --git a/Tests/RunCMake/InstallParallel/serial-mask-check.cmake b/Tests/RunCMake/InstallParallel/serial-mask-check.cmake new file mode 100644 index 0000000000..3e8cb97e2f --- /dev/null +++ b/Tests/RunCMake/InstallParallel/serial-mask-check.cmake @@ -0,0 +1,5 @@ +file(GLOB_RECURSE ok_files "${RunCMake_TEST_BINARY_DIR}/install/*ok.txt") +if (ok_files) + set(RunCMake_TEST_FAILED + "comp_ok was installed after comp_fail failed; serial fail-fast not honored: ${ok_files}") +endif () diff --git a/Tests/RunCMake/InstallParallel/serial-mask-result.txt b/Tests/RunCMake/InstallParallel/serial-mask-result.txt new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/Tests/RunCMake/InstallParallel/serial-mask-result.txt @@ -0,0 +1 @@ +1