From 2725ecff38ccd25905a9bc968bfb1b2f0d09b34a Mon Sep 17 00:00:00 2001 From: Brad King Date: Wed, 19 May 2021 10:21:11 -0400 Subject: [PATCH] Ninja: Handle depfiles with absolute paths to generated files Ninja treats every (normalized) path as its own node. It does not recognize `/abs/path/to/file` in a depfile as matching `path/to/file` even when `build.ninja` and the working directory are in `/abs/`. See Ninja Issue 1251. In cases where we pass absolute paths to the compiler, it will write a depfile containing absolute paths. If those files are generated in the build tree by custom commands, `build.ninja` references them by relative path in build statement outputs, so Ninja does not hook up the dependency and rebuild the project correctly. Add infrastructure to work around this problem by adding implicit outputs to custom command build statements that reference the main outputs by absolute path. Use a `${cmake_ninja_workdir}` placeholder to avoid repeating the base path. For example: build out.txt | ${cmake_ninja_workdir}out.txt: CUSTOM_COMMAND ... Ninja will create two nodes for the output file, one with a relative path and one with an absolute path. A depfile may then mention either form of the path and Ninja will hook up the dependency. Unfortunately Ninja will also stat the file twice. Issue: #13894 Fixes: #21865 --- Source/cmGlobalNinjaGenerator.cxx | 17 ++++++++++++++++- Source/cmGlobalNinjaGenerator.h | 1 + Source/cmLocalNinjaGenerator.cxx | 13 +++++++++++++ Source/cmLocalNinjaGenerator.h | 1 + Source/cmNinjaTypes.h | 1 + Tests/RunCMake/BuildDepends/RunCMakeTest.cmake | 2 +- 6 files changed, 33 insertions(+), 2 deletions(-) diff --git a/Source/cmGlobalNinjaGenerator.cxx b/Source/cmGlobalNinjaGenerator.cxx index a0baf3ffc5..6034434e2c 100644 --- a/Source/cmGlobalNinjaGenerator.cxx +++ b/Source/cmGlobalNinjaGenerator.cxx @@ -222,7 +222,7 @@ void cmGlobalNinjaGenerator::WriteBuild(std::ostream& os, } } // Write implicit outputs - if (!build.ImplicitOuts.empty()) { + if (!build.ImplicitOuts.empty() || !build.WorkDirOuts.empty()) { buildStr = cmStrCat(buildStr, " |"); for (std::string const& implicitOut : build.ImplicitOuts) { buildStr = cmStrCat(buildStr, ' ', this->EncodePath(implicitOut)); @@ -230,6 +230,14 @@ void cmGlobalNinjaGenerator::WriteBuild(std::ostream& os, this->CombinedBuildOutputs.insert(implicitOut); } } + for (std::string const& workdirOut : build.WorkDirOuts) { + // Repeat some outputs, but expressed as absolute paths. + // This helps Ninja handle absolute paths found in a depfile. + // FIXME: Unfortunately this causes Ninja to stat the file twice. + // We could avoid this if Ninja Issue 1251 were fixed. + buildStr = cmStrCat(buildStr, " ${cmake_ninja_workdir}", + this->EncodePath(workdirOut)); + } } // Write the rule. @@ -314,6 +322,12 @@ void cmGlobalNinjaGenerator::CCOutputs::Add( { for (std::string const& path : paths) { std::string out = this->GG->ConvertToNinjaPath(path); + if (this->GG->SupportsImplicitOuts() && + !cmSystemTools::FileIsFullPath(out)) { + // This output is expressed as a relative path. Repeat it, + // but expressed as an absolute path for Ninja Issue 1251. + this->WorkDirOuts.emplace_back(out); + } this->GG->SeenCustomCommandOutput(out); this->ExplicitOuts.emplace_back(std::move(out)); } @@ -340,6 +354,7 @@ void cmGlobalNinjaGenerator::WriteCustomCommandBuild( cmNinjaBuild build("CUSTOM_COMMAND"); build.Comment = comment; build.Outputs = std::move(outputs.ExplicitOuts); + build.WorkDirOuts = std::move(outputs.WorkDirOuts); build.ExplicitDeps = std::move(explicitDeps); build.OrderOnlyDeps = std::move(orderOnlyDeps); diff --git a/Source/cmGlobalNinjaGenerator.h b/Source/cmGlobalNinjaGenerator.h index 2833367e57..7a3674e76f 100644 --- a/Source/cmGlobalNinjaGenerator.h +++ b/Source/cmGlobalNinjaGenerator.h @@ -121,6 +121,7 @@ public: } void Add(std::vector const& outputs); cmNinjaDeps ExplicitOuts; + cmNinjaDeps WorkDirOuts; }; void WriteCustomCommandBuild(std::string const& command, diff --git a/Source/cmLocalNinjaGenerator.cxx b/Source/cmLocalNinjaGenerator.cxx index 9eb3e46136..fb6c730e6a 100644 --- a/Source/cmLocalNinjaGenerator.cxx +++ b/Source/cmLocalNinjaGenerator.cxx @@ -263,6 +263,7 @@ void cmLocalNinjaGenerator::WriteBuildFileTop() this->GetConfigNames().front()); } this->WriteNinjaFilesInclusionCommon(this->GetCommonFileStream()); + this->WriteNinjaWorkDir(this->GetCommonFileStream()); // For the rule file. this->WriteProjectHeader(this->GetRulesFileStream()); @@ -364,6 +365,17 @@ void cmLocalNinjaGenerator::WriteNinjaFilesInclusionCommon(std::ostream& os) os << "\n"; } +void cmLocalNinjaGenerator::WriteNinjaWorkDir(std::ostream& os) +{ + cmGlobalNinjaGenerator::WriteDivider(os); + cmGlobalNinjaGenerator::WriteComment( + os, "Logical path to working directory; prefix for absolute paths."); + cmGlobalNinjaGenerator* ng = this->GetGlobalNinjaGenerator(); + std::string ninja_workdir = this->GetBinaryDirectory(); + ng->StripNinjaOutputPathPrefixAsSuffix(ninja_workdir); // Also appends '/'. + os << "cmake_ninja_workdir = " << ng->EncodePath(ninja_workdir) << "\n"; +} + void cmLocalNinjaGenerator::WriteProcessedMakefile(std::ostream& os) { cmGlobalNinjaGenerator::WriteDivider(os); @@ -654,6 +666,7 @@ void cmLocalNinjaGenerator::WriteCustomCommandBuildStatement( cmNinjaBuild build("phony"); build.Comment = cmStrCat("Phony custom command for ", mainOutput); build.Outputs = std::move(ccOutputs.ExplicitOuts); + build.WorkDirOuts = std::move(ccOutputs.WorkDirOuts); build.ExplicitDeps = std::move(ninjaDeps); build.OrderOnlyDeps = orderOnlyDeps; gg->WriteBuild(this->GetImplFileStream(fileConfig), build); diff --git a/Source/cmLocalNinjaGenerator.h b/Source/cmLocalNinjaGenerator.h index a73fa275f1..64040374e4 100644 --- a/Source/cmLocalNinjaGenerator.h +++ b/Source/cmLocalNinjaGenerator.h @@ -108,6 +108,7 @@ private: const std::string& config); void WriteNinjaFilesInclusionConfig(std::ostream& os); void WriteNinjaFilesInclusionCommon(std::ostream& os); + void WriteNinjaWorkDir(std::ostream& os); void WriteProcessedMakefile(std::ostream& os); void WritePools(std::ostream& os); diff --git a/Source/cmNinjaTypes.h b/Source/cmNinjaTypes.h index 320f41bb32..c8a411e1df 100644 --- a/Source/cmNinjaTypes.h +++ b/Source/cmNinjaTypes.h @@ -53,6 +53,7 @@ public: std::string Rule; cmNinjaDeps Outputs; cmNinjaDeps ImplicitOuts; + cmNinjaDeps WorkDirOuts; // For cmake_ninja_workdir. cmNinjaDeps ExplicitDeps; cmNinjaDeps ImplicitDeps; cmNinjaDeps OrderOnlyDeps; diff --git a/Tests/RunCMake/BuildDepends/RunCMakeTest.cmake b/Tests/RunCMake/BuildDepends/RunCMakeTest.cmake index 947a2fe2fb..0a8058049b 100644 --- a/Tests/RunCMake/BuildDepends/RunCMakeTest.cmake +++ b/Tests/RunCMake/BuildDepends/RunCMakeTest.cmake @@ -183,7 +183,7 @@ if(RunCMake_GENERATOR MATCHES "Make") endif() if(RunCMake_GENERATOR MATCHES "^Visual Studio 9 " OR - RunCMake_GENERATOR MATCHES "Ninja") + (RunCMake_GENERATOR MATCHES "Ninja" AND ninja_version VERSION_LESS 1.7)) # This build tool misses the dependency. set(run_BuildDepends_skip_step_2 1) endif()