From 74a6ddc339ba36637949e40cad216d41adb552a7 Mon Sep 17 00:00:00 2001
From: Craig Scott <craig.scott@crascit.com>
Date: Sun, 15 May 2022 19:02:27 +1000
Subject: cmFindPackageCommand: Handle Makefile variable definitions more
 robustly

During argument parsing in InitialPass(), Makefile variables were
being added for components. Most other such variables were set
in the call to SetModuleVariables(), which happens much later.
Both sets of variables were then restored to their previous values
as part of a call to AppendSuccessInformation(), but that is not an
obvious nor robust place to undo those variable changes.

InitialPass() also pushes a new item to the package root stack, but
the corresponding pop was in AppendSuccessInformation().
Again, this puts a symmetric operation in an asymmetric place.

Refactor the code slightly such that Makefile variables are set in
one clear location, then restored later in the same function.
Also move the package root stack pop into the same function as
the push. AppendSuccessInformation() now has one clear
responsibility and doesn't perform any unrelated cleanup on
behalf of InitialPass().
---
 Source/cmFindPackageCommand.cxx | 26 ++++++++++++++++----------
 Source/cmFindPackageCommand.h   |  4 +++-
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/Source/cmFindPackageCommand.cxx b/Source/cmFindPackageCommand.cxx
index 6586c69..fa2eb1c 100644
--- a/Source/cmFindPackageCommand.cxx
+++ b/Source/cmFindPackageCommand.cxx
@@ -238,6 +238,7 @@ bool cmFindPackageCommand::InitialPass(std::vector<std::string> const& args)
   const char* components_sep = "";
   std::set<std::string> requiredComponents;
   std::set<std::string> optionalComponents;
+  std::vector<std::pair<std::string, const char*>> componentVarDefs;
 
   // Always search directly in a generated path.
   this->SearchPathSuffixes.emplace_back();
@@ -356,7 +357,7 @@ bool cmFindPackageCommand::InitialPass(std::vector<std::string> const& args)
       }
 
       std::string req_var = this->Name + "_FIND_REQUIRED_" + args[i];
-      this->AddFindDefinition(req_var, isRequired);
+      componentVarDefs.emplace_back(req_var, isRequired);
 
       // Append to the list of required components.
       components += components_sep;
@@ -573,7 +574,7 @@ bool cmFindPackageCommand::InitialPass(std::vector<std::string> const& args)
     }
   }
 
-  this->SetModuleVariables(components);
+  this->SetModuleVariables(components, componentVarDefs);
 
   // See if we have been told to delegate to FetchContent or some other
   // redirected config package first. We have to check all names that
@@ -697,6 +698,12 @@ bool cmFindPackageCommand::InitialPass(std::vector<std::string> const& args)
 
   this->AppendSuccessInformation();
 
+  // Restore original state of "_FIND_" variables set in SetModuleVariables()
+  this->RestoreFindDefinitions();
+
+  // Pop the package stack
+  this->Makefile->FindPackageRootPathStack.pop_back();
+
   if (!this->DebugBuffer.empty()) {
     this->DebugMessage(this->DebugBuffer);
   }
@@ -778,13 +785,18 @@ void cmFindPackageCommand::SetVersionVariables(
   addDefinition(prefix + "_COUNT", buf);
 }
 
-void cmFindPackageCommand::SetModuleVariables(const std::string& components)
+void cmFindPackageCommand::SetModuleVariables(
+  const std::string& components,
+  const std::vector<std::pair<std::string, const char*>>& componentVarDefs)
 {
   this->AddFindDefinition("CMAKE_FIND_PACKAGE_NAME", this->Name);
 
-  // Store the list of components.
+  // Store the list of components and associated variable definitions
   std::string components_var = this->Name + "_FIND_COMPONENTS";
   this->AddFindDefinition(components_var, components);
+  for (const auto& varDef : componentVarDefs) {
+    this->AddFindDefinition(varDef.first, varDef.second);
+  }
 
   if (this->Quiet) {
     // Tell the module that is about to be read that it should find
@@ -1388,12 +1400,6 @@ void cmFindPackageCommand::AppendSuccessInformation()
     this->Makefile->GetState()->SetGlobalProperty(requiredInfoPropName,
                                                   "REQUIRED");
   }
-
-  // Restore original state of "_FIND_" variables we set.
-  this->RestoreFindDefinitions();
-
-  // Pop the package stack
-  this->Makefile->FindPackageRootPathStack.pop_back();
 }
 
 inline std::size_t collectPathsForDebug(std::string& buffer,
diff --git a/Source/cmFindPackageCommand.h b/Source/cmFindPackageCommand.h
index 902fa32..1767b74 100644
--- a/Source/cmFindPackageCommand.h
+++ b/Source/cmFindPackageCommand.h
@@ -97,7 +97,9 @@ private:
     const std::string& prefix, const std::string& version, unsigned int count,
     unsigned int major, unsigned int minor, unsigned int patch,
     unsigned int tweak);
-  void SetModuleVariables(const std::string& components);
+  void SetModuleVariables(
+    const std::string& components,
+    const std::vector<std::pair<std::string, const char*>>& componentVarDefs);
   bool FindModule(bool& found);
   void AddFindDefinition(const std::string& var, cm::string_view value);
   void RestoreFindDefinitions();
-- 
cgit v0.12