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