Merge topic 'ninja-require-byproducts'

bd9c7f9b Ninja: Add policy to require explicit custom command byproducts
ed8e30b0 cmGlobalNinjaGenerator: Optimize handling of known build outputs
ad094f43 cmGlobalNinjaGenerator: Fix spelling of "unknown"
82a37d3c cmGlobalNinjaGenerator: Drop unused member

Help/manual/cmake-policies.7.rst

@@ -115,3 +115,4 @@ All Policies
    /policy/CMP0055
    /policy/CMP0056
    /policy/CMP0057
+   /policy/CMP0058

Help/policy/CMP0058.rst

@@ -0,0 +1,108 @@
+CMP0058
+-------
+
+Ninja requires custom command byproducts to be explicit.
+
+When an intermediate file generated during the build is consumed
+by an expensive operation or a large tree of dependents, one may
+reduce the work needed for an incremental rebuild by updating the
+file timestamp only when its content changes.  With this approach
+the generation rule must have a separate output file that is always
+updated with a new timestamp that is newer than any dependencies of
+the rule so that the build tool re-runs the rule only when the input
+changes.  We refer to the separate output file as a rule's *witness*
+and the generated file as a rule's *byproduct*.
+
+Byproducts may not be listed as outputs because their timestamps are
+allowed to be older than the inputs.  No build tools (like ``make``)
+that existed when CMake was designed have a way to express byproducts.
+Therefore CMake versions prior to 3.2 had no way to specify them.
+Projects typically left byproducts undeclared in the rules that
+generate them.  For example:
+
+.. code-block:: cmake
+
+  add_custom_command(
+    OUTPUT witness.txt
+    COMMAND ${CMAKE_COMMAND} -E copy_if_different
+            ${CMAKE_CURRENT_SOURCE_DIR}/input.txt
+            byproduct.txt # timestamp may not change
+    COMMAND ${CMAKE_COMMAND} -E touch witness.txt
+    DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/input.txt
+    )
+  add_custom_target(Provider DEPENDS witness.txt)
+  add_custom_command(
+    OUTPUT generated.c
+    COMMAND expensive-task -i byproduct.txt -o generated.c
+    DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/byproduct.txt
+    )
+  add_library(Consumer generated.c)
+  add_dependencies(Consumer Provider)
+
+This works well for all generators except :generator:`Ninja`.
+The Ninja build tool sees a rule listing ``byproduct.txt``
+as a dependency and no rule listing it as an output.  Ninja then
+complains that there is no way to satisfy the dependency and
+stops building even though there are order-only dependencies
+that ensure ``byproduct.txt`` will exist before its consumers
+need it.  See discussion of this problem in `Ninja Issue 760`_
+for further details on why Ninja works this way.
+
+.. _`Ninja Issue 760`: https://github.com/martine/ninja/issues/760
+
+Instead of leaving byproducts undeclared in the rules that generate
+them, Ninja expects byproducts to be listed along with other outputs.
+Such rules may be marked with a ``restat`` option that tells Ninja
+to check the timestamps of outputs after the rules run.  This
+prevents byproducts whose timestamps do not change from causing
+their dependents to re-build unnecessarily.
+
+Since the above approach does not tell CMake what custom command
+generates ``byproduct.txt``, the Ninja generator does not have
+enough information to add the byproduct as an output of any rule.
+CMake 2.8.12 and above work around this problem and allow projects
+using the above approach to build by generating ``phony`` build
+rules to tell Ninja to tolerate such missing files.  However, this
+workaround prevents Ninja from diagnosing a dependency that is
+really missing.  It also works poorly in in-source builds where
+every custom command dependency, even on source files, needs to
+be treated this way because CMake does not have enough information
+to know which files are generated as byproducts of custom commands.
+
+CMake 3.2 introduced the ``BYPRODUCTS`` option to the
+:command:`add_custom_command` and :command:`add_custom_target`
+commands.  This option allows byproducts to be specified explicitly:
+
+.. code-block:: cmake
+
+  add_custom_command(
+    OUTPUT witness.txt
+    BYPRODUCTS byproduct.txt # explicit byproduct specification
+    COMMAND ${CMAKE_COMMAND} -E copy_if_different
+            ${CMAKE_CURRENT_SOURCE_DIR}/input.txt
+            byproduct.txt # timestamp may not change
+  ...
+
+The ``BYPRODUCTS`` option is used by the :generator:`Ninja` generator
+to list byproducts among the outputs of the custom commands that
+generate them, and is ignored by other generators.
+
+CMake 3.3 and above prefer to require projects to specify custom
+command byproducts explicitly so that it can avoid using the
+``phony`` rule workaround altogether.  Policy ``CMP0058`` was
+introduced to provide compatibility with existing projects that
+still need the workaround.
+
+This policy has no effect on generators other than :generator:`Ninja`.
+The ``OLD`` behavior for this policy is to generate Ninja ``phony``
+rules for unknown dependencies in the build tree.  The ``NEW``
+behavior for this policy is to not generate these and instead
+require projects to specify custom command ``BYPRODUCTS`` explicitly.
+
+This policy was introduced in CMake version 3.3.
+CMake version |release| warns when it sees unknown dependencies in
+out-of-source build trees if the policy is not set and then uses
+``OLD`` behavior.  Use the :command:`cmake_policy` command to set
+the policy to ``OLD`` or ``NEW`` explicitly.  The policy setting
+must be in scope at the end of the top-level ``CMakeLists.txt``
+file of the project and has global effect.

Help/release/dev/ninja-require-byproducts.rst

@@ -0,0 +1,9 @@
+ninja-require-byproducts
+------------------------
+
+* The :generator:`Ninja` generator now requires that calls to the
+  :command:`add_custom_command` and :command:`add_custom_target`
+  commands use the ``BYPRODUCTS`` option to explicitly specify any
+  files generated by the custom commands that are not listed as
+  outputs (perhaps because their timestamps are allowed to be older
+  than the inputs).  See policy :policy:`CMP0058`.

Source/cmGlobalNinjaGenerator.cxx

@@ -17,6 +17,7 @@
 #include "cmLocalNinjaGenerator.h"
 #include "cmMakefile.h"
 #include "cmVersion.h"
+#include "cmAlgorithms.h"
 
 #include <algorithm>
 #include <assert.h>
@@ -183,7 +184,10 @@ void cmGlobalNinjaGenerator::WriteBuild(std::ostream& os,
       i != outputs.end(); ++i)
     {
     build += " " + EncodeIdent(EncodePath(*i), os);
-    this->CombinedBuildOutputs.insert( EncodePath(*i) );
+    if (this->ComputingUnknownDependencies)
+      {
+      this->CombinedBuildOutputs.insert( EncodePath(*i) );
+      }
     }
   build += ":";
 
@@ -281,11 +285,14 @@ cmGlobalNinjaGenerator::WriteCustomCommandBuild(const std::string& command,
                    orderOnly,
                    vars);
 
-  //we need to track every dependency that comes in, since we are trying
-  //to find dependencies that are side effects of build commands
-  for(cmNinjaDeps::const_iterator i = deps.begin(); i != deps.end(); ++i)
+  if (this->ComputingUnknownDependencies)
     {
-    this->CombinedCustomCommandExplicitDependencies.insert( EncodePath(*i) );
+    //we need to track every dependency that comes in, since we are trying
+    //to find dependencies that are side effects of build commands
+    for(cmNinjaDeps::const_iterator i = deps.begin(); i != deps.end(); ++i)
+      {
+      this->CombinedCustomCommandExplicitDependencies.insert(EncodePath(*i));
+      }
     }
 }
 
@@ -477,6 +484,8 @@ cmGlobalNinjaGenerator::cmGlobalNinjaGenerator()
   , CompileCommandsStream(0)
   , Rules()
   , AllDependencies()
+  , ComputingUnknownDependencies(false)
+  , PolicyCMP0058(cmPolicies::WARN)
 {
   // // Ninja is not ported to non-Unix OS yet.
   // this->ForceUnixPaths = true;
@@ -510,6 +519,13 @@ void cmGlobalNinjaGenerator::Generate()
   this->OpenBuildFileStream();
   this->OpenRulesFileStream();
 
+  this->PolicyCMP0058 =
+    this->LocalGenerators[0]->GetMakefile()
+    ->GetPolicyStatus(cmPolicies::CMP0058);
+  this->ComputingUnknownDependencies =
+    (this->PolicyCMP0058 == cmPolicies::OLD ||
+     this->PolicyCMP0058 == cmPolicies::WARN);
+
   this->cmGlobalGenerator::Generate();
 
   this->WriteAssumedSourceDependencies();
@@ -955,6 +971,18 @@ void cmGlobalNinjaGenerator::WriteTargetAliases(std::ostream& os)
 
 void cmGlobalNinjaGenerator::WriteUnknownExplicitDependencies(std::ostream& os)
 {
+  if (!this->ComputingUnknownDependencies)
+    {
+    return;
+    }
+
+  // We need to collect the set of known build outputs.
+  // Start with those generated by WriteBuild calls.
+  // No other method needs this so we can take ownership
+  // of the set locally and throw it out when we are done.
+  std::set<std::string> knownDependencies;
+  knownDependencies.swap(this->CombinedBuildOutputs);
+
   //now write out the unknown explicit dependencies.
 
   //union the configured files, evaluations files and the CombinedBuildOutputs,
@@ -971,7 +999,6 @@ void cmGlobalNinjaGenerator::WriteUnknownExplicitDependencies(std::ostream& os)
   cmLocalNinjaGenerator *ng =
     static_cast<cmLocalNinjaGenerator *>(this->LocalGenerators[0]);
 
-  std::set<std::string> knownDependencies;
   for (std::vector<cmLocalGenerator *>::const_iterator i =
        this->LocalGenerators.begin(); i != this->LocalGenerators.end(); ++i)
     {
@@ -1026,36 +1053,29 @@ void cmGlobalNinjaGenerator::WriteUnknownExplicitDependencies(std::ostream& os)
     knownDependencies.insert( ng->ConvertToNinjaPath(i->first) );
     }
 
-  //insert outputs from all WirteBuild commands
-  //these paths have already be encoded when added to CombinedBuildOutputs
-  knownDependencies.insert(this->CombinedBuildOutputs.begin(),
-                           this->CombinedBuildOutputs.end());
-
-  //after we have combined the data into knownDependencies we have no need
-  //to keep this data around
-  this->CombinedBuildOutputs.clear();
-
   //now we difference with CombinedCustomCommandExplicitDependencies to find
   //the list of items we know nothing about.
   //We have encoded all the paths in CombinedCustomCommandExplicitDependencies
   //and knownDependencies so no matter if unix or windows paths they
   //should all match now.
 
-  std::vector<std::string> unkownExplicitDepends;
+  std::vector<std::string> unknownExplicitDepends;
   this->CombinedCustomCommandExplicitDependencies.erase("all");
 
   std::set_difference(this->CombinedCustomCommandExplicitDependencies.begin(),
                       this->CombinedCustomCommandExplicitDependencies.end(),
                       knownDependencies.begin(),
                       knownDependencies.end(),
-                      std::back_inserter(unkownExplicitDepends));
-
+                      std::back_inserter(unknownExplicitDepends));
 
   std::string const rootBuildDirectory =
       this->GetCMakeInstance()->GetHomeOutputDirectory();
+  bool const inSourceBuild =
+    (rootBuildDirectory == this->GetCMakeInstance()->GetHomeDirectory());
+  std::vector<std::string> warnExplicitDepends;
   for (std::vector<std::string>::const_iterator
-       i = unkownExplicitDepends.begin();
-       i != unkownExplicitDepends.end();
+       i = unknownExplicitDepends.begin();
+       i != unknownExplicitDepends.end();
        ++i)
     {
     //verify the file is in the build directory
@@ -1070,8 +1090,34 @@ void cmGlobalNinjaGenerator::WriteUnknownExplicitDependencies(std::ostream& os)
                             "",
                             deps,
                             cmNinjaDeps());
+      if (this->PolicyCMP0058 == cmPolicies::WARN &&
+          !inSourceBuild && warnExplicitDepends.size() < 10)
+        {
+        warnExplicitDepends.push_back(*i);
+        }
       }
    }
+
+  if (!warnExplicitDepends.empty())
+    {
+    std::ostringstream w;
+    w <<
+      (this->GetCMakeInstance()->GetPolicies()->
+       GetPolicyWarning(cmPolicies::CMP0058)) << "\n"
+      "This project specifies custom command DEPENDS on files "
+      "in the build tree that are not specified as the OUTPUT or "
+      "BYPRODUCTS of any add_custom_command or add_custom_target:\n"
+      " " << cmJoin(warnExplicitDepends, "\n ") <<
+      "\n"
+      "For compatibility with versions of CMake that did not have "
+      "the BYPRODUCTS option, CMake is generating phony rules for "
+      "such files to convince 'ninja' to build."
+      "\n"
+      "Project authors should add the missing BYPRODUCTS or OUTPUT "
+      "options to the custom commands that produce these files."
+      ;
+    this->GetCMakeInstance()->IssueMessage(cmake::AUTHOR_WARNING, w.str());
+    }
 }
 
 void cmGlobalNinjaGenerator::WriteBuiltinTargets(std::ostream& os)

Source/cmGlobalNinjaGenerator.h

@@ -368,6 +368,11 @@ private:
   /// The set of custom command outputs we have seen.
   std::set<std::string> CustomCommandOutputs;
 
+  /// Whether we are collecting known build outputs and needed
+  /// dependencies to determine unknown dependencies.
+  bool ComputingUnknownDependencies;
+  cmPolicies::PolicyStatus PolicyCMP0058;
+
   /// The combined explicit dependencies of custom build commands
   std::set<std::string> CombinedCustomCommandExplicitDependencies;
 
@@ -381,8 +386,6 @@ private:
   typedef std::map<std::string, cmTarget*> TargetAliasMap;
   TargetAliasMap TargetAliases;
 
-  static cmLocalGenerator* LocalGenerator;
-
   static bool UsingMinGW;
 
 };

Source/cmPolicies.cxx

@@ -380,6 +380,11 @@ cmPolicies::cmPolicies()
     CMP0057, "CMP0057",
     "Disallow multiple MAIN_DEPENDENCY specifications for the same file.",
     3,3,0, cmPolicies::WARN);
+
+  this->DefinePolicy(
+    CMP0058, "CMP0058",
+    "Ninja requires custom command byproducts to be explicit.",
+    3,3,0, cmPolicies::WARN);
 }
 
 cmPolicies::~cmPolicies()

Source/cmPolicies.h

@@ -115,6 +115,7 @@ public:
     CMP0056, ///< Honor link flags in try_compile() source-file signature.
     CMP0057, ///< Disallow multiple MAIN_DEPENDENCY specifications
     /// for the same file.
+    CMP0058, ///< Ninja requires custom command byproducts to be explicit
 
     /** \brief Always the last entry.
      *

Tests/RunCMake/CMakeLists.txt

@@ -64,6 +64,9 @@ add_RunCMake_test(CMP0053)
 add_RunCMake_test(CMP0054)
 add_RunCMake_test(CMP0055)
 add_RunCMake_test(CMP0057)
+if(CMAKE_GENERATOR STREQUAL "Ninja")
+  add_RunCMake_test(Ninja)
+endif()
 add_RunCMake_test(CTest)
 
 if(NOT CMake_TEST_EXTERNAL_CMAKE)

Tests/RunCMake/Ninja/CMP0058-NEW-by-build-stdout.txt

@@ -0,0 +1,4 @@
+^[^
+]* Generating output1
+[^
+]* Generating output2$

Tests/RunCMake/Ninja/CMP0058-NEW-by.cmake

@@ -0,0 +1,3 @@
+cmake_policy(SET CMP0058 NEW)
+set(byproducts BYPRODUCTS byproduct1a byproduct1b)
+include(CMP0058-common.cmake)

Tests/RunCMake/Ninja/CMP0058-NEW-no-build-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/Ninja/CMP0058-NEW-no-build-stderr.txt

@@ -0,0 +1 @@
+ninja: error: 'byproduct1a', needed by 'output2', missing and no known rule to make it

Tests/RunCMake/Ninja/CMP0058-NEW-no.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0058 NEW)
+include(CMP0058-common.cmake)

Tests/RunCMake/Ninja/CMP0058-OLD-by-build-stdout.txt

@@ -0,0 +1,4 @@
+^[^
+]* Generating output1
+[^
+]* Generating output2$

Tests/RunCMake/Ninja/CMP0058-OLD-by.cmake

@@ -0,0 +1,3 @@
+cmake_policy(SET CMP0058 OLD)
+set(byproducts BYPRODUCTS byproduct1a byproduct1b)
+include(CMP0058-common.cmake)

Tests/RunCMake/Ninja/CMP0058-OLD-no-build-stdout.txt

@@ -0,0 +1,4 @@
+^[^
+]* Generating output1
+[^
+]* Generating output2$

Tests/RunCMake/Ninja/CMP0058-OLD-no.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0058 OLD)
+include(CMP0058-common.cmake)

Tests/RunCMake/Ninja/CMP0058-WARN-by-build-stdout.txt

@@ -0,0 +1,4 @@
+^[^
+]* Generating output1
+[^
+]* Generating output2$

Tests/RunCMake/Ninja/CMP0058-WARN-by.cmake

@@ -0,0 +1,2 @@
+set(byproducts BYPRODUCTS byproduct1a byproduct1b)
+include(CMP0058-common.cmake)

Tests/RunCMake/Ninja/CMP0058-WARN-no-build-stdout.txt

@@ -0,0 +1,4 @@
+^[^
+]* Generating output1
+[^
+]* Generating output2$

Tests/RunCMake/Ninja/CMP0058-WARN-no-stderr.txt

@@ -0,0 +1,19 @@
+^CMake Warning \(dev\):
+  Policy CMP0058 is not set: Ninja requires custom command byproducts to be
+  explicit.  Run "cmake --help-policy CMP0058" for policy details.  Use the
+  cmake_policy command to set the policy and suppress this warning.
+
+  This project specifies custom command DEPENDS on files in the build tree
+  that are not specified as the OUTPUT or BYPRODUCTS of any
+  add_custom_command or add_custom_target:
+
+   byproduct1a
+   byproduct1b
+
+  For compatibility with versions of CMake that did not have the BYPRODUCTS
+  option, CMake is generating phony rules for such files to convince 'ninja'
+  to build.
+
+  Project authors should add the missing BYPRODUCTS or OUTPUT options to the
+  custom commands that produce these files.
+This warning is for project developers.  Use -Wno-dev to suppress it.

Tests/RunCMake/Ninja/CMP0058-WARN-no.cmake

@@ -0,0 +1 @@
+include(CMP0058-common.cmake)

Tests/RunCMake/Ninja/CMP0058-common.cmake

@@ -0,0 +1,17 @@
+add_custom_command(
+  OUTPUT output1
+  ${byproducts}
+  COMMAND ${CMAKE_COMMAND} -E touch output1
+  COMMAND ${CMAKE_COMMAND} -E touch byproduct1a
+  COMMAND ${CMAKE_COMMAND} -E touch byproduct1b
+  )
+add_custom_target(Drive1 ALL DEPENDS output1)
+add_custom_command(
+  OUTPUT output2
+  COMMAND ${CMAKE_COMMAND} -E copy output1 output2
+  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/output1
+          ${CMAKE_CURRENT_BINARY_DIR}/byproduct1a
+          ${CMAKE_CURRENT_BINARY_DIR}/byproduct1b
+  )
+add_custom_target(Drive2 ALL DEPENDS output2)
+add_dependencies(Drive2 Drive1)

Tests/RunCMake/Ninja/CMakeLists.txt

@@ -0,0 +1,3 @@
+cmake_minimum_required(VERSION 3.2)
+project(${RunCMake_TEST} NONE)
+include(${RunCMake_TEST}.cmake NO_POLICY_SCOPE)

Tests/RunCMake/Ninja/RunCMakeTest.cmake

@@ -0,0 +1,18 @@
+include(RunCMake)
+
+function(run_CMP0058 case)
+  # Use a single build tree for a few tests without cleaning.
+  set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/CMP0058-${case}-build)
+  set(RunCMake_TEST_NO_CLEAN 1)
+  file(REMOVE_RECURSE "${RunCMake_TEST_BINARY_DIR}")
+  file(MAKE_DIRECTORY "${RunCMake_TEST_BINARY_DIR}")
+  run_cmake(CMP0058-${case})
+  run_cmake_command(CMP0058-${case}-build ${CMAKE_COMMAND} --build .)
+endfunction()
+
+run_CMP0058(OLD-no)
+run_CMP0058(OLD-by)
+run_CMP0058(WARN-no)
+run_CMP0058(WARN-by)
+run_CMP0058(NEW-no)
+run_CMP0058(NEW-by)