add_subdirectory: Run subdirectory install rules in correct order

Before this change, install rules created by add_subdirectory() would be executed after all of the top-level install rules, even if they were declared before the top-level rules. This change adds a new policy, CMP0082, which interleaves the add_subdirectory() install rules with the other install rules so they are run in the correct order.
Kitware · Oct 10, 2018 · fc8955e · fc8955e
1 parent 514f0b5
commit fc8955e
Show file tree

Hide file tree

Showing 14 changed files with 235 additions and 19 deletions.
diff --git a/Help/manual/cmake-policies.7.rst b/Help/manual/cmake-policies.7.rst
@@ -51,6 +51,14 @@ The :variable:`CMAKE_MINIMUM_REQUIRED_VERSION` variable may also be used
 to determine whether to report an error on use of deprecated macros or
 functions.
 
+Policies Introduced by CMake 3.14
+=================================
+
+.. toctree::
+   :maxdepth: 1
+
+   CMP0082: Install rules from add_subdirectory() are interleaved with those in caller. </policy/CMP0082>
+
 Policies Introduced by CMake 3.13
 =================================
 

diff --git a/Help/policy/CMP0082.rst b/Help/policy/CMP0082.rst
@@ -0,0 +1,24 @@
+CMP0082
+-------
+
+Install rules from :command:`add_subdirectory` calls are interleaved with
+those in caller.
+
+CMake 3.13 and lower ran the install rules from :command:`add_subdirectory`
+after all other install rules, even if :command:`add_subdirectory` was called
+before the other install rules.  CMake 3.14 and later interleaves these
+:command:`add_subdirectory` install rules with the others so that they are
+run in the order they are declared.
+
+The ``OLD`` behavior for this policy is to run the install rules from
+:command:`add_subdirectory` after the other install rules.  The ``NEW``
+behavior for this policy is to run all install rules in the order they are
+declared.
+
+This policy was introduced in CMake version 3.14.  Unlike most policies,
+CMake version |release| does *not* warn by default when this policy
+is not set and simply uses OLD behavior.  See documentation of the
+:variable:`CMAKE_POLICY_WARNING_CMP0082 <CMAKE_POLICY_WARNING_CMP<NNNN>>`
+variable to control the warning.
+
+.. include:: DEPRECATED.txt
diff --git a/Help/release/dev/install-subdirectory-order.rst b/Help/release/dev/install-subdirectory-order.rst
@@ -0,0 +1,5 @@
+install-subdirectory-order
+--------------------------
+
+* Install rules under :command:`add_subdirectory` now interleave with those in
+  the calling directory. See policy :policy:`CMP0082` for details.
diff --git a/Help/variable/CMAKE_POLICY_WARNING_CMPNNNN.rst b/Help/variable/CMAKE_POLICY_WARNING_CMPNNNN.rst
@@ -19,6 +19,8 @@ warn by default:
   policy :policy:`CMP0066`.
 * ``CMAKE_POLICY_WARNING_CMP0067`` controls the warning for
   policy :policy:`CMP0067`.
+* ``CMAKE_POLICY_WARNING_CMP0082`` controls the warning for
+  policy :policy:`CMP0082`.
 
 This variable should not be set by a project in CMake code.  Project
 developers running CMake may set this variable in their cache to

diff --git a/Source/CMakeLists.txt b/Source/CMakeLists.txt
@@ -260,6 +260,8 @@ set(SRCS
   cmInstallFilesGenerator.cxx
   cmInstallScriptGenerator.h
   cmInstallScriptGenerator.cxx
+  cmInstallSubdirectoryGenerator.h
+  cmInstallSubdirectoryGenerator.cxx
   cmInstallTargetGenerator.h
   cmInstallTargetGenerator.cxx
   cmInstallDirectoryGenerator.h

diff --git a/Source/cmInstallGenerator.cxx b/Source/cmInstallGenerator.cxx
@@ -22,6 +22,19 @@ cmInstallGenerator::~cmInstallGenerator()
 {
 }
 
+bool cmInstallGenerator::HaveInstall()
+{
+  return true;
+}
+
+void cmInstallGenerator::CheckCMP0082(bool& haveSubdirectoryInstall,
+                                      bool& haveInstallAfterSubdirectory)
+{
+  if (haveSubdirectoryInstall) {
+    haveInstallAfterSubdirectory = true;
+  }
+}
+
 void cmInstallGenerator::AddInstallRule(
   std::ostream& os, std::string const& dest, cmInstallType type,
   std::vector<std::string> const& files, bool optional /* = false */,

diff --git a/Source/cmInstallGenerator.h b/Source/cmInstallGenerator.h
@@ -38,6 +38,10 @@ class cmInstallGenerator : public cmScriptGenerator
                      bool exclude_from_all);
   ~cmInstallGenerator() override;
 
+  virtual bool HaveInstall();
+  virtual void CheckCMP0082(bool& haveSubdirectoryInstall,
+                            bool& haveInstallAfterSubdirectory);
+
   void AddInstallRule(
     std::ostream& os, std::string const& dest, cmInstallType type,
     std::vector<std::string> const& files, bool optional = false,

diff --git a/Source/cmInstallScriptGenerator.cxx b/Source/cmInstallScriptGenerator.cxx
@@ -37,9 +37,9 @@ void cmInstallScriptGenerator::AddScriptInstallRule(std::ostream& os,
                                                     std::string const& script)
 {
   if (this->Code) {
-    os << indent.Next() << script << "\n";
+    os << indent << script << "\n";
   } else {
-    os << indent.Next() << "include(\"" << script << "\")\n";
+    os << indent << "include(\"" << script << "\")\n";
   }
 }
 

diff --git a/Source/cmInstallSubdirectoryGenerator.cxx b/Source/cmInstallSubdirectoryGenerator.cxx
@@ -0,0 +1,77 @@
+/* Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
+   file Copyright.txt or https://cmake.org/licensing for details.  */
+#include "cmInstallSubdirectoryGenerator.h"
+
+#include "cmLocalGenerator.h"
+#include "cmMakefile.h"
+#include "cmPolicies.h"
+#include "cmScriptGenerator.h"
+#include "cmSystemTools.h"
+
+#include <sstream>
+#include <vector>
+
+cmInstallSubdirectoryGenerator::cmInstallSubdirectoryGenerator(
+  cmMakefile* makefile, const char* binaryDirectory, bool excludeFromAll)
+  : cmInstallGenerator(nullptr, std::vector<std::string>(), nullptr,
+                       MessageDefault, excludeFromAll)
+  , Makefile(makefile)
+  , BinaryDirectory(binaryDirectory)
+{
+}
+
+cmInstallSubdirectoryGenerator::~cmInstallSubdirectoryGenerator()
+{
+}
+
+bool cmInstallSubdirectoryGenerator::HaveInstall()
+{
+  for (auto generator : this->Makefile->GetInstallGenerators()) {
+    if (generator->HaveInstall()) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+void cmInstallSubdirectoryGenerator::CheckCMP0082(
+  bool& haveSubdirectoryInstall, bool& /*unused*/)
+{
+  if (this->HaveInstall()) {
+    haveSubdirectoryInstall = true;
+  }
+}
+
+void cmInstallSubdirectoryGenerator::Compute(cmLocalGenerator* lg)
+{
+  this->LocalGenerator = lg;
+}
+
+void cmInstallSubdirectoryGenerator::GenerateScript(std::ostream& os)
+{
+  if (!this->ExcludeFromAll) {
+    cmPolicies::PolicyStatus status =
+      this->LocalGenerator->GetPolicyStatus(cmPolicies::CMP0082);
+    switch (status) {
+      case cmPolicies::WARN:
+      case cmPolicies::OLD:
+        // OLD behavior is handled in cmLocalGenerator::GenerateInstallRules()
+        break;
+
+      case cmPolicies::NEW:
+      case cmPolicies::REQUIRED_IF_USED:
+      case cmPolicies::REQUIRED_ALWAYS: {
+        Indent indent;
+        std::string odir = this->BinaryDirectory;
+        cmSystemTools::ConvertToUnixSlashes(odir);
+        os << indent << "if(NOT CMAKE_INSTALL_LOCAL_ONLY)\n"
+           << indent.Next()
+           << "# Include the install script for the subdirectory.\n"
+           << indent.Next() << "include(\"" << odir
+           << "/cmake_install.cmake\")\n"
+           << indent << "endif()\n\n";
+      } break;
+    }
+  }
+}
diff --git a/Source/cmInstallSubdirectoryGenerator.h b/Source/cmInstallSubdirectoryGenerator.h
@@ -0,0 +1,41 @@
+/* Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
+   file Copyright.txt or https://cmake.org/licensing for details.  */
+#ifndef cmInstallSubdirectoryGenerator_h
+#define cmInstallSubdirectoryGenerator_h
+
+#include "cmConfigure.h" // IWYU pragma: keep
+
+#include "cmInstallGenerator.h"
+
+#include <iosfwd>
+#include <string>
+
+class cmLocalGenerator;
+class cmMakefile;
+
+/** \class cmInstallSubdirectoryGenerator
+ * \brief Generate target installation rules.
+ */
+class cmInstallSubdirectoryGenerator : public cmInstallGenerator
+{
+public:
+  cmInstallSubdirectoryGenerator(cmMakefile* makefile,
+                                 const char* binaryDirectory,
+                                 bool excludeFromAll);
+  ~cmInstallSubdirectoryGenerator() override;
+
+  bool HaveInstall() override;
+  void CheckCMP0082(bool& haveSubdirectoryInstall,
+                    bool& haveInstallAfterSubdirectory) override;
+
+  void Compute(cmLocalGenerator* lg) override;
+
+protected:
+  void GenerateScript(std::ostream& os) override;
+
+  cmMakefile* Makefile;
+  std::string BinaryDirectory;
+  cmLocalGenerator* LocalGenerator;
+};
+
+#endif
diff --git a/Source/cmLocalGenerator.cxx b/Source/cmLocalGenerator.cxx
@@ -517,31 +517,62 @@ void cmLocalGenerator::GenerateInstallRules()
   }
 
   // Ask each install generator to write its code.
+  cmPolicies::PolicyStatus status = this->GetPolicyStatus(cmPolicies::CMP0082);
   std::vector<cmInstallGenerator*> const& installers =
     this->Makefile->GetInstallGenerators();
-  for (cmInstallGenerator* installer : installers) {
-    installer->Generate(fout, config, configurationTypes);
+  bool haveSubdirectoryInstall = false;
+  bool haveInstallAfterSubdirectory = false;
+  if (status == cmPolicies::WARN) {
+    for (cmInstallGenerator* installer : installers) {
+      installer->CheckCMP0082(haveSubdirectoryInstall,
+                              haveInstallAfterSubdirectory);
+      installer->Generate(fout, config, configurationTypes);
+    }
+  } else {
+    for (cmInstallGenerator* installer : installers) {
+      installer->Generate(fout, config, configurationTypes);
+    }
   }
 
   // Write rules from old-style specification stored in targets.
   this->GenerateTargetInstallRules(fout, config, configurationTypes);
 
   // Include install scripts from subdirectories.
-  std::vector<cmStateSnapshot> children =
-    this->Makefile->GetStateSnapshot().GetChildren();
-  if (!children.empty()) {
-    fout << "if(NOT CMAKE_INSTALL_LOCAL_ONLY)\n";
-    fout << "  # Include the install script for each subdirectory.\n";
-    for (cmStateSnapshot const& c : children) {
-      if (!c.GetDirectory().GetPropertyAsBool("EXCLUDE_FROM_ALL")) {
-        std::string odir = c.GetDirectory().GetCurrentBinary();
-        cmSystemTools::ConvertToUnixSlashes(odir);
-        fout << "  include(\"" << odir << "/cmake_install.cmake\")"
-             << std::endl;
+  switch (status) {
+    case cmPolicies::WARN:
+      if (haveInstallAfterSubdirectory &&
+          this->Makefile->PolicyOptionalWarningEnabled(
+            "CMAKE_POLICY_WARNING_CMP0082")) {
+        std::ostringstream e;
+        e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0082) << "\n";
+        this->IssueMessage(cmake::AUTHOR_WARNING, e.str());
       }
-    }
-    fout << "\n";
-    fout << "endif()\n\n";
+      CM_FALLTHROUGH;
+    case cmPolicies::OLD: {
+      std::vector<cmStateSnapshot> children =
+        this->Makefile->GetStateSnapshot().GetChildren();
+      if (!children.empty()) {
+        fout << "if(NOT CMAKE_INSTALL_LOCAL_ONLY)\n";
+        fout << "  # Include the install script for each subdirectory.\n";
+        for (cmStateSnapshot const& c : children) {
+          if (!c.GetDirectory().GetPropertyAsBool("EXCLUDE_FROM_ALL")) {
+            std::string odir = c.GetDirectory().GetCurrentBinary();
+            cmSystemTools::ConvertToUnixSlashes(odir);
+            fout << "  include(\"" << odir << "/cmake_install.cmake\")"
+                 << std::endl;
+          }
+        }
+        fout << "\n";
+        fout << "endif()\n\n";
+      }
+    } break;
+
+    case cmPolicies::REQUIRED_IF_USED:
+    case cmPolicies::REQUIRED_ALWAYS:
+    case cmPolicies::NEW:
+      // NEW behavior is handled in
+      // cmInstallSubdirectoryGenerator::GenerateScript()
+      break;
   }
 
   // Record the install manifest.

diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx
@@ -28,6 +28,7 @@
 #include "cmGeneratorExpressionEvaluationFile.h"
 #include "cmGlobalGenerator.h"
 #include "cmInstallGenerator.h" // IWYU pragma: keep
+#include "cmInstallSubdirectoryGenerator.h"
 #include "cmListFileCache.h"
 #include "cmSourceFile.h"
 #include "cmSourceFileLocation.h"
@@ -1669,6 +1670,9 @@ void cmMakefile::AddSubDirectory(const std::string& srcPath,
   } else {
     this->UnConfiguredDirectories.push_back(subMf);
   }
+
+  this->AddInstallGenerator(new cmInstallSubdirectoryGenerator(
+    subMf, binPath.c_str(), excludeFromAll));
 }
 
 const std::string& cmMakefile::GetCurrentSourceDirectory() const

diff --git a/Source/cmPolicies.h b/Source/cmPolicies.h
@@ -240,7 +240,11 @@ class cmMakefile;
          cmPolicies::WARN)                                                    \
   SELECT(POLICY, CMP0081,                                                     \
          "Relative paths not allowed in LINK_DIRECTORIES target property.",   \
-         3, 13, 0, cmPolicies::WARN)
+         3, 13, 0, cmPolicies::WARN)                                          \
+  SELECT(POLICY, CMP0082,                                                     \
+         "Install rules from add_subdirectory() are interleaved with those "  \
+         "in caller.",                                                        \
+         3, 14, 0, cmPolicies::WARN)
 
 #define CM_SELECT_ID(F, A1, A2, A3, A4, A5, A6) F(A1)
 #define CM_FOR_EACH_POLICY_ID(POLICY)                                         \

diff --git a/bootstrap b/bootstrap
@@ -348,6 +348,7 @@ CMAKE_CXX_SOURCES="\
   cmInstallFilesGenerator \
   cmInstallGenerator \
   cmInstallScriptGenerator \
+  cmInstallSubdirectoryGenerator \
   cmInstallTargetGenerator \
   cmInstallTargetsCommand \
   cmInstalledFile \