Merge topic 'custom-command-cleanup'

3e36d5e846 cmGeneratorTarget: Refactor custom command dependency evaluation
c404f64289 cmCustomCommandGenerator: Collect genex target references in commands
2a640d4199 cmCustomCommandGenerator: Add move operations
fab772c3e1 cmAddCustomCommandCommand: Drop outdated comment
84fecbf214 cmLocalNinjaGenerator: Remove leftover local debugging comment
9e5e2d704a Remove unnecessary arbitrary CollapseFullPath second arguments

Acked-by: Kitware Robot <[email protected]>
Merge-request: !5435

Source/cmAddCustomCommandCommand.cxx

@@ -190,15 +190,7 @@ bool cmAddCustomCommandCommand(std::vector<std::string> const& args,
         case doing_byproducts:
           if (!cmSystemTools::FileIsFullPath(copy)) {
             // This is an output to be generated, so it should be
-            // under the build tree.  CMake 2.4 placed this under the
-            // source tree.  However the only case that this change
-            // will break is when someone writes
-            //
-            //   add_custom_command(OUTPUT out.txt ...)
-            //
-            // and later references "${CMAKE_CURRENT_SOURCE_DIR}/out.txt".
-            // This is fairly obscure so we can wait for someone to
-            // complain.
+            // under the build tree.
             filename = cmStrCat(mf.GetCurrentBinaryDirectory(), '/');
           }
           filename += copy;
@@ -215,8 +207,7 @@ bool cmAddCustomCommandCommand(std::vector<std::string> const& args,
       }
 
       if (cmSystemTools::FileIsFullPath(filename)) {
-        filename = cmSystemTools::CollapseFullPath(
-          filename, status.GetMakefile().GetHomeOutputDirectory());
+        filename = cmSystemTools::CollapseFullPath(filename);
       }
       switch (doing) {
         case doing_depfile:

Source/cmCustomCommandGenerator.cxx

@@ -35,8 +35,7 @@ void AppendPaths(const std::vector<std::string>& inputs,
     for (std::string& it : result) {
       cmSystemTools::ConvertToUnixSlashes(it);
       if (cmSystemTools::FileIsFullPath(it)) {
-        it = cmSystemTools::CollapseFullPath(
-          it, lg->GetMakefile()->GetHomeOutputDirectory());
+        it = cmSystemTools::CollapseFullPath(it);
       }
     }
     cm::append(output, result);
@@ -48,7 +47,7 @@ cmCustomCommandGenerator::cmCustomCommandGenerator(cmCustomCommand const& cc,
                                                    std::string config,
                                                    cmLocalGenerator* lg,
                                                    bool transformDepfile)
-  : CC(cc)
+  : CC(&cc)
   , Config(std::move(config))
   , LG(lg)
   , OldStyle(cc.GetEscapeOldStyle())
@@ -57,23 +56,35 @@ cmCustomCommandGenerator::cmCustomCommandGenerator(cmCustomCommand const& cc,
 {
   cmGeneratorExpression ge(cc.GetBacktrace());
 
-  const cmCustomCommandLines& cmdlines = this->CC.GetCommandLines();
+  const cmCustomCommandLines& cmdlines = this->CC->GetCommandLines();
   for (cmCustomCommandLine const& cmdline : cmdlines) {
     cmCustomCommandLine argv;
     for (std::string const& clarg : cmdline) {
       std::unique_ptr<cmCompiledGeneratorExpression> cge = ge.Parse(clarg);
       std::string parsed_arg = cge->Evaluate(this->LG, this->Config);
-      if (this->CC.GetCommandExpandLists()) {
+      for (cmGeneratorTarget* gt : cge->GetTargets()) {
+        this->Utilities.emplace(BT<std::pair<std::string, bool>>(
+          { gt->GetName(), true }, cge->GetBacktrace()));
+      }
+      if (this->CC->GetCommandExpandLists()) {
         cm::append(argv, cmExpandedList(parsed_arg));
       } else {
         argv.push_back(std::move(parsed_arg));
       }
     }
 
-    // Later code assumes at least one entry exists, but expanding
-    // lists on an empty command may have left this empty.
-    // FIXME: Should we define behavior for removing empty commands?
-    if (argv.empty()) {
+    if (!argv.empty()) {
+      // If the command references an executable target by name,
+      // collect the target to add a target-level dependency on it.
+      cmGeneratorTarget* gt = this->LG->FindGeneratorTargetToUse(argv.front());
+      if (gt && gt->GetType() == cmStateEnums::EXECUTABLE) {
+        this->Utilities.emplace(BT<std::pair<std::string, bool>>(
+          { gt->GetName(), true }, cc.GetBacktrace()));
+      }
+    } else {
+      // Later code assumes at least one entry exists, but expanding
+      // lists on an empty command may have left this empty.
+      // FIXME: Should we define behavior for removing empty commands?
       argv.emplace_back();
     }
 
@@ -114,7 +125,7 @@ cmCustomCommandGenerator::cmCustomCommandGenerator(cmCustomCommand const& cc,
               this->Byproducts);
   AppendPaths(cc.GetDepends(), ge, this->LG, this->Config, this->Depends);
 
-  const std::string& workingdirectory = this->CC.GetWorkingDirectory();
+  const std::string& workingdirectory = this->CC->GetWorkingDirectory();
   if (!workingdirectory.empty()) {
     std::unique_ptr<cmCompiledGeneratorExpression> cge =
       ge.Parse(workingdirectory);
@@ -271,7 +282,7 @@ void cmCustomCommandGenerator::AppendArguments(unsigned int c,
 
 std::string cmCustomCommandGenerator::GetFullDepfile() const
 {
-  std::string depfile = this->CC.GetDepfile();
+  std::string depfile = this->CC->GetDepfile();
   if (depfile.empty()) {
     return "";
   }
@@ -305,7 +316,7 @@ std::string cmCustomCommandGenerator::GetInternalDepfile() const
 
 const char* cmCustomCommandGenerator::GetComment() const
 {
-  return this->CC.GetComment();
+  return this->CC->GetComment();
 }
 
 std::string cmCustomCommandGenerator::GetWorkingDirectory() const
@@ -315,7 +326,7 @@ std::string cmCustomCommandGenerator::GetWorkingDirectory() const
 
 std::vector<std::string> const& cmCustomCommandGenerator::GetOutputs() const
 {
-  return this->CC.GetOutputs();
+  return this->CC->GetOutputs();
 }
 
 std::vector<std::string> const& cmCustomCommandGenerator::GetByproducts() const
@@ -327,3 +338,9 @@ std::vector<std::string> const& cmCustomCommandGenerator::GetDepends() const
 {
   return this->Depends;
 }
+
+std::set<BT<std::pair<std::string, bool>>> const&
+cmCustomCommandGenerator::GetUtilities() const
+{
+  return this->Utilities;
+}

Source/cmCustomCommandGenerator.h

@@ -4,17 +4,20 @@
 
 #include "cmConfigure.h" // IWYU pragma: keep
 
+#include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "cmCustomCommandLines.h"
+#include "cmListFileCache.h"
 
 class cmCustomCommand;
 class cmLocalGenerator;
 
 class cmCustomCommandGenerator
 {
-  cmCustomCommand const& CC;
+  cmCustomCommand const* CC;
   std::string Config;
   cmLocalGenerator* LG;
   bool OldStyle;
@@ -24,6 +27,7 @@ class cmCustomCommandGenerator
   std::vector<std::string> Byproducts;
   std::vector<std::string> Depends;
   std::string WorkingDirectory;
+  std::set<BT<std::pair<std::string, bool>>> Utilities;
 
   void FillEmulatorsWithArguments();
   std::vector<std::string> GetCrossCompilingEmulator(unsigned int c) const;
@@ -33,9 +37,11 @@ public:
   cmCustomCommandGenerator(cmCustomCommand const& cc, std::string config,
                            cmLocalGenerator* lg, bool transformDepfile = true);
   cmCustomCommandGenerator(const cmCustomCommandGenerator&) = delete;
+  cmCustomCommandGenerator(cmCustomCommandGenerator&&) = default;
   cmCustomCommandGenerator& operator=(const cmCustomCommandGenerator&) =
     delete;
-  cmCustomCommand const& GetCC() const { return this->CC; }
+  cmCustomCommandGenerator& operator=(cmCustomCommandGenerator&&) = default;
+  cmCustomCommand const& GetCC() const { return *(this->CC); }
   unsigned int GetNumberOfCommands() const;
   std::string GetCommand(unsigned int c) const;
   void AppendArguments(unsigned int c, std::string& cmd) const;
@@ -44,6 +50,7 @@ public:
   std::vector<std::string> const& GetOutputs() const;
   std::vector<std::string> const& GetByproducts() const;
   std::vector<std::string> const& GetDepends() const;
+  std::set<BT<std::pair<std::string, bool>>> const& GetUtilities() const;
   bool HasOnlyEmptyCommandLines() const;
   std::string GetFullDepfile() const;
   std::string GetInternalDepfile() const;

Source/cmGeneratorTarget.cxx

@@ -24,9 +24,7 @@
 
 #include "cmAlgorithms.h"
 #include "cmComputeLinkInformation.h"
-#include "cmCustomCommand.h"
 #include "cmCustomCommandGenerator.h"
-#include "cmCustomCommandLines.h"
 #include "cmFileTimes.h"
 #include "cmGeneratedFileStream.h"
 #include "cmGeneratorExpression.h"
@@ -2892,9 +2890,6 @@ private:
   bool IsUtility(std::string const& dep);
   void CheckCustomCommand(cmCustomCommand const& cc);
   void CheckCustomCommands(const std::vector<cmCustomCommand>& commands);
-  void FollowCommandDepends(cmCustomCommand const& cc,
-                            const std::string& config,
-                            std::set<std::string>& emitted);
 };
 
 cmTargetTraceDependencies::cmTargetTraceDependencies(cmGeneratorTarget* target)
@@ -3081,71 +3076,27 @@ bool cmTargetTraceDependencies::IsUtility(std::string const& dep)
 
 void cmTargetTraceDependencies::CheckCustomCommand(cmCustomCommand const& cc)
 {
-  // Transform command names that reference targets built in this
-  // project to corresponding target-level dependencies.
-  cmGeneratorExpression ge(cc.GetBacktrace());
-
-  // Add target-level dependencies referenced by generator expressions.
-  std::set<cmGeneratorTarget*> targets;
-
-  for (cmCustomCommandLine const& cCmdLine : cc.GetCommandLines()) {
-    std::string const& command = cCmdLine.front();
-    // Check for a target with this name.
-    if (cmGeneratorTarget* t =
-          this->LocalGenerator->FindGeneratorTargetToUse(command)) {
-      if (t->GetType() == cmStateEnums::EXECUTABLE) {
-        // The command refers to an executable target built in
-        // this project.  Add the target-level dependency to make
-        // sure the executable is up to date before this custom
-        // command possibly runs.
-        this->GeneratorTarget->Target->AddUtility(command, true);
-      }
-    }
+  // Collect dependencies referenced by all configurations.
+  std::set<std::string> depends;
+  for (std::string const& config :
+       this->Makefile->GetGeneratorConfigs(cmMakefile::IncludeEmptyConfig)) {
+    cmCustomCommandGenerator ccg(cc, config, this->LocalGenerator);
 
-    // Check for target references in generator expressions.
-    std::vector<std::string> const& configs =
-      this->Makefile->GetGeneratorConfigs(cmMakefile::IncludeEmptyConfig);
-    for (std::string const& c : configs) {
-      for (std::string const& cl : cCmdLine) {
-        const std::unique_ptr<cmCompiledGeneratorExpression> cge =
-          ge.Parse(cl);
-        cge->SetQuiet(true);
-        cge->Evaluate(this->GeneratorTarget->GetLocalGenerator(), c);
-        std::set<cmGeneratorTarget*> geTargets = cge->GetTargets();
-        targets.insert(geTargets.begin(), geTargets.end());
-      }
+    // Collect target-level dependencies referenced in command lines.
+    for (auto const& util : ccg.GetUtilities()) {
+      this->GeneratorTarget->Target->AddUtility(util);
     }
-  }
 
-  for (cmGeneratorTarget* target : targets) {
-    this->GeneratorTarget->Target->AddUtility(target->GetName(), true);
+    // Collect file-level dependencies referenced in DEPENDS.
+    depends.insert(ccg.GetDepends().begin(), ccg.GetDepends().end());
   }
 
-  // Queue the custom command dependencies.
-  std::set<std::string> emitted;
-  std::vector<std::string> const& configs =
-    this->Makefile->GetGeneratorConfigs(cmMakefile::IncludeEmptyConfig);
-  for (std::string const& conf : configs) {
-    this->FollowCommandDepends(cc, conf, emitted);
-  }
-}
-
-void cmTargetTraceDependencies::FollowCommandDepends(
-  cmCustomCommand const& cc, const std::string& config,
-  std::set<std::string>& emitted)
-{
-  cmCustomCommandGenerator ccg(cc, config,
-                               this->GeneratorTarget->LocalGenerator);
-
-  const std::vector<std::string>& depends = ccg.GetDepends();
-
+  // Queue file-level dependencies.
   for (std::string const& dep : depends) {
-    if (emitted.insert(dep).second) {
-      if (!this->IsUtility(dep)) {
-        // The dependency does not name a target and may be a file we
-        // know how to generate.  Queue it.
-        this->FollowName(dep);
-      }
+    if (!this->IsUtility(dep)) {
+      // The dependency does not name a target and may be a file we
+      // know how to generate.  Queue it.
+      this->FollowName(dep);
     }
   }
 }

Source/cmLocalNinjaGenerator.cxx

@@ -603,11 +603,6 @@ void cmLocalNinjaGenerator::WriteCustomCommandBuildStatement(
     }
   }
 
-#if 0
-#  error TODO: Once CC in an ExternalProject target must provide the \
-    file of each imported target that has an add_dependencies pointing \
-    at us.  How to know which ExternalProject step actually provides it?
-#endif
   cmNinjaDeps ninjaOutputs(outputs.size() + byproducts.size());
   std::transform(outputs.begin(), outputs.end(), ninjaOutputs.begin(),
                  gg->MapToNinjaPath());

Source/cmSourceFileLocation.cxx

@@ -33,8 +33,7 @@ cmSourceFileLocation::cmSourceFileLocation(cmMakefile const* mf,
   this->AmbiguousExtension = true;
   this->Directory = cmSystemTools::GetFilenamePath(name);
   if (cmSystemTools::FileIsFullPath(this->Directory)) {
-    this->Directory = cmSystemTools::CollapseFullPath(
-      this->Directory, mf->GetHomeOutputDirectory());
+    this->Directory = cmSystemTools::CollapseFullPath(this->Directory);
   }
   this->Name = cmSystemTools::GetFilenameName(name);
   if (kind == cmSourceFileLocationKind::Known) {

Source/cmTarget.cxx

@@ -623,6 +623,11 @@ void cmTarget::AddUtility(std::string const& name, bool cross, cmMakefile* mf)
     { name, cross }, mf ? mf->GetBacktrace() : cmListFileBacktrace()));
 }
 
+void cmTarget::AddUtility(BT<std::pair<std::string, bool>> util)
+{
+  impl->Utilities.emplace(std::move(util));
+}
+
 std::set<BT<std::pair<std::string, bool>>> const& cmTarget::GetUtilities()
   const
 {

Source/cmTarget.h

@@ -164,6 +164,7 @@ public:
    */
   void AddUtility(std::string const& name, bool cross,
                   cmMakefile* mf = nullptr);
+  void AddUtility(BT<std::pair<std::string, bool>> util);
   //! Get the utilities used by this target
   std::set<BT<std::pair<std::string, bool>>> const& GetUtilities() const;