Ninja: Add policy to require explicit custom command byproducts

Add policy CMP0058 to avoid generating 'phony' ninja rules for unknown
custom command dependencies.  This requires projects to specify their
custom command byproducts explicitly.  With this requirement we no
longer have to assume that unknown custom command dependencies are
generated and can instead simply assume they are source files expected
to exist when the build starts.  This is particularly important in
in-source builds.  It is also helpful for out-of-source builds to allow
Ninja to diagnose missing files before running custom command rules that
depend on them.

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,11 @@ 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
@@ -1047,9 +1068,11 @@ void cmGlobalNinjaGenerator::WriteUnknownExplicitDependencies(std::ostream& os)
                       knownDependencies.end(),
                       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 = unknownExplicitDepends.begin();
        i != unknownExplicitDepends.end();
@@ -1067,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;
 

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)