Merge topic 'target-level-dependencies-via-byproducts'

482d858500 Depend: Add test for target-level dependencies via byproducts
f6574c9a81 Depend: Hook up automatic target-level dependencies via byproducts
2edb0b71ed cmMakefile: Add lookup from source name to targets via byproducts
62d5932389 Refatoring: Extract AnyOutputMatches utility

Acked-by: Kitware Robot <kwrobot@kitware.com>
Merge-request: !3806

Help/command/add_custom_command.rst

@@ -68,9 +68,6 @@ The options are:
   order-only dependencies to ensure the byproducts will be
   available before their dependents build.
 
-  The ``BYPRODUCTS`` option is ignored on non-Ninja generators
-  except to mark byproducts ``GENERATED``.
-
 ``COMMAND``
   Specify the command-line(s) to execute at build time.
   If more than one ``COMMAND`` is specified they will be executed in order,
@@ -111,6 +108,9 @@ The options are:
   an ``OUTPUT`` of another custom command in the same directory
   (``CMakeLists.txt`` file) CMake automatically brings the other
   custom command into the target in which this command is built.
+  A target-level dependency is added if any dependency is listed as
+  ``BYPRODUCTS`` of a target or any of its build events in the same
+  directory to ensure the byproducts will be available.
   If ``DEPENDS`` is not specified the command will run whenever
   the ``OUTPUT`` is missing; if the command does not actually
   create the ``OUTPUT`` then the rule will always run.

Help/command/add_custom_target.rst

@@ -49,9 +49,6 @@ The options are:
   order-only dependencies to ensure the byproducts will be
   available before their dependents build.
 
-  The ``BYPRODUCTS`` option is ignored on non-Ninja generators
-  except to mark byproducts ``GENERATED``.
-
 ``COMMAND``
   Specify the command-line(s) to execute at build time.
   If more than one ``COMMAND`` is specified they will be executed in order,
@@ -86,6 +83,9 @@ The options are:
   :command:`add_custom_command` command calls in the same directory
   (``CMakeLists.txt`` file).  They will be brought up to date when
   the target is built.
+  A target-level dependency is added if any dependency is a byproduct
+  of a target or any of its build events in the same directory to ensure
+  the byproducts will be available before this target is built.
 
   Use the :command:`add_dependencies` command to add dependencies
   on other targets.

Source/cmGeneratorTarget.cxx

@@ -2599,7 +2599,7 @@ private:
   SourceEntry* CurrentEntry;
   std::queue<cmSourceFile*> SourceQueue;
   std::set<cmSourceFile*> SourcesQueued;
-  using NameMapType = std::map<std::string, cmSourceFile*>;
+  using NameMapType = std::map<std::string, cmSourcesWithOutput>;
   NameMapType NameMap;
   std::vector<std::string> NewSources;
 
@@ -2705,19 +2705,30 @@ void cmTargetTraceDependencies::QueueSource(cmSourceFile* sf)
 
 void cmTargetTraceDependencies::FollowName(std::string const& name)
 {
-  auto i = this->NameMap.find(name);
-  if (i == this->NameMap.end()) {
+  // Use lower bound with key comparison to not repeat the search for the
+  // insert position if the name could not be found (which is the common case).
+  auto i = this->NameMap.lower_bound(name);
+  if (i == this->NameMap.end() || i->first != name) {
     // Check if we know how to generate this file.
-    cmSourceFile* sf = this->Makefile->GetSourceFileWithOutput(name);
-    NameMapType::value_type entry(name, sf);
-    i = this->NameMap.insert(entry).first;
+    cmSourcesWithOutput sources = this->Makefile->GetSourcesWithOutput(name);
+    i = this->NameMap.emplace_hint(i, name, sources);
   }
-  if (cmSourceFile* sf = i->second) {
-    // Record the dependency we just followed.
-    if (this->CurrentEntry) {
-      this->CurrentEntry->Depends.push_back(sf);
+  if (cmTarget* t = i->second.Target) {
+    // The name is a byproduct of a utility target or a PRE_BUILD, PRE_LINK, or
+    // POST_BUILD command.
+    this->GeneratorTarget->Target->AddUtility(t->GetName());
+  }
+  if (cmSourceFile* sf = i->second.Source) {
+    // For now only follow the dependency if the source file is not a
+    // byproduct.  Semantics of byproducts in a non-Ninja context will have to
+    // be defined first.
+    if (!i->second.SourceIsByproduct) {
+      // Record the dependency we just followed.
+      if (this->CurrentEntry) {
+        this->CurrentEntry->Depends.push_back(sf);
+      }
+      this->QueueSource(sf);
     }
-    this->QueueSource(sf);
   }
 }
 

Source/cmMakefile.cxx

@@ -908,6 +908,37 @@ void cmMakefile::AddCustomCommandToTarget(
       t.AddPostBuildCommand(cc);
       break;
   }
+  this->UpdateOutputToSourceMap(byproducts, &t);
+}
+
+void cmMakefile::UpdateOutputToSourceMap(
+  std::vector<std::string> const& byproducts, cmTarget* target)
+{
+  for (std::string const& o : byproducts) {
+    this->UpdateOutputToSourceMap(o, target);
+  }
+}
+
+void cmMakefile::UpdateOutputToSourceMap(std::string const& byproduct,
+                                         cmTarget* target)
+{
+  SourceEntry entry;
+  entry.Sources.Target = target;
+
+  auto pr = this->OutputToSource.emplace(byproduct, entry);
+  if (!pr.second) {
+    SourceEntry& current = pr.first->second;
+    // Has the target already been set?
+    if (!current.Sources.Target) {
+      current.Sources.Target = target;
+    } else {
+      // Multiple custom commands/targets produce the same output (source file
+      // or target).  See also comment in other UpdateOutputToSourceMap
+      // overload.
+      //
+      // TODO: Warn the user about this case.
+    }
+  }
 }
 
 cmSourceFile* cmMakefile::AddCustomCommandToOutput(
@@ -1003,35 +1034,47 @@ cmSourceFile* cmMakefile::AddCustomCommandToOutput(
     cc->SetDepfile(depfile);
     cc->SetJobPool(job_pool);
     file->SetCustomCommand(cc);
-    this->UpdateOutputToSourceMap(outputs, file);
+    this->UpdateOutputToSourceMap(outputs, file, false);
+    this->UpdateOutputToSourceMap(byproducts, file, true);
   }
   return file;
 }
 
 void cmMakefile::UpdateOutputToSourceMap(
-  std::vector<std::string> const& outputs, cmSourceFile* source)
+  std::vector<std::string> const& outputs, cmSourceFile* source,
+  bool byproduct)
 {
   for (std::string const& o : outputs) {
-    this->UpdateOutputToSourceMap(o, source);
+    this->UpdateOutputToSourceMap(o, source, byproduct);
   }
 }
 
 void cmMakefile::UpdateOutputToSourceMap(std::string const& output,
-                                         cmSourceFile* source)
+                                         cmSourceFile* source, bool byproduct)
 {
-  auto i = this->OutputToSource.find(output);
-  if (i != this->OutputToSource.end()) {
-    // Multiple custom commands produce the same output but may
-    // be attached to a different source file (MAIN_DEPENDENCY).
-    // LinearGetSourceFileWithOutput would return the first one,
-    // so keep the mapping for the first one.
-    //
-    // TODO: Warn the user about this case.  However, the VS 8 generator
-    // triggers it for separate generate.stamp rules in ZERO_CHECK and
-    // individual targets.
-    return;
+  SourceEntry entry;
+  entry.Sources.Source = source;
+  entry.Sources.SourceIsByproduct = byproduct;
+
+  auto pr = this->OutputToSource.emplace(output, entry);
+  if (!pr.second) {
+    SourceEntry& current = pr.first->second;
+    // Outputs take precedence over byproducts
+    if (!current.Sources.Source ||
+        (current.Sources.SourceIsByproduct && !byproduct)) {
+      current.Sources.Source = source;
+      current.Sources.SourceIsByproduct = false;
+    } else {
+      // Multiple custom commands produce the same output but may
+      // be attached to a different source file (MAIN_DEPENDENCY).
+      // LinearGetSourceFileWithOutput would return the first one,
+      // so keep the mapping for the first one.
+      //
+      // TODO: Warn the user about this case.  However, the VS 8 generator
+      // triggers it for separate generate.stamp rules in ZERO_CHECK and
+      // individual targets.
+    }
   }
-  this->OutputToSource[output] = source;
 }
 
 cmSourceFile* cmMakefile::AddCustomCommandToOutput(
@@ -1194,6 +1237,8 @@ cmTarget* cmMakefile::AddUtilityCommand(
     } else {
       cmSystemTools::Error("Could not get source file entry for " + force);
     }
+
+    this->UpdateOutputToSourceMap(byproducts, target);
   }
   return target;
 }
@@ -2007,52 +2052,126 @@ cmTarget* cmMakefile::AddNewTarget(cmStateEnums::TargetType type,
     this->Targets
       .emplace(name, cmTarget(name, type, cmTarget::VisibilityNormal, this))
       .first;
+  this->OrderedTargets.push_back(&it->second);
   this->GetGlobalGenerator()->IndexTarget(&it->second);
   this->GetStateSnapshot().GetDirectory().AddNormalTargetName(name);
   return &it->second;
 }
 
-cmSourceFile* cmMakefile::LinearGetSourceFileWithOutput(
-  const std::string& name) const
+namespace {
+bool AnyOutputMatches(const std::string& name,
+                      const std::vector<std::string>& outputs)
 {
-  std::string out;
+  for (std::string const& output : outputs) {
+    std::string::size_type pos = output.rfind(name);
+    // If the output matches exactly
+    if (pos != std::string::npos && pos == output.size() - name.size() &&
+        (pos == 0 || output[pos - 1] == '/')) {
+      return true;
+    }
+  }
+  return false;
+}
 
-  // look through all the source files that have custom commands
-  // and see if the custom command has the passed source file as an output
+bool AnyTargetCommandOutputMatches(
+  const std::string& name, const std::vector<cmCustomCommand>& commands)
+{
+  for (cmCustomCommand const& command : commands) {
+    if (AnyOutputMatches(name, command.GetByproducts())) {
+      return true;
+    }
+  }
+  return false;
+}
+}
+
+cmTarget* cmMakefile::LinearGetTargetWithOutput(const std::string& name) const
+{
+  // We go through the ordered vector of targets to get reproducible results
+  // should multiple names match.
+  for (cmTarget* t : this->OrderedTargets) {
+    // Does the output of any command match the source file name?
+    if (AnyTargetCommandOutputMatches(name, t->GetPreBuildCommands())) {
+      return t;
+    }
+    if (AnyTargetCommandOutputMatches(name, t->GetPreLinkCommands())) {
+      return t;
+    }
+    if (AnyTargetCommandOutputMatches(name, t->GetPostBuildCommands())) {
+      return t;
+    }
+  }
+  return nullptr;
+}
+
+cmSourceFile* cmMakefile::LinearGetSourceFileWithOutput(
+  const std::string& name, cmSourceOutputKind kind, bool& byproduct) const
+{
+  // Outputs take precedence over byproducts.
+  byproduct = false;
+  cmSourceFile* fallback = nullptr;
+
+  // Look through all the source files that have custom commands and see if the
+  // custom command has the passed source file as an output.
   for (cmSourceFile* src : this->SourceFiles) {
-    // does this source file have a custom command?
+    // Does this source file have a custom command?
     if (src->GetCustomCommand()) {
       // Does the output of the custom command match the source file name?
-      const std::vector<std::string>& outputs =
-        src->GetCustomCommand()->GetOutputs();
-      for (std::string const& output : outputs) {
-        out = output;
-        std::string::size_type pos = out.rfind(name);
-        // If the output matches exactly
-        if (pos != std::string::npos && pos == out.size() - name.size() &&
-            (pos == 0 || out[pos - 1] == '/')) {
-          return src;
+      if (AnyOutputMatches(name, src->GetCustomCommand()->GetOutputs())) {
+        // Return the first matching output.
+        return src;
+      }
+      if (kind == cmSourceOutputKind::OutputOrByproduct) {
+        if (AnyOutputMatches(name, src->GetCustomCommand()->GetByproducts())) {
+          // Do not return the source yet as there might be a matching output.
+          fallback = src;
         }
       }
     }
   }
 
-  // otherwise return NULL
-  return nullptr;
+  // Did we find a byproduct?
+  byproduct = fallback != nullptr;
+  return fallback;
 }
 
-cmSourceFile* cmMakefile::GetSourceFileWithOutput(
+cmSourcesWithOutput cmMakefile::GetSourcesWithOutput(
   const std::string& name) const
 {
-  // If the queried path is not absolute we use the backward compatible
-  // linear-time search for an output with a matching suffix.
+  // Linear search?  Also see GetSourceFileWithOutput for detail.
   if (!cmSystemTools::FileIsFullPath(name)) {
-    return this->LinearGetSourceFileWithOutput(name);
+    cmSourcesWithOutput sources;
+    sources.Target = this->LinearGetTargetWithOutput(name);
+    sources.Source = this->LinearGetSourceFileWithOutput(
+      name, cmSourceOutputKind::OutputOrByproduct, sources.SourceIsByproduct);
+    return sources;
   }
   // Otherwise we use an efficient lookup map.
   auto o = this->OutputToSource.find(name);
   if (o != this->OutputToSource.end()) {
-    return (*o).second;
+    return o->second.Sources;
+  }
+  return {};
+}
+
+cmSourceFile* cmMakefile::GetSourceFileWithOutput(
+  const std::string& name, cmSourceOutputKind kind) const
+{
+  // If the queried path is not absolute we use the backward compatible
+  // linear-time search for an output with a matching suffix.
+  if (!cmSystemTools::FileIsFullPath(name)) {
+    bool byproduct = false;
+    return this->LinearGetSourceFileWithOutput(name, kind, byproduct);
+  }
+  // Otherwise we use an efficient lookup map.
+  auto o = this->OutputToSource.find(name);
+  if (o != this->OutputToSource.end() &&
+      (!o->second.Sources.SourceIsByproduct ||
+       kind == cmSourceOutputKind::OutputOrByproduct)) {
+    // Source file could also be null pointer for example if we found the
+    // byproduct of a utility target or a PRE_BUILD, PRE_LINK, or POST_BUILD
+    // command of a target.
+    return o->second.Sources.Source;
   }
   return nullptr;
 }

Source/cmMakefile.h

@@ -51,6 +51,24 @@ class cmTestGenerator;
 class cmVariableWatch;
 class cmake;
 
+/** Flag if byproducts shall also be considered.  */
+enum class cmSourceOutputKind
+{
+  OutputOnly,
+  OutputOrByproduct
+};
+
+/** Target and source file which have a specific output.  */
+struct cmSourcesWithOutput
+{
+  /** Target with byproduct.  */
+  cmTarget* Target = nullptr;
+
+  /** Source file with output or byproduct.  */
+  cmSourceFile* Source = nullptr;
+  bool SourceIsByproduct = false;
+};
+
 /** A type-safe wrapper for a string representing a directory id.  */
 class cmDirectoryId
 {
@@ -687,10 +705,19 @@ public:
   }
 
   /**
-   * Is there a source file that has the provided source file as an output?
-   * if so then return it
+   * Return the target if the provided source name is a byproduct of a utility
+   * target or a PRE_BUILD, PRE_LINK, or POST_BUILD command.
+   * Return the source file which has the provided source name as output.
    */
-  cmSourceFile* GetSourceFileWithOutput(const std::string& outName) const;
+  cmSourcesWithOutput GetSourcesWithOutput(const std::string& name) const;
+
+  /**
+   * Is there a source file that has the provided source name as an output?
+   * If so then return it.
+   */
+  cmSourceFile* GetSourceFileWithOutput(
+    const std::string& name,
+    cmSourceOutputKind kind = cmSourceOutputKind::OutputOnly) const;
 
   //! Add a new cmTest to the list of tests for this makefile.
   cmTest* CreateTest(const std::string& testName);
@@ -914,6 +941,9 @@ protected:
   mutable cmTargetMap Targets;
   std::map<std::string, std::string> AliasTargets;
 
+  using TargetsVec = std::vector<cmTarget*>;
+  TargetsVec OrderedTargets;
+
   using SourceFileVec = std::vector<cmSourceFile*>;
   SourceFileVec SourceFiles;
 
@@ -1036,21 +1066,37 @@ private:
     cmSourceFileLocationKind kind = cmSourceFileLocationKind::Ambiguous);
 
   /**
-   * Old version of GetSourceFileWithOutput(const std::string&) kept for
-   * backward-compatibility. It implements a linear search and support
-   * relative file paths. It is used as a fall back by
-   * GetSourceFileWithOutput(const std::string&).
+   * See LinearGetSourceFileWithOutput for background information
    */
-  cmSourceFile* LinearGetSourceFileWithOutput(const std::string& cname) const;
+  cmTarget* LinearGetTargetWithOutput(const std::string& name) const;
+
+  /**
+   * Generalized old version of GetSourceFileWithOutput kept for
+   * backward-compatibility. It implements a linear search and supports
+   * relative file paths. It is used as a fall back by GetSourceFileWithOutput
+   * and GetSourcesWithOutput.
+   */
+  cmSourceFile* LinearGetSourceFileWithOutput(const std::string& name,
+                                              cmSourceOutputKind kind,
+                                              bool& byproduct) const;
+
+  struct SourceEntry
+  {
+    cmSourcesWithOutput Sources;
+  };
 
   // A map for fast output to input look up.
-  using OutputToSourceMap = std::unordered_map<std::string, cmSourceFile*>;
+  using OutputToSourceMap = std::unordered_map<std::string, SourceEntry>;
   OutputToSourceMap OutputToSource;
 
+  void UpdateOutputToSourceMap(std::vector<std::string> const& byproducts,
+                               cmTarget* target);
+  void UpdateOutputToSourceMap(std::string const& byproduct, cmTarget* target);
+
   void UpdateOutputToSourceMap(std::vector<std::string> const& outputs,
-                               cmSourceFile* source);
-  void UpdateOutputToSourceMap(std::string const& output,
-                               cmSourceFile* source);
+                               cmSourceFile* source, bool byproduct);
+  void UpdateOutputToSourceMap(std::string const& output, cmSourceFile* source,
+                               bool byproduct);
 
   bool AddRequiredTargetCFeature(cmTarget* target, const std::string& feature,
                                  std::string* error = nullptr) const;

Tests/CustomCommandByproducts/CMakeLists.txt

@@ -28,6 +28,7 @@ add_custom_target(Producer3_4
   COMMAND ${CMAKE_COMMAND} -E copy_if_different
     ${CMAKE_CURRENT_SOURCE_DIR}/byproduct3.c.in byproduct3.c
   BYPRODUCTS byproduct3.c
+  DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/byproduct3.c.in
   )
 
 # Generate a byproduct in a custom target POST_BUILD command.
@@ -36,32 +37,36 @@ add_custom_command(
   COMMAND ${CMAKE_COMMAND} -E copy_if_different
     ${CMAKE_CURRENT_SOURCE_DIR}/byproduct4.c.in byproduct4.c
   BYPRODUCTS byproduct4.c
+  DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/byproduct4.c.in
   )
 
-add_executable(ProducerExe ProducerExe.c)
+add_executable(ProducerExe5_6_7 ProducerExe.c)
 
 # Generate a byproduct in an executable POST_BUILD command.
 add_custom_command(
-  TARGET ProducerExe POST_BUILD
+  TARGET ProducerExe5_6_7 POST_BUILD
   COMMAND ${CMAKE_COMMAND} -E copy_if_different
     ${CMAKE_CURRENT_SOURCE_DIR}/byproduct5.c.in byproduct5.c
   BYPRODUCTS byproduct5.c
+  DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/byproduct5.c.in
   )
 
 # Generate a byproduct in an executable PRE_LINK command.
 add_custom_command(
-  TARGET ProducerExe PRE_LINK
+  TARGET ProducerExe5_6_7 PRE_LINK
   COMMAND ${CMAKE_COMMAND} -E copy_if_different
     ${CMAKE_CURRENT_SOURCE_DIR}/byproduct6.c.in byproduct6.c
   BYPRODUCTS byproduct6.c
+  DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/byproduct6.c.in
   )
 
 # Generate a byproduct in an executable PRE_BUILD command.
 add_custom_command(
-  TARGET ProducerExe PRE_BUILD
+  TARGET ProducerExe5_6_7 PRE_BUILD
   COMMAND ${CMAKE_COMMAND} -E copy_if_different
     ${CMAKE_CURRENT_SOURCE_DIR}/byproduct7.c.in byproduct7.c
   BYPRODUCTS byproduct7.c
+  DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/byproduct7.c.in
   )
 
 # Generate a byproduct in a custom command that consumes other byproducts.
@@ -80,6 +85,25 @@ add_custom_command(OUTPUT timestamp8.txt
     ${CMAKE_CURRENT_SOURCE_DIR}/byproduct8.c.in
   )
 
+add_executable(ProducerExe9 ProducerExe.c)
+
+# Generate a byproduct in a custom target which depends on a byproduct of a
+# POST_BUILD command (test if dependency of custom target Producer9 to
+# ProducerExe9 is added).
+add_custom_command(
+  TARGET ProducerExe9 POST_BUILD
+  COMMAND ${CMAKE_COMMAND} -E copy_if_different
+    ${CMAKE_CURRENT_SOURCE_DIR}/byproduct9.c.in byproduct9a.c
+  BYPRODUCTS byproduct9a.c
+  DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/byproduct9.c.in
+  )
+add_custom_target(Producer9
+  COMMAND ${CMAKE_COMMAND} -E copy_if_different
+    byproduct9a.c byproduct9.c
+  BYPRODUCTS byproduct9.c
+  DEPENDS byproduct9a.c
+  )
+
 # Generate the library file of an imported target as a byproduct
 # of an external project.
 get_property(_isMultiConfig GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG)
@@ -136,10 +160,13 @@ add_executable(CustomCommandByproducts
   byproduct6.c
   byproduct7.c
   byproduct8.c timestamp8.txt
+  byproduct9.c
   )
+
+# Dependencies to byproducts of custom commands other than build events are not
+# yet traced (see issue #19005).
 add_dependencies(CustomCommandByproducts Producer2)
-add_dependencies(CustomCommandByproducts Producer3_4)
-add_dependencies(CustomCommandByproducts ProducerExe)
+
 target_link_libraries(CustomCommandByproducts ExternalLibrary)
 
 if(CMAKE_GENERATOR STREQUAL "Ninja")

Tests/CustomCommandByproducts/CustomCommandByproducts.c

@@ -6,10 +6,11 @@ extern int byproduct5(void);
 extern int byproduct6(void);
 extern int byproduct7(void);
 extern int byproduct8(void);
+extern int byproduct9(void);
 extern int ExternalLibrary(void);
 int main(void)
 {
   return (byproduct1() + byproduct2() + byproduct3() + byproduct4() +
           byproduct5() + byproduct6() + byproduct7() + byproduct8() +
-          ExternalLibrary() + 0);
+          byproduct9() + ExternalLibrary() + 0);
 }

Tests/CustomCommandByproducts/byproduct9.c.in

@@ -0,0 +1 @@
+int byproduct9(void) { return 0; }