Merge topic 'find_package-stack'

1b946dfbfe Merge branch 'backport-4.2-find_package-stack' into find_package-stack
e8ae3645db Merge branch 'backport-4.2-find_package-stack' into find_package-stack
aae02ee60a find_package: Share package information among copies of package stack
a789c21100 Merge branch 'backport-4.2-find_package-stack' into find_package-stack
9387988626 find_package: Save package information only after successfully loading it
561ece2407 cmFindPackageStack: Restore pure value semantics
e935ed22fb find_package: Share package information among copies of package stack
5aa649d5f6 find_package: Save package information only after successfully loading it
...

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

Source/CMakeLists.txt

@@ -151,6 +151,8 @@ add_library(
   cmComputeTargetDepends.cxx
   cmConfigureLog.h
   cmConfigureLog.cxx
+  cmConstStack.h
+  cmConstStack.tcc
   cmCPackPropertiesGenerator.h
   cmCPackPropertiesGenerator.cxx
   cmCryptoHash.cxx
@@ -486,8 +488,6 @@ add_library(
   cmSourceFileLocationKind.h
   cmSourceGroup.cxx
   cmSourceGroup.h
-  cmStack.h
-  cmStack.tcc
   cmStandardLevel.h
   cmStandardLevelResolver.cxx
   cmStandardLevelResolver.h

Source/cmConstStack.h

@@ -5,31 +5,19 @@
 #include "cmConfigure.h" // IWYU pragma: keep
 
 #include <memory>
-#include <type_traits>
 
-enum class cmStackType
+/** Base class template for CRTP to represent a stack of constant values.
+    Provide value semantics, but use efficient reference-counting underneath
+    to avoid copies.  */
+template <typename T, typename Stack>
+class cmConstStack
 {
-  Const,
-  Mutable,
-};
-
-template <typename T, cmStackType Mutable>
-struct cmStackEntry;
-
-/** Base class template for CRTP to represent a stack of values.
-    Copies of the stack <i>share data</i>; mutating data on one copy will
-    change the data on <i>all</i> copies.  */
-template <typename T, typename Stack,
-          cmStackType Mutable = cmStackType::Mutable>
-class cmStack
-{
-  using Entry = cmStackEntry<T, Mutable>;
-
+  struct Entry;
   std::shared_ptr<Entry const> TopEntry;
 
 public:
   /** Default-construct an empty stack.  */
-  cmStack();
+  cmConstStack();
 
   /** Get a stack with the given call context added to the top.  */
   Stack Push(T value) const;
@@ -41,21 +29,11 @@ public:
   /** Get the value at the top of the stack.
       This may be called only if Empty() would return false.  */
   T const& Top() const;
-  template <bool E = (Mutable == cmStackType::Mutable)>
-  typename std::enable_if<E, T>::type& Top();
 
   /** Return true if this stack is empty.  */
   bool Empty() const;
 
 protected:
-  using Base = cmStack<T, Stack, Mutable>;
-
-  cmStack(std::shared_ptr<Entry const> parent, T value);
-  cmStack(std::shared_ptr<Entry const> top);
+  cmConstStack(std::shared_ptr<Entry const> parent, T value);
+  cmConstStack(std::shared_ptr<Entry const> top);
 };
-
-/** Specialization of cmStack for CRTP to represent a stack of constant values.
-    Provide value semantics, but use efficient reference-counting underneath
-    to avoid copies.  */
-template <typename T, typename Stack>
-using cmConstStack = cmStack<T const, Stack, cmStackType::Const>;

Source/cmConstStack.tcc

@@ -0,0 +1,62 @@
+/* Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
+   file LICENSE.rst or https://cmake.org/licensing for details.  */
+
+#include <cassert>
+#include <memory>
+#include <utility>
+
+template <typename T, typename Stack>
+struct cmConstStack<T, Stack>::Entry
+{
+  Entry(std::shared_ptr<Entry const> parent, T value)
+    : Value(std::move(value))
+    , Parent(std::move(parent))
+  {
+  }
+
+  T Value;
+  std::shared_ptr<Entry const> Parent;
+};
+
+template <typename T, typename Stack>
+cmConstStack<T, Stack>::cmConstStack() = default;
+
+template <typename T, typename Stack>
+Stack cmConstStack<T, Stack>::Push(T value) const
+{
+  return Stack(this->TopEntry, std::move(value));
+}
+
+template <typename T, typename Stack>
+Stack cmConstStack<T, Stack>::Pop() const
+{
+  assert(this->TopEntry);
+  return Stack(this->TopEntry->Parent);
+}
+
+template <typename T, typename Stack>
+T const& cmConstStack<T, Stack>::Top() const
+{
+  assert(this->TopEntry);
+  return this->TopEntry->Value;
+}
+
+template <typename T, typename Stack>
+bool cmConstStack<T, Stack>::Empty() const
+{
+  return !this->TopEntry;
+}
+
+template <typename T, typename Stack>
+cmConstStack<T, Stack>::cmConstStack(std::shared_ptr<Entry const> parent,
+                                     T value)
+  : TopEntry(
+      std::make_shared<Entry const>(std::move(parent), std::move(value)))
+{
+}
+
+template <typename T, typename Stack>
+cmConstStack<T, Stack>::cmConstStack(std::shared_ptr<Entry const> top)
+  : TopEntry(std::move(top))
+{
+}

Source/cmExportPackageInfoGenerator.cxx

@@ -311,7 +311,8 @@ bool cmExportPackageInfoGenerator::NoteLinkedTarget(
     auto pkgInfo = [](cmTarget* t) -> Package {
       cmFindPackageStack pkgStack = t->GetFindPackageStack();
       if (!pkgStack.Empty()) {
-        return std::make_pair(pkgStack.Top().Name, pkgStack.Top().PackageInfo);
+        return std::make_pair(pkgStack.Top().Name,
+                              *pkgStack.Top().PackageInfo);
       }
 
       cmPackageInformation package;

Source/cmExportSbomGenerator.cxx

@@ -4,6 +4,7 @@
 
 #include <array>
 #include <map>
+#include <memory>
 #include <set>
 #include <string>
 #include <utility>
@@ -320,7 +321,8 @@ bool cmExportSbomGenerator::NoteLinkedTarget(
     auto pkgInfo = [](cmTarget* t) -> Package {
       cmFindPackageStack pkgStack = t->GetFindPackageStack();
       if (!pkgStack.Empty()) {
-        return std::make_pair(pkgStack.Top().Name, pkgStack.Top().PackageInfo);
+        return std::make_pair(pkgStack.Top().Name,
+                              *pkgStack.Top().PackageInfo);
       }
       std::string const pkgName =
         t->GetSafeProperty("EXPORT_FIND_PACKAGE_NAME");

Source/cmFindPackageCommand.cxx

@@ -1213,13 +1213,15 @@ bool cmFindPackageCommand::FindPackage(
     }
   }
 
+  // Record package information discovered while it is loaded.
+  this->PackageInfo = std::make_shared<cmPackageInformation>();
+
   // RAII objects to ensure we leave this function with consistent state.
   FlushDebugBufferOnExit flushDebugBufferOnExit(*this);
   PushPopRootPathStack pushPopRootPathStack(*this);
   SetRestoreFindDefinitions setRestoreFindDefinitions(*this);
-  cmFindPackageStackRAII findPackageStackRAII(this->Makefile, this->Name);
-
-  findPackageStackRAII.BindTop(this->CurrentPackageInfo);
+  cmMakefile::FindPackageStackRAII findPackageStackRAII(
+    this->Makefile, this->Name, this->PackageInfo);
 
   // See if we have been told to delegate to FetchContent or some other
   // redirected config package first. We have to check all names that
@@ -1267,8 +1269,8 @@ bool cmFindPackageCommand::FindPackage(
       this->Names.clear();
       this->Names.emplace_back(overrideName); // Force finding this one
       this->Variable = cmStrCat(this->Name, "_DIR");
-      this->CurrentPackageInfo->Directory = redirectsDir;
-      this->CurrentPackageInfo->Version = this->VersionFound;
+      this->PackageInfo->Directory = redirectsDir;
+      this->PackageInfo->Version = this->VersionFound;
       this->SetConfigDirCacheVariable(redirectsDir);
       break;
     }
@@ -1666,12 +1668,6 @@ bool cmFindPackageCommand::HandlePackageMode(
         "fileFound is true but FileFound is empty!");
       fileFound = false;
     }
-
-    if (fileFound) {
-      this->CurrentPackageInfo->Directory =
-        cmSystemTools::GetFilenamePath(this->FileFound);
-      this->CurrentPackageInfo->Version = this->VersionFound;
-    }
   }
 
   std::string const foundVar = cmStrCat(this->Name, "_FOUND");
@@ -1733,6 +1729,12 @@ bool cmFindPackageCommand::HandlePackageMode(
       // The configuration file is invalid.
       result = false;
     }
+
+    if (this->UseConfigFiles && found) {
+      this->PackageInfo->Directory =
+        cmSystemTools::GetFilenamePath(this->FileFound);
+      this->PackageInfo->Version = this->VersionFound;
+    }
   }
 
   if (this->UseFindModules && !found &&

Source/cmFindPackageCommand.h

@@ -284,7 +284,7 @@ private:
   std::set<std::string> OptionalComponents;
   std::set<std::string> RequiredTargets;
   std::string DebugBuffer;
-  cmPackageInformation* CurrentPackageInfo;
+  std::shared_ptr<cmPackageInformation> PackageInfo;
 
   enum class SearchResult
   {

Source/cmFindPackageStack.cxx

@@ -3,13 +3,5 @@
 #define cmFindPackageStack_cxx
 #include "cmFindPackageStack.h"
 
-#include "cmStack.tcc" // IWYU pragma: keep
-template class cmStack<cmFindPackageCall, cmFindPackageStack>;
-
-template cmFindPackageCall&
-cmStack<cmFindPackageCall, cmFindPackageStack>::Top<true>();
-
-cmFindPackageCall const& cmFindPackageStack::Top() const
-{
-  return this->cmStack::Top();
-}
+#include "cmConstStack.tcc" // IWYU pragma: keep
+template class cmConstStack<cmFindPackageCall, cmFindPackageStack>;

Source/cmFindPackageStack.h

@@ -10,9 +10,7 @@
 
 #include <cm/optional>
 
-#include "cmStack.h"
-
-class cmMakefile;
+#include "cmConstStack.h"
 
 /**
  * This data represents the actual contents of find_package
@@ -39,52 +37,19 @@ class cmFindPackageCall
 {
 public:
   std::string const Name;
-  cmPackageInformation PackageInfo;
+  std::shared_ptr<cmPackageInformation const> PackageInfo;
   unsigned int Index;
 };
 
-/**
- * RAII type to manage the find_package call stack.
- */
-// Note: implemented in cmMakefile.cxx
-class cmFindPackageStackRAII
-{
-  cmMakefile* Makefile;
-  cmPackageInformation** Value = nullptr;
-
-public:
-  cmFindPackageStackRAII(cmMakefile* mf, std::string const& pkg);
-  ~cmFindPackageStackRAII();
-
-  cmFindPackageStackRAII(cmFindPackageStackRAII const&) = delete;
-  cmFindPackageStackRAII& operator=(cmFindPackageStackRAII const&) = delete;
-
-  /** Get a mutable pointer to the top of the stack.
-      The pointer is invalidated if BindTop is called again or when the
-      cmFindPackageStackRAII goes out of scope.  */
-  void BindTop(cmPackageInformation*& value);
-};
-
 /**
  * Represents a stack of find_package calls with efficient value semantics.
  */
 class cmFindPackageStack
-  : protected cmStack<cmFindPackageCall, cmFindPackageStack>
+  : public cmConstStack<cmFindPackageCall, cmFindPackageStack>
 {
-  using cmStack::cmStack;
-  friend cmFindPackageStack::Base;
-  friend class cmFindPackageStackRAII;
-
-public:
-  using cmStack::Push;
-  using cmStack::Pop;
-  using cmStack::Empty;
-
-  cmFindPackageCall const& Top() const;
+  using cmConstStack::cmConstStack;
+  friend class cmConstStack<cmFindPackageCall, cmFindPackageStack>;
 };
 #ifndef cmFindPackageStack_cxx
-extern template class cmStack<cmFindPackageCall, cmFindPackageStack>;
-
-extern template cmFindPackageCall&
-cmStack<cmFindPackageCall, cmFindPackageStack>::Top<true>();
+extern template class cmConstStack<cmFindPackageCall, cmFindPackageStack>;
 #endif

Source/cmListFileCache.cxx

@@ -458,9 +458,8 @@ bool cmListFile::ParseString(cm::string_view str,
   return parser.ParseString(str);
 }
 
-#include "cmStack.tcc"
-template class cmStack<cmListFileContext const, cmListFileBacktrace,
-                       cmStackType::Const>;
+#include "cmConstStack.tcc"
+template class cmConstStack<cmListFileContext, cmListFileBacktrace>;
 
 std::ostream& operator<<(std::ostream& os, cmListFileContext const& lfc)
 {

Source/cmListFileCache.h

@@ -14,8 +14,8 @@
 #include <cm/optional>
 #include <cm/string_view>
 
+#include "cmConstStack.h"
 #include "cmList.h"
-#include "cmStack.h"
 #include "cmSystemTools.h"
 
 /** \class cmListFileCache
@@ -171,12 +171,11 @@ bool operator!=(cmListFileContext const& lhs, cmListFileContext const& rhs);
 class cmListFileBacktrace
   : public cmConstStack<cmListFileContext, cmListFileBacktrace>
 {
-  using cmStack::cmStack;
-  friend cmListFileBacktrace::Base;
+  using cmConstStack::cmConstStack;
+  friend class cmConstStack<cmListFileContext, cmListFileBacktrace>;
 };
 #ifndef cmListFileCache_cxx
-extern template class cmStack<cmListFileContext const, cmListFileBacktrace,
-                              cmStackType::Const>;
+extern template class cmConstStack<cmListFileContext, cmListFileBacktrace>;
 #endif
 
 // Wrap type T as a value with a backtrace.  For purposes of

Source/cmMakefile.cxx

@@ -51,7 +51,6 @@
 #include "cmSourceFile.h"
 #include "cmSourceFileLocation.h"
 #include "cmSourceGroup.h"
-#include "cmStack.h"
 #include "cmState.h"
 #include "cmStateDirectory.h"
 #include "cmStateTypes.h"
@@ -4338,47 +4337,45 @@ cmMakefile::MacroPushPop::~MacroPushPop()
   this->Makefile->PopMacroScope(this->ReportError);
 }
 
-cmFindPackageStackRAII::cmFindPackageStackRAII(cmMakefile* mf,
-                                               std::string const& name)
+cmMakefile::FindPackageStackRAII::FindPackageStackRAII(
+  cmMakefile* mf, std::string const& name,
+  std::shared_ptr<cmPackageInformation const> pkgInfo)
   : Makefile(mf)
 {
   this->Makefile->FindPackageStack =
     this->Makefile->FindPackageStack.Push(cmFindPackageCall{
       name,
-      cmPackageInformation(),
+      std::move(pkgInfo),
       this->Makefile->FindPackageStackNextIndex,
     });
   this->Makefile->FindPackageStackNextIndex++;
 }
 
-void cmFindPackageStackRAII::BindTop(cmPackageInformation*& value)
+cmMakefile::FindPackageStackRAII::~FindPackageStackRAII()
 {
-  if (this->Value) {
-    *this->Value = nullptr;
-  }
-  this->Value = &value;
-  value = &this->Makefile->FindPackageStack.cmStack::Top().PackageInfo;
-}
-
-cmFindPackageStackRAII::~cmFindPackageStackRAII()
-{
-  if (this->Value) {
-    *this->Value = nullptr;
-  }
-
   this->Makefile->FindPackageStackNextIndex =
     this->Makefile->FindPackageStack.Top().Index + 1;
   this->Makefile->FindPackageStack = this->Makefile->FindPackageStack.Pop();
 
   if (!this->Makefile->FindPackageStack.Empty()) {
-    auto top = this->Makefile->FindPackageStack.Top();
+    // We have just finished an inner package found as a dependency of an
+    // outer package.  Targets created in the outer package after this
+    // point may depend on the inner package, so if they are exported,
+    // their find_dependency call for the outer package should be
+    // ordered after the find_dependency call for the inner package.
+    //
+    // Any targets created by the outer package before the inner package
+    // was loaded will have already saved a copy of the outer package
+    // stack with its original index.  Replace the top entry with a new
+    // one representing the same outer package with a new index.
+    cmFindPackageCall outer = this->Makefile->FindPackageStack.Top();
     this->Makefile->FindPackageStack = this->Makefile->FindPackageStack.Pop();
 
-    top.Index = this->Makefile->FindPackageStackNextIndex;
+    outer.Index = this->Makefile->FindPackageStackNextIndex;
     this->Makefile->FindPackageStackNextIndex++;
 
     this->Makefile->FindPackageStack =
-      this->Makefile->FindPackageStack.Push(top);
+      this->Makefile->FindPackageStack.Push(outer);
   }
 }
 

Source/cmMakefile.h

@@ -1100,7 +1100,21 @@ public:
   // searches
   std::deque<std::vector<std::string>> FindPackageRootPathStack;
 
-  friend class cmFindPackageStackRAII;
+  /**
+   * RAII type to manage the find_package call stack.
+   */
+  class FindPackageStackRAII
+  {
+    cmMakefile* Makefile;
+
+  public:
+    FindPackageStackRAII(cmMakefile* mf, std::string const& pkg,
+                         std::shared_ptr<cmPackageInformation const> pkgInfo);
+    ~FindPackageStackRAII();
+
+    FindPackageStackRAII(FindPackageStackRAII const&) = delete;
+    FindPackageStackRAII& operator=(FindPackageStackRAII const&) = delete;
+  };
 
   class DebugFindPkgRAII
   {

Source/cmStack.tcc

@@ -1,85 +0,0 @@
-/* Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
-   file LICENSE.rst or https://cmake.org/licensing for details.  */
-
-#include <cassert>
-#include <memory>
-#include <utility>
-
-template <typename T>
-struct cmStackEntry<T, cmStackType::Mutable>
-{
-  cmStackEntry(std::shared_ptr<cmStackEntry const> parent, T value)
-    : Value(std::move(value))
-    , Parent(std::move(parent))
-  {
-  }
-
-  T mutable Value;
-  std::shared_ptr<cmStackEntry const> Parent;
-};
-
-template <typename T>
-struct cmStackEntry<T, cmStackType::Const>
-{
-  cmStackEntry(std::shared_ptr<cmStackEntry const> parent, T value)
-    : Value(std::move(value))
-    , Parent(std::move(parent))
-  {
-  }
-
-  T Value;
-  std::shared_ptr<cmStackEntry const> Parent;
-};
-
-template <typename T, typename Stack, cmStackType Mutable>
-cmStack<T, Stack, Mutable>::cmStack() = default;
-
-template <typename T, typename Stack, cmStackType Mutable>
-Stack cmStack<T, Stack, Mutable>::Push(T value) const
-{
-  return Stack(this->TopEntry, std::move(value));
-}
-
-template <typename T, typename Stack, cmStackType Mutable>
-Stack cmStack<T, Stack, Mutable>::Pop() const
-{
-  assert(this->TopEntry);
-  return Stack(this->TopEntry->Parent);
-}
-
-template <typename T, typename Stack, cmStackType Mutable>
-T const& cmStack<T, Stack, Mutable>::Top() const
-{
-  assert(this->TopEntry);
-  return this->TopEntry->Value;
-}
-
-template <typename T, typename Stack, cmStackType Mutable>
-template <bool E>
-typename std::enable_if<E, T>::type& cmStack<T, Stack, Mutable>::Top()
-{
-  static_assert(Mutable == cmStackType::Mutable,
-                "T& cmStack::Top should only exist for mutable cmStack");
-  assert(this->TopEntry);
-  return this->TopEntry->Value;
-}
-
-template <typename T, typename Stack, cmStackType Mutable>
-bool cmStack<T, Stack, Mutable>::Empty() const
-{
-  return !this->TopEntry;
-}
-
-template <typename T, typename Stack, cmStackType Mutable>
-cmStack<T, Stack, Mutable>::cmStack(std::shared_ptr<Entry const> parent,
-                                    T value)
-  : TopEntry(
-      std::make_shared<Entry const>(std::move(parent), std::move(value)))
-{
-}
-
-template <typename T, typename Stack, cmStackType Mutable>
-cmStack<T, Stack, Mutable>::cmStack(std::shared_ptr<Entry const> top)
-  : TopEntry(std::move(top))
-{
-}

Tests/RunCMake/find_package/NestedConfig.cmake

@@ -0,0 +1,3 @@
+cmake_policy(SET CMP0074 NEW)
+set(Outer_ROOT ${CMAKE_CURRENT_SOURCE_DIR}/NestedConfig)
+find_package(Outer CONFIG)

Tests/RunCMake/find_package/NestedConfig/OuterConfig.cmake

@@ -0,0 +1 @@
+find_package(Inner CONFIG NO_DEFAULT_PATH)

Tests/RunCMake/find_package/RunCMakeTest.cmake

@@ -36,6 +36,7 @@ run_cmake(MissingConfigRequired)
 run_cmake(MissingConfigVersion)
 run_cmake(MixedModeOptions)
 run_cmake_with_options(ModuleModeDebugPkg --debug-find-pkg=Foo,Zot)
+run_cmake(NestedConfig)
 run_cmake(PackageRoot)
 run_cmake(PackageRootNestedConfig)
 run_cmake(PackageRootNestedModule)

Utilities/IWYU/mapping.imp

@@ -70,12 +70,8 @@
   { include: [ "<ncurses.h>", private, "\"cmCursesStandardIncludes.h\"", public ] },
   { include: [ "\"form.h\"", private, "\"cmCursesStandardIncludes.h\"", public ] },
 
-  # Help IWYU understand our explicit instantiation for cmStack.
-  { symbol: [ "cmStack::cmStack<T, Stack, Mutable>", private, "\"cmStack.h\"", public ] },
-  { symbol: [ "cmStack<cmFindPackageCall, cmFindPackageStack>::Empty", private, "\"cmStack.h\"", public ] },
-  { symbol: [ "cmStack<cmFindPackageCall, cmFindPackageStack>::Top", private, "\"cmStack.h\"", public ] },
-  { symbol: [ "cmStack<cmFindPackageCall, cmFindPackageStack>::Pop", private, "\"cmStack.h\"", public ] },
-  { symbol: [ "cmStack<cmFindPackageCall, cmFindPackageStack>::Push", private, "\"cmStack.h\"", public ] },
+  # Help IWYU understand our explicit instantiation for cmConstStack.
+  { symbol: [ "cmConstStack::cmConstStack<T, Stack>", private, "\"cmConstStack.h\"", public ] },
 ]
 
 # vim: set ft=toml: