Merge topic 'custom-command-dedup'

45fedf0e17 Makefile: Add policy CMP0113 to avoid duplication of custom commands
844779bdc1 cmMakefileTargetGenerator: Simplify custom command output collection

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

Help/manual/cmake-policies.7.rst

@@ -57,6 +57,7 @@ Policies Introduced by CMake 3.19
 .. toctree::
    :maxdepth: 1
 
+   CMP0113: Makefile generators do not repeat custom commands from target dependencies. </policy/CMP0113>
    CMP0112: Target file component generator expressions do not add target dependencies. </policy/CMP0112>
    CMP0111: An imported target with a missing location fails during generation. </policy/CMP0111>
    CMP0110: add_test() supports arbitrary characters in test names. </policy/CMP0110>

Help/policy/CMP0113.rst

@@ -0,0 +1,43 @@
+CMP0113
+-------
+
+.. versionadded:: 3.19
+
+:ref:`Makefile Generators` do not repeat custom commands from target
+dependencies.
+
+Consider a chain of custom commands split across two dependent targets:
+
+.. code-block:: cmake
+
+  add_custom_command(OUTPUT output-not-created
+    COMMAND ... DEPENDS ...)
+  set_property(SOURCE output-not-created PROPERTY SYMBOLIC 1)
+  add_custom_command(OUTPUT output-created
+    COMMAND ... DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/output-not-created)
+  add_custom_target(first DEPENDS output-not-created)
+  add_custom_target(second DEPENDS output-created)
+  add_dependencies(second first)
+
+In CMake 3.18 and lower, the Makefile generators put a copy of both custom
+commands in the Makefile for target ``second`` even though its dependency on
+target ``first`` ensures that the first custom command runs before the second.
+Running ``make second`` would cause the first custom command to run once in
+the ``first`` target and then again in the ``second`` target.
+
+CMake 3.19 and above prefer to not duplicate custom commands in a target that
+are already generated in other targets on which the target depends (directly or
+indirectly).  This policy provides compatibility for projects that have not
+been updated to expect the new behavior.  In particular, projects that relied
+on the duplicate execution or that did not properly set the :prop_sf:`SYMBOLIC`
+source file property may be affected.
+
+The ``OLD`` behavior for this policy is to duplicate custom commands in
+dependent targets.  The ``NEW`` behavior of this policy is to not duplicate
+custom commands in dependent targets.
+
+This policy was introduced in CMake version 3.19.  Unlike many policies,
+CMake version |release| does *not* warn when this policy is not set and
+simply uses ``OLD`` behavior.
+
+.. include:: DEPRECATED.txt

Help/release/dev/custom-command-dedup.rst

@@ -0,0 +1,5 @@
+custom-command-dedup
+--------------------
+
+* :ref:`Makefile Generators` no longer repeat custom commands from target
+  dependencies.  See policy :policy:`CMP0113`.

Source/cmLocalUnixMakefileGenerator3.cxx

@@ -38,6 +38,7 @@
 #include "cmStateTypes.h"
 #include "cmStringAlgorithms.h"
 #include "cmSystemTools.h"
+#include "cmTargetDepend.h"
 #include "cmVersion.h"
 #include "cmake.h"
 
@@ -105,6 +106,15 @@ void cmLocalUnixMakefileGenerator3::Generate()
     if (!gt->IsInBuildSystem()) {
       continue;
     }
+
+    auto& gtVisited = this->GetCommandsVisited(gt);
+    auto& deps = this->GlobalGenerator->GetTargetDirectDepends(gt);
+    for (auto& d : deps) {
+      // Take the union of visited source files of custom commands
+      auto depVisited = this->GetCommandsVisited(d);
+      gtVisited.insert(depVisited.begin(), depVisited.end());
+    }
+
     std::unique_ptr<cmMakefileTargetGenerator> tg(
       cmMakefileTargetGenerator::New(gt));
     if (tg) {

Source/cmLocalUnixMakefileGenerator3.h

@@ -19,6 +19,7 @@ class cmCustomCommandGenerator;
 class cmGeneratorTarget;
 class cmGlobalGenerator;
 class cmMakefile;
+class cmSourceFile;
 
 /** \class cmLocalUnixMakefileGenerator3
  * \brief Write a LocalUnix makefiles.
@@ -294,4 +295,13 @@ private:
   bool ColorMakefile;
   bool SkipPreprocessedSourceRules;
   bool SkipAssemblySourceRules;
+
+  std::set<cmSourceFile const*>& GetCommandsVisited(
+    cmGeneratorTarget const* target)
+  {
+    return this->CommandsVisited[target];
+  };
+
+  std::map<cmGeneratorTarget const*, std::set<cmSourceFile const*>>
+    CommandsVisited;
 };

Source/cmMakefileTargetGenerator.cxx

@@ -26,10 +26,12 @@
 #include "cmMakefileLibraryTargetGenerator.h"
 #include "cmMakefileUtilityTargetGenerator.h"
 #include "cmOutputConverter.h"
+#include "cmPolicies.h"
 #include "cmProperty.h"
 #include "cmRange.h"
 #include "cmRulePlaceholderExpander.h"
 #include "cmSourceFile.h"
+#include "cmSourceFileLocationKind.h"
 #include "cmState.h"
 #include "cmStateDirectory.h"
 #include "cmStateSnapshot.h"
@@ -51,6 +53,17 @@ cmMakefileTargetGenerator::cmMakefileTargetGenerator(cmGeneratorTarget* target)
   if (cmProp ruleStatus = cm->GetState()->GetGlobalProperty("RULE_MESSAGES")) {
     this->NoRuleMessages = cmIsOff(*ruleStatus);
   }
+  switch (this->GeneratorTarget->GetPolicyStatusCMP0113()) {
+    case cmPolicies::WARN:
+    case cmPolicies::OLD:
+      this->CMP0113New = false;
+      break;
+    case cmPolicies::NEW:
+    case cmPolicies::REQUIRED_IF_USED:
+    case cmPolicies::REQUIRED_ALWAYS:
+      this->CMP0113New = true;
+      break;
+  }
   MacOSXContentGenerator = cm::make_unique<MacOSXContentGeneratorType>(this);
 }
 
@@ -217,6 +230,12 @@ void cmMakefileTargetGenerator::WriteTargetBuildRules()
   this->GeneratorTarget->GetCustomCommands(customCommands,
                                            this->GetConfigName());
   for (cmSourceFile const* sf : customCommands) {
+    if (this->CMP0113New &&
+        !this->LocalGenerator->GetCommandsVisited(this->GeneratorTarget)
+           .insert(sf)
+           .second) {
+      continue;
+    }
     cmCustomCommandGenerator ccg(*sf->GetCustomCommand(),
                                  this->GetConfigName(), this->LocalGenerator);
     this->GenerateCustomRuleFile(ccg);
@@ -1290,16 +1309,7 @@ void cmMakefileTargetGenerator::DriveCustomCommands(
   std::vector<std::string>& depends)
 {
   // Depend on all custom command outputs.
-  std::vector<cmSourceFile*> sources;
-  this->GeneratorTarget->GetSourceFiles(
-    sources, this->Makefile->GetSafeDefinition("CMAKE_BUILD_TYPE"));
-  for (cmSourceFile* source : sources) {
-    if (cmCustomCommand* cc = source->GetCustomCommand()) {
-      cmCustomCommandGenerator ccg(*cc, this->GetConfigName(),
-                                   this->LocalGenerator);
-      cm::append(depends, ccg.GetOutputs());
-    }
-  }
+  cm::append(depends, this->CustomCommandOutputs);
 }
 
 void cmMakefileTargetGenerator::WriteObjectDependRules(
@@ -1346,6 +1356,22 @@ void cmMakefileTargetGenerator::GenerateCustomRuleFile(
   bool symbolic = this->WriteMakeRule(*this->BuildFileStream, nullptr, outputs,
                                       depends, commands);
 
+  // Symbolic inputs are not expected to exist, so add dummy rules.
+  if (this->CMP0113New && !depends.empty()) {
+    std::vector<std::string> no_depends;
+    std::vector<std::string> no_commands;
+    for (std::string const& dep : depends) {
+      if (cmSourceFile* dsf =
+            this->Makefile->GetSource(dep, cmSourceFileLocationKind::Known)) {
+        if (dsf->GetPropertyAsBool("SYMBOLIC")) {
+          this->LocalGenerator->WriteMakeRule(*this->BuildFileStream, nullptr,
+                                              dep, no_depends, no_commands,
+                                              true);
+        }
+      }
+    }
+  }
+
   // If the rule has changed make sure the output is rebuilt.
   if (!symbolic) {
     this->GlobalGenerator->AddRuleHash(ccg.GetOutputs(), content.str());
@@ -1360,6 +1386,8 @@ void cmMakefileTargetGenerator::GenerateCustomRuleFile(
     this->LocalGenerator->AddImplicitDepends(this->GeneratorTarget, idi.first,
                                              objFullPath, srcFullPath);
   }
+
+  this->CustomCommandOutputs.insert(outputs.begin(), outputs.end());
 }
 
 void cmMakefileTargetGenerator::MakeEchoProgress(

Source/cmMakefileTargetGenerator.h

@@ -196,6 +196,8 @@ protected:
   unsigned long NumberOfProgressActions;
   bool NoRuleMessages;
 
+  bool CMP0113New = false;
+
   // the path to the directory the build file is in
   std::string TargetBuildDirectory;
   std::string TargetBuildDirectoryFull;
@@ -228,6 +230,9 @@ protected:
   // Set of extra output files to be driven by the build.
   std::set<std::string> ExtraFiles;
 
+  // Set of custom command output files to be driven by the build.
+  std::set<std::string> CustomCommandOutputs;
+
   using MultipleOutputPairsType = std::map<std::string, std::string>;
   MultipleOutputPairsType MultipleOutputPairs;
   bool WriteMakeRule(std::ostream& os, const char* comment,

Source/cmPolicies.h

@@ -333,6 +333,10 @@ class cmMakefile;
   SELECT(POLICY, CMP0112,                                                     \
          "Target file component generator expressions do not add target "     \
          "dependencies.",                                                     \
+         3, 19, 0, cmPolicies::WARN)                                          \
+  SELECT(POLICY, CMP0113,                                                     \
+         "Makefile generators do not repeat custom commands from target "     \
+         "dependencies.",                                                     \
          3, 19, 0, cmPolicies::WARN)
 
 #define CM_SELECT_ID(F, A1, A2, A3, A4, A5, A6) F(A1)
@@ -367,7 +371,8 @@ class cmMakefile;
   F(CMP0104)                                                                  \
   F(CMP0105)                                                                  \
   F(CMP0108)                                                                  \
-  F(CMP0112)
+  F(CMP0112)                                                                  \
+  F(CMP0113)
 
 /** \class cmPolicies
  * \brief Handles changes in CMake behavior and policies

Tests/RunCMake/Make/CMP0113-Common.cmake

@@ -0,0 +1,17 @@
+add_custom_command(
+  OUTPUT output-not-created
+  COMMAND ${CMAKE_COMMAND} -E echo output-not-created
+  DEPENDS ${CMAKE_CURRENT_LIST_FILE}
+  VERBATIM
+  )
+
+add_custom_command(
+  OUTPUT output-created
+  COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_LIST_FILE} output-created
+  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/output-not-created
+  VERBATIM
+  )
+
+add_custom_target(drive1 ALL DEPENDS output-not-created)
+add_custom_target(drive2 ALL DEPENDS output-created)
+add_dependencies(drive2 drive1)

Tests/RunCMake/Make/CMP0113-NEW-build-gnu-stderr.txt

@@ -0,0 +1,5 @@
+No rule to make target [^
+]*output-not-created[^
+]*, needed by [^
+]*output-created[^
+]*

Tests/RunCMake/Make/CMP0113-NEW-build-result.txt

@@ -0,0 +1 @@
+[^0]

Tests/RunCMake/Make/CMP0113-NEW-build-stderr.txt

@@ -0,0 +1 @@
+.*

Tests/RunCMake/Make/CMP0113-NEW-build-stdout.txt

@@ -0,0 +1 @@
+Generating output-not-created.*Built target drive1

Tests/RunCMake/Make/CMP0113-NEW.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0113 NEW)
+include(CMP0113-Common.cmake)

Tests/RunCMake/Make/CMP0113-OLD-build-stdout.txt

@@ -0,0 +1 @@
+Generating output-not-created.*Built target drive1.*Generating output-not-created.*Built target drive2

Tests/RunCMake/Make/CMP0113-OLD.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0113 OLD)
+include(CMP0113-Common.cmake)

Tests/RunCMake/Make/CMP0113-WARN-build-stdout.txt

@@ -0,0 +1 @@
+Generating output-not-created.*Built target drive1.*Generating output-not-created.*Built target drive2

Tests/RunCMake/Make/CMP0113-WARN.cmake

@@ -0,0 +1,2 @@
+# Policy CMP0113 not set.
+include(CMP0113-Common.cmake)

Tests/RunCMake/Make/RunCMakeTest.cmake

@@ -41,3 +41,25 @@ run_VerboseBuild()
 
 run_cmake(CustomCommandDepfile-ERROR)
 run_cmake(IncludeRegexSubdir)
+
+function(run_CMP0113 val)
+  set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/CMP0113-${val}-build)
+  run_cmake(CMP0113-${val})
+  set(RunCMake_TEST_NO_CLEAN 1)
+  set(_backup_lang "$ENV{LANG}")
+  set(_backup_lc_Messages "$ENV{LC_MESSAGES}")
+  if(MAKE_IS_GNU)
+    set(RunCMake-stderr-file CMP0113-${val}-build-gnu-stderr.txt)
+    set(ENV{LANG} "C")
+    set(ENV{LC_MESSAGES} "C")
+  endif()
+  run_cmake_command(CMP0113-${val}-build ${CMAKE_COMMAND} --build .)
+  set(ENV{LANG} "${_backup_lang}")
+  set(ENV{LC_MESSAGES} "${_backup_lc_messages}")
+endfunction()
+
+if(NOT RunCMake_GENERATOR STREQUAL "Watcom WMake")
+  run_CMP0113(WARN)
+  run_CMP0113(OLD)
+  run_CMP0113(NEW)
+endif()

Tests/RunCMake/TargetPolicies/PolicyList-stderr.txt

@@ -33,6 +33,7 @@
    \* CMP0105
    \* CMP0108
    \* CMP0112
+   \* CMP0113
 
 Call Stack \(most recent call first\):
   CMakeLists.txt:3 \(include\)