From de5fa2ae53c75eccff75df9df7be84e2d8f51397 Mon Sep 17 00:00:00 2001 From: Matthew Woehlke Date: Thu, 3 Apr 2025 14:40:39 -0400 Subject: [PATCH] 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 `_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 | 1 + Source/cmFindPackageCommand.cxx | 44 ++++++++++++------- Source/cmFindPackageCommand.h | 6 ++- Source/cmPackageState.h | 17 +++++++ Source/cmStatePrivate.h | 4 ++ Source/cmStateSnapshot.cxx | 8 ++++ Source/cmStateSnapshot.h | 3 ++ Tests/FindPackageCpsTest/CMakeLists.txt | 21 ++++++--- Tests/FindPackageCpsTest/cps/Repeat-extra.cps | 9 ++++ Tests/FindPackageCpsTest/cps/Repeat.cps | 2 +- 10 files changed, 89 insertions(+), 26 deletions(-) create mode 100644 Source/cmPackageState.h create mode 100644 Tests/FindPackageCpsTest/cps/Repeat-extra.cps diff --git a/Source/CMakeLists.txt b/Source/CMakeLists.txt index 24e190b466..24f87f5c1b 100644 --- a/Source/CMakeLists.txt +++ b/Source/CMakeLists.txt @@ -415,6 +415,7 @@ add_library( cmOrderDirectories.h cmPackageInfoReader.cxx cmPackageInfoReader.h + cmPackageState.h cmPathResolver.cxx cmPathResolver.h cmPlistParser.cxx diff --git a/Source/cmFindPackageCommand.cxx b/Source/cmFindPackageCommand.cxx index 398bc94ce2..781c80be28 100644 --- a/Source/cmFindPackageCommand.cxx +++ b/Source/cmFindPackageCommand.cxx @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -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; - - // 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(); - } + 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), '/', - cmSystemTools::GetFilenameWithoutExtension(fileName), + cmStrCat(cmSystemTools::GetFilenamePath(filePath), '/', + 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; } diff --git a/Source/cmFindPackageCommand.h b/Source/cmFindPackageCommand.h index adf64761e5..e228cf7ae0 100644 --- a/Source/cmFindPackageCommand.h +++ b/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); diff --git a/Source/cmPackageState.h b/Source/cmPackageState.h new file mode 100644 index 0000000000..92c9aa0213 --- /dev/null +++ b/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 +#include + +/** \class cmPackageState + * \brief Information about the state of an imported package. + */ +class cmPackageState +{ +public: + std::unordered_set ImportedFiles; +}; diff --git a/Source/cmStatePrivate.h b/Source/cmStatePrivate.h index 65e2111c14..c693d34168 100644 --- a/Source/cmStatePrivate.h +++ b/Source/cmStatePrivate.h @@ -6,11 +6,13 @@ #include "cmConfigure.h" // IWYU pragma: keep #include +#include #include #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 Packages; + std::vector Children; }; diff --git a/Source/cmStateSnapshot.cxx b/Source/cmStateSnapshot.cxx index 71b7dda7df..3e3fbbbe04 100644 --- a/Source/cmStateSnapshot.cxx +++ b/Source/cmStateSnapshot.cxx @@ -6,12 +6,14 @@ #include #include #include +#include #include #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"); diff --git a/Source/cmStateSnapshot.h b/Source/cmStateSnapshot.h index 866ec0ab4d..edc7e29399 100644 --- a/Source/cmStateSnapshot.h +++ b/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 diff --git a/Tests/FindPackageCpsTest/CMakeLists.txt b/Tests/FindPackageCpsTest/CMakeLists.txt index 1304ee7286..d85c7ca349 100644 --- a/Tests/FindPackageCpsTest/CMakeLists.txt +++ b/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. diff --git a/Tests/FindPackageCpsTest/cps/Repeat-extra.cps b/Tests/FindPackageCpsTest/cps/Repeat-extra.cps new file mode 100644 index 0000000000..2621babd12 --- /dev/null +++ b/Tests/FindPackageCpsTest/cps/Repeat-extra.cps @@ -0,0 +1,9 @@ +{ + "cps_version": "0.13", + "name": "Repeat", + "components": { + "Extra": { + "type": "interface" + } + } +} diff --git a/Tests/FindPackageCpsTest/cps/Repeat.cps b/Tests/FindPackageCpsTest/cps/Repeat.cps index 55354fdbfc..95b240c3d4 100644 --- a/Tests/FindPackageCpsTest/cps/Repeat.cps +++ b/Tests/FindPackageCpsTest/cps/Repeat.cps @@ -3,7 +3,7 @@ "name": "Repeat", "cps_path": "@prefix@/cps", "components": { - "Repeat": { + "Base": { "type": "interface" } }