From 7dbe092d7721d75b63c02aaaaa2945fceec1f65e Mon Sep 17 00:00:00 2001 From: Tyler Yankee Date: Mon, 7 Jul 2025 13:43:55 -0400 Subject: [PATCH] instrumentation: Support preBuild and postBuild hooks on Windows * Use `uv_disable_stdio_inheritance` to resolve the deadlock between the parent build system process and `ctest --wait-and-collect-instrumentation` on Windows. * Remove Windows gating from preBuild and postBuild indexing and update tests and documentation accordingly. Fixes: #26668 --- Help/manual/cmake-instrumentation.7.rst | 4 ++-- Source/cmGlobalNinjaGenerator.cxx | 11 +++++----- Source/cmGlobalNinjaGenerator.h | 3 +-- Source/cmInstrumentation.cxx | 4 ++++ Source/cmLocalUnixMakefileGenerator3.cxx | 12 +++++------ .../Instrumentation/RunCMakeTest.cmake | 20 +++++++++++++++++-- 6 files changed, 36 insertions(+), 18 deletions(-) diff --git a/Help/manual/cmake-instrumentation.7.rst b/Help/manual/cmake-instrumentation.7.rst index 5b32f184fc..64808323e7 100644 --- a/Help/manual/cmake-instrumentation.7.rst +++ b/Help/manual/cmake-instrumentation.7.rst @@ -189,8 +189,8 @@ key is required, but all other fields are optional. should be one of the following: * ``postGenerate`` - * ``preBuild`` (called when ``ninja`` or ``make`` is invoked; unavailable on Windows) - * ``postBuild`` (called when ``ninja`` or ``make`` completes; unavailable on Windows) + * ``preBuild`` (called when ``ninja`` or ``make`` is invoked) + * ``postBuild`` (called when ``ninja`` or ``make`` completes) * ``preCMakeBuild`` (called when ``cmake --build`` is invoked) * ``postCMakeBuild`` (called when ``cmake --build`` completes) * ``postInstall`` diff --git a/Source/cmGlobalNinjaGenerator.cxx b/Source/cmGlobalNinjaGenerator.cxx index bd23906c4e..b886885008 100644 --- a/Source/cmGlobalNinjaGenerator.cxx +++ b/Source/cmGlobalNinjaGenerator.cxx @@ -1772,8 +1772,7 @@ void cmGlobalNinjaGenerator::WriteBuiltinTargets(std::ostream& os) this->WriteTargetRebuildManifest(os); this->WriteTargetClean(os); this->WriteTargetHelp(os); -#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32) - // FIXME(#26668) This does not work on Windows +#ifndef CMAKE_BOOTSTRAP if (this->GetCMakeInstance() ->GetInstrumentation() ->HasPreOrPostBuildHook()) { @@ -1854,8 +1853,7 @@ void cmGlobalNinjaGenerator::WriteTargetRebuildManifest(std::ostream& os) } reBuild.ImplicitDeps.push_back(this->CMakeCacheFile); -#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32) - // FIXME(#26668) This does not work on Windows +#ifndef CMAKE_BOOTSTRAP if (this->GetCMakeInstance() ->GetInstrumentation() ->HasPreOrPostBuildHook()) { @@ -2208,8 +2206,7 @@ void cmGlobalNinjaGenerator::WriteTargetHelp(std::ostream& os) } } -#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32) -// FIXME(#26668) This does not work on Windows +#ifndef CMAKE_BOOTSTRAP void cmGlobalNinjaGenerator::WriteTargetInstrument(std::ostream& os) { // Write rule @@ -2218,11 +2215,13 @@ void cmGlobalNinjaGenerator::WriteTargetInstrument(std::ostream& os) rule.Command = cmStrCat( '"', cmSystemTools::GetCTestCommand(), "\" --start-instrumentation \"", this->GetCMakeInstance()->GetHomeOutputDirectory(), '"'); +# ifndef _WIN32 /* * On Unix systems, Ninja will prefix the command with `/bin/sh -c`. * Use exec so that Ninja is the parent process of the command. */ rule.Command = cmStrCat("exec ", rule.Command); +# endif rule.Description = "Collecting build metrics"; rule.Comment = "Rule to initialize instrumentation daemon."; rule.Restat = "1"; diff --git a/Source/cmGlobalNinjaGenerator.h b/Source/cmGlobalNinjaGenerator.h index 7e02541d9d..c27437b127 100644 --- a/Source/cmGlobalNinjaGenerator.h +++ b/Source/cmGlobalNinjaGenerator.h @@ -535,8 +535,7 @@ private: void WriteTargetRebuildManifest(std::ostream& os); bool WriteTargetCleanAdditional(std::ostream& os); void WriteTargetClean(std::ostream& os); -#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32) - // FIXME(#26668) This does not work on Windows +#ifndef CMAKE_BOOTSTRAP void WriteTargetInstrument(std::ostream& os); #endif void WriteTargetHelp(std::ostream& os); diff --git a/Source/cmInstrumentation.cxx b/Source/cmInstrumentation.cxx index 906a207811..4e2ea6fc16 100644 --- a/Source/cmInstrumentation.cxx +++ b/Source/cmInstrumentation.cxx @@ -621,6 +621,10 @@ std::string cmInstrumentation::ComputeSuffixTime() */ int cmInstrumentation::SpawnBuildDaemon() { + // Do not inherit handles from the parent process, so that the daemon is + // fully detached. This helps prevent deadlock between the two. + uv_disable_stdio_inheritance(); + // preBuild Hook this->CollectTimingData(cmInstrumentationQuery::Hook::PreBuild); diff --git a/Source/cmLocalUnixMakefileGenerator3.cxx b/Source/cmLocalUnixMakefileGenerator3.cxx index 609f46e03f..8971eafc51 100644 --- a/Source/cmLocalUnixMakefileGenerator3.cxx +++ b/Source/cmLocalUnixMakefileGenerator3.cxx @@ -74,21 +74,22 @@ std::string cmSplitExtension(std::string const& in, std::string& base) return ext; } -#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32) +#ifndef CMAKE_BOOTSTRAP // Helper function to add the Start Instrumentation command void addInstrumentationCommand(cmInstrumentation* instrumentation, std::vector& commands) { - // FIXME(#26668) This does not work on Windows if (instrumentation->HasPreOrPostBuildHook()) { std::string instrumentationCommand = "$(CTEST_COMMAND) --start-instrumentation $(CMAKE_BINARY_DIR)"; +# ifndef _WIN32 /* * On Unix systems, Make will prefix the command with `/bin/sh -c`. * Use exec so that Make is the parent process of the command. * Add a `;` to convince BSD make to not optimize out the shell. */ instrumentationCommand = cmStrCat("exec ", instrumentationCommand, " ;"); +# endif commands.push_back(instrumentationCommand); } } @@ -687,8 +688,7 @@ void cmLocalUnixMakefileGenerator3::WriteMakeVariables( "CMAKE_COMMAND = " << cmakeShellCommand << "\n"; -#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32) - // FIXME(#26668) This does not work on Windows +#ifndef CMAKE_BOOTSTRAP if (this->GetCMakeInstance() ->GetInstrumentation() ->HasPreOrPostBuildHook()) { @@ -849,7 +849,7 @@ void cmLocalUnixMakefileGenerator3::WriteSpecialTargetsBottom( std::vector no_depends; commands.push_back(std::move(runRule)); -#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32) +#ifndef CMAKE_BOOTSTRAP addInstrumentationCommand(this->GetCMakeInstance()->GetInstrumentation(), commands); #endif @@ -1855,7 +1855,7 @@ void cmLocalUnixMakefileGenerator3::WriteLocalAllRules( this->ConvertToOutputFormat(cmakefileName, cmOutputConverter::SHELL), " 1"); commands.push_back(std::move(runRule)); -#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32) +#ifndef CMAKE_BOOTSTRAP addInstrumentationCommand(this->GetCMakeInstance()->GetInstrumentation(), commands); #endif diff --git a/Tests/RunCMake/Instrumentation/RunCMakeTest.cmake b/Tests/RunCMake/Instrumentation/RunCMakeTest.cmake index cb94fb172f..40d6809cee 100644 --- a/Tests/RunCMake/Instrumentation/RunCMakeTest.cmake +++ b/Tests/RunCMake/Instrumentation/RunCMakeTest.cmake @@ -129,8 +129,24 @@ instrument(cmake-command-resets-generated NO_WARN CHECK_SCRIPT check-data-dir.cmake ) -# FIXME(#26668) This does not work on Windows -if (UNIX) +if(RunCMake_GENERATOR STREQUAL "NMake Makefiles") + execute_process( + COMMAND "${RunCMake_MAKE_PROGRAM}" -? + OUTPUT_VARIABLE nmake_out + ERROR_VARIABLE nmake_out + RESULT_VARIABLE nmake_res + OUTPUT_STRIP_TRAILING_WHITESPACE + ) + if(nmake_res EQUAL 0 AND nmake_out MATCHES "Program Maintenance Utility[^\n]+Version ([1-9][0-9.]+)") + set(nmake_version "${CMAKE_MATCH_1}") + else() + message(FATAL_ERROR "'nmake -?' reported:\n${nmake_out}") + endif() + if(nmake_version VERSION_LESS 9) + set(Skip_BUILD_MAKE_PROGRAM_Case 1) + endif() +endif() +if(NOT Skip_BUILD_MAKE_PROGRAM_Case) instrument(cmake-command-make-program NO_WARN BUILD_MAKE_PROGRAM CHECK_SCRIPT check-make-program-hooks.cmake)