find_package: Improve support for CPS multiple inclusion

Keep track of CPS files we have imported in CMake's state, and use this
(instead of the prior, temporary work-around that used `<name>_CONFIG`)
as a guard for trying to import more than once from the same file. This
has two advantages; first, it is robust against finding the same package
name in different locations in alternating searches, and second, it
allows us to load additional appendices associated with a root package
that has already been loaded.

Fixes: #26731

Source/CMakeLists.txt

@@ -415,6 +415,7 @@ add_library(
   cmOrderDirectories.h
   cmPackageInfoReader.cxx
   cmPackageInfoReader.h
+  cmPackageState.h
   cmPathResolver.cxx
   cmPathResolver.h
   cmPlistParser.cxx

Source/cmFindPackageCommand.cxx

@@ -9,6 +9,7 @@
 #include <functional>
 #include <iterator>
 #include <sstream>
+#include <unordered_set>
 #include <utility>
 
 #include <cm/optional>
@@ -29,10 +30,12 @@
 #include "cmListFileCache.h"
 #include "cmMakefile.h"
 #include "cmMessageType.h"
+#include "cmPackageState.h"
 #include "cmPolicies.h"
 #include "cmRange.h"
 #include "cmSearchPath.h"
 #include "cmState.h"
+#include "cmStateSnapshot.h"
 #include "cmStateTypes.h"
 #include "cmStringAlgorithms.h"
 #include "cmSystemTools.h"
@@ -1543,7 +1546,6 @@ bool cmFindPackageCommand::HandlePackageMode(
     }
   }
 
-  std::string const fileVar = cmStrCat(this->Name, "_CONFIG");
   std::string const foundVar = cmStrCat(this->Name, "_FOUND");
   std::string const notFoundMessageVar =
     cmStrCat(this->Name, "_NOT_FOUND_MESSAGE");
@@ -1574,15 +1576,7 @@ bool cmFindPackageCommand::HandlePackageMode(
     if (this->CpsReader) {
       // The package has been found.
       found = true;
-
+      result = this->ReadPackage();
-      // Don't read a CPS file if we've already read it.
-      cmValue const& previousFileFound =
-        this->Makefile->GetDefinition(fileVar);
-      if (previousFileFound.Compare(this->FileFound) == 0) {
-        result = true;
-      } else {
-        result = this->ReadPackage();
-      }
     } else if (this->ReadListFile(this->FileFound, DoPolicyScope)) {
       // The package has been found.
       found = true;
@@ -1765,6 +1759,7 @@ bool cmFindPackageCommand::HandlePackageMode(
   this->Makefile->AddDefinition(foundVar, found ? "1" : "0");
 
   // Set a variable naming the configuration file that was found.
+  std::string const fileVar = cmStrCat(this->Name, "_CONFIG");
   if (found) {
     this->Makefile->AddDefinition(fileVar, this->FileFound);
   } else {
@@ -2016,8 +2011,15 @@ bool cmFindPackageCommand::ReadPackage()
     }
   }
 
+  // If we made it here, we want to actually import something, but we also
+  // need to ensure we don't try to import the same file more than once (which
+  // will fail due to the targets already existing). Retrieve the package state
+  // so we can record what we're doing.
+  cmPackageState& state =
+    this->Makefile->GetStateSnapshot().GetPackageState(this->FileFound);
+
   // Import targets from root file.
-  if (!this->ImportPackageTargets(this->FileFound, *this->CpsReader)) {
+  if (!this->ImportPackageTargets(state, this->FileFound, *this->CpsReader)) {
     return false;
   }
 
@@ -2026,7 +2028,7 @@ bool cmFindPackageCommand::ReadPackage()
   for (auto const& appendix : this->CpsAppendices) {
     cmMakefile::CallRAII appendixScope{ this->Makefile, appendix.first,
                                         this->Status };
-    if (!this->ImportPackageTargets(appendix.first, appendix.second)) {
+    if (!this->ImportPackageTargets(state, appendix.first, appendix.second)) {
       return false;
     }
   }
@@ -2035,13 +2037,13 @@ bool cmFindPackageCommand::ReadPackage()
 }
 
 bool cmFindPackageCommand::FindPackageDependencies(
-  std::string const& fileName, cmPackageInfoReader const& reader,
+  std::string const& filePath, cmPackageInfoReader const& reader,
   RequiredStatus required)
 {
   // Get package requirements.
   for (cmPackageRequirement const& dep : reader.GetRequirements()) {
     cmExecutionStatus status{ *this->Makefile };
-    cmMakefile::CallRAII scope{ this->Makefile, fileName, status };
+    cmMakefile::CallRAII scope{ this->Makefile, filePath, status };
 
     // For each requirement, set up a nested instance to find it.
     cmFindPackageCommand fp{ status };
@@ -2077,9 +2079,16 @@ bool cmFindPackageCommand::FindPackageDependencies(
   return true;
 }
 
-bool cmFindPackageCommand::ImportPackageTargets(std::string const& fileName,
+bool cmFindPackageCommand::ImportPackageTargets(cmPackageState& packageState,
+                                                std::string const& filePath,
                                                 cmPackageInfoReader& reader)
 {
+  // Check if we've already imported this file.
+  std::string fileName = cmSystemTools::GetFilenameName(filePath);
+  if (cm::contains(packageState.ImportedFiles, fileName)) {
+    return true;
+  }
+
   // Import base file.
   if (!reader.ImportTargets(this->Makefile, this->Status)) {
     return false;
@@ -2089,8 +2098,8 @@ bool cmFindPackageCommand::ImportPackageTargets(std::string const& fileName,
   cmsys::Glob glob;
   glob.RecurseOff();
   if (glob.FindFiles(
-        cmStrCat(cmSystemTools::GetFilenamePath(fileName), '/',
+        cmStrCat(cmSystemTools::GetFilenamePath(filePath), '/',
-                 cmSystemTools::GetFilenameWithoutExtension(fileName),
+                 cmSystemTools::GetFilenameWithoutExtension(filePath),
                  "@*.[Cc][Pp][Ss]"_s))) {
 
     // Try to read supplemental data from each file found.
@@ -2106,6 +2115,7 @@ bool cmFindPackageCommand::ImportPackageTargets(std::string const& fileName,
     }
   }
 
+  packageState.ImportedFiles.emplace(std::move(fileName));
   return true;
 }
 

Source/cmFindPackageCommand.h

@@ -33,6 +33,7 @@ namespace std {
 #endif
 
 class cmExecutionStatus;
+class cmPackageState;
 class cmSearchPath;
 
 /** \class cmFindPackageCommand
@@ -159,11 +160,12 @@ private:
     RequiredFromPackageVar,
     RequiredFromFindVar
   };
-  bool FindPackageDependencies(std::string const& fileName,
+  bool FindPackageDependencies(std::string const& filePath,
                                cmPackageInfoReader const& reader,
                                RequiredStatus required);
 
-  bool ImportPackageTargets(std::string const& fileName,
+  bool ImportPackageTargets(cmPackageState& packageState,
+                            std::string const& filePath,
                             cmPackageInfoReader& reader);
   void StoreVersionFound();
   void SetConfigDirCacheVariable(std::string const& value);

Source/cmPackageState.h

@@ -0,0 +1,17 @@
+/* Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
+   file LICENSE.rst or https://cmake.org/licensing for details.  */
+#pragma once
+
+#include "cmConfigure.h" // IWYU pragma: keep
+
+#include <string>
+#include <unordered_set>
+
+/** \class cmPackageState
+ * \brief Information about the state of an imported package.
+ */
+class cmPackageState
+{
+public:
+  std::unordered_set<std::string> ImportedFiles;
+};

Source/cmStatePrivate.h

@@ -6,11 +6,13 @@
 #include "cmConfigure.h" // IWYU pragma: keep
 
 #include <string>
+#include <unordered_map>
 #include <vector>
 
 #include "cmDefinitions.h"
 #include "cmLinkedTree.h"
 #include "cmListFileCache.h"
+#include "cmPackageState.h"
 #include "cmPolicies.h"
 #include "cmPropertyMap.h"
 #include "cmStateSnapshot.h"
@@ -84,5 +86,7 @@ struct cmStateDetail::BuildsystemDirectoryStateType
 
   cmPropertyMap Properties;
 
+  std::unordered_map<std::string, cmPackageState> Packages;
+
   std::vector<cmStateSnapshot> Children;
 };

Source/cmStateSnapshot.cxx

@@ -6,12 +6,14 @@
 #include <algorithm>
 #include <cassert>
 #include <string>
+#include <unordered_map>
 
 #include <cm/iterator>
 
 #include "cmDefinitions.h"
 #include "cmLinkedTree.h"
 #include "cmListFileCache.h"
+#include "cmPackageState.h"
 #include "cmPropertyMap.h"
 #include "cmState.h"
 #include "cmStateDirectory.h"
@@ -418,6 +420,12 @@ std::string cmStateSnapshot::GetProjectName() const
   return this->Position->BuildSystemDirectory->ProjectName;
 }
 
+cmPackageState& cmStateSnapshot::GetPackageState(
+  std::string const& packagePath)
+{
+  return this->Position->BuildSystemDirectory->Packages[packagePath];
+}
+
 void cmStateSnapshot::InitializeFromParent_ForSubdirsCommand()
 {
   std::string currentSrcDir = *this->GetDefinition("CMAKE_CURRENT_SOURCE_DIR");

Source/cmStateSnapshot.h

@@ -14,6 +14,7 @@
 #include "cmStateTypes.h"
 #include "cmValue.h"
 
+class cmPackageState;
 class cmState;
 class cmStateDirectory;
 
@@ -57,6 +58,8 @@ public:
   void SetProjectName(std::string const& name);
   std::string GetProjectName() const;
 
+  cmPackageState& GetPackageState(std::string const& packagePath);
+
   void InitializeFromParent_ForSubdirsCommand();
 
   struct StrictWeakOrder

Tests/FindPackageCpsTest/CMakeLists.txt

@@ -54,12 +54,6 @@ endfunction()
 find_package(Sample CONFIG)
 test_version(Sample "2.10.11" 3 2 10 11 0)
 
-###############################################################################
-# Test finding a package more than once.
-
-find_package(Repeat REQUIRED)
-find_package(Repeat REQUIRED)
-
 ###############################################################################
 # Test some more complicated version parsing.
 
@@ -275,6 +269,21 @@ elseif(TARGET TransitiveDep::Target5)
   message(SEND_ERROR "TransitiveDep::Target5 exists ?!")
 endif()
 
+###############################################################################
+# Test finding a package more than once.
+
+find_package(Repeat REQUIRED OPTIONAL_COMPONENTS DoesNotExist)
+if(NOT TARGET Repeat::Base)
+  message(SEND_ERROR "Repeat::Base missing !")
+elseif(TARGET Repeat::Extra)
+  message(SEND_ERROR "Repeat::Extra exists ?!")
+endif()
+
+find_package(Repeat REQUIRED COMPONENTS Extra)
+if(NOT TARGET Repeat::Extra)
+  message(SEND_ERROR "Repeat::Extra missing !")
+endif()
+
 ###############################################################################
 # Test default configurations.
 

Tests/FindPackageCpsTest/cps/Repeat-extra.cps

@@ -0,0 +1,9 @@
+{
+  "cps_version": "0.13",
+  "name": "Repeat",
+  "components": {
+    "Extra": {
+      "type": "interface"
+    }
+  }
+}

Tests/FindPackageCpsTest/cps/Repeat.cps

@@ -3,7 +3,7 @@
   "name": "Repeat",
   "cps_path": "@prefix@/cps",
   "components": {
-    "Repeat": {
+    "Base": {
       "type": "interface"
     }
   }