Merge topic 'find_item-consistent-behavior-cache-variables'

08db1341a6 find_*: ensure consistent behavior for cache variables
f5fa6d53b0 class cmake: Store working directory at cmake launch
b1729200c3 find_*: refactor cache variable handling

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

11 files changed, 167 insertions, 112 deletions

diff --git a/Source/cmFindBase.cxx b/Source/cmFindBase.cxx
index bf52d7513a..6296d06e24 100644
--- a/Source/cmFindBase.cxx
+++ b/Source/cmFindBase.cxx
@@ -9,7 +9,10 @@
 
 #include <cmext/algorithm>
 
+#include "cmCMakePath.h"
 #include "cmMakefile.h"
+#include "cmMessageType.h"
+#include "cmPolicies.h"
 #include "cmProperty.h"
 #include "cmRange.h"
 #include "cmSearchPath.h"
@@ -17,11 +20,13 @@
 #include "cmStateTypes.h"
 #include "cmStringAlgorithms.h"
 #include "cmSystemTools.h"
+#include "cmake.h"
 
 class cmExecutionStatus;
 
-cmFindBase::cmFindBase(cmExecutionStatus& status)
+cmFindBase::cmFindBase(std::string findCommandName, cmExecutionStatus& status)
   : cmFindCommon(status)
+  , FindCommandName(std::move(findCommandName))
 {
 }
 
@@ -299,27 +304,106 @@ bool cmFindBase::CheckForVariableInCache()
     cmProp cacheEntry = state->GetCacheEntryValue(this->VariableName);
     bool found = !cmIsNOTFOUND(*cacheValue);
     bool cached = cacheEntry != nullptr;
+    auto cacheType = cached ? state->GetCacheEntryType(this->VariableName)
+                            : cmStateEnums::UNINITIALIZED;
+
+    if (cached && cacheType != cmStateEnums::UNINITIALIZED) {
+      this->VariableType = cacheType;
+      if (const auto* hs =
+            state->GetCacheEntryProperty(this->VariableName, "HELPSTRING")) {
+        this->VariableDocumentation = *hs;
+      }
+    }
+
     if (found) {
       // If the user specifies the entry on the command line without a
       // type we should add the type and docstring but keep the
       // original value.  Tell the subclass implementations to do
       // this.
-      if (cached &&
-          state->GetCacheEntryType(this->VariableName) ==
-            cmStateEnums::UNINITIALIZED) {
+      if (cached && cacheType == cmStateEnums::UNINITIALIZED) {
         this->AlreadyInCacheWithoutMetaInfo = true;
       }
       return true;
     }
-    if (cached) {
-      cmProp hs =
-        state->GetCacheEntryProperty(this->VariableName, "HELPSTRING");
-      this->VariableDocumentation = hs ? *hs : "(none)";
-    }
   }
   return false;
 }
 
+void cmFindBase::NormalizeFindResult()
+{
+  if (this->Makefile->GetPolicyStatus(cmPolicies::CMP0125) ==
+      cmPolicies::NEW) {
+    // ensure the path returned by find_* command is absolute
+    const auto* existingValue =
+      this->Makefile->GetDefinition(this->VariableName);
+    std::string value;
+    if (!existingValue->empty()) {
+      value =
+        cmCMakePath(*existingValue, cmCMakePath::auto_format)
+          .Absolute(cmCMakePath(
+            this->Makefile->GetCMakeInstance()->GetCMakeWorkingDirectory()))
+          .Normal()
+          .GenericString();
+      // value = cmSystemTools::CollapseFullPath(*existingValue);
+      if (!cmSystemTools::FileExists(value, false)) {
+        value = *existingValue;
+      }
+    }
+
+    // If the user specifies the entry on the command line without a
+    // type we should add the type and docstring but keep the original
+    // value.
+    if (value != *existingValue || this->AlreadyInCacheWithoutMetaInfo) {
+      this->Makefile->GetCMakeInstance()->AddCacheEntry(
+        this->VariableName, value.c_str(), this->VariableDocumentation.c_str(),
+        this->VariableType);
+      // if there was a definition then remove it
+      // This is required to ensure same behavior as
+      // cmMakefile::AddCacheDefinition.
+      // See #22038 for problems raised by this behavior.
+      this->Makefile->RemoveDefinition(this->VariableName);
+    }
+  } else {
+    // If the user specifies the entry on the command line without a
+    // type we should add the type and docstring but keep the original
+    // value.
+    if (this->AlreadyInCacheWithoutMetaInfo) {
+      this->Makefile->AddCacheDefinition(this->VariableName, "",
+                                         this->VariableDocumentation.c_str(),
+                                         this->VariableType);
+    }
+  }
+}
+
+void cmFindBase::StoreFindResult(const std::string& value)
+{
+  bool force =
+    this->Makefile->GetPolicyStatus(cmPolicies::CMP0125) == cmPolicies::NEW;
+
+  if (!value.empty()) {
+    this->Makefile->AddCacheDefinition(this->VariableName, value,
+                                       this->VariableDocumentation.c_str(),
+                                       this->VariableType, force);
+    return;
+  }
+
+  this->Makefile->AddCacheDefinition(
+    this->VariableName, cmStrCat(this->VariableName, "-NOTFOUND"),
+    this->VariableDocumentation.c_str(), this->VariableType, force);
+
+  if (this->Required) {
+    this->Makefile->IssueMessage(
+      MessageType::FATAL_ERROR,
+      cmStrCat("Could not find ", this->VariableName, " using the following ",
+               (this->FindCommandName == "find_file" ||
+                    this->FindCommandName == "find_path"
+                  ? "files"
+                  : "names"),
+               ": ", cmJoin(this->Names, ", ")));
+    cmSystemTools::SetFatalErrorOccured();
+  }
+}
+
 cmFindBaseDebugState::cmFindBaseDebugState(std::string commandName,
                                            cmFindBase const* findBase)
   : FindCommand(findBase)
diff --git a/Source/cmFindBase.h b/Source/cmFindBase.h
index 57a40be9b6..c2a9288aad 100644
--- a/Source/cmFindBase.h
+++ b/Source/cmFindBase.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "cmFindCommon.h"
+#include "cmStateTypes.h"
 
 class cmExecutionStatus;
 
@@ -21,7 +22,7 @@ class cmExecutionStatus;
 class cmFindBase : public cmFindCommon
 {
 public:
-  cmFindBase(cmExecutionStatus& status);
+  cmFindBase(std::string findCommandName, cmExecutionStatus& status);
   virtual ~cmFindBase() = default;
 
   /**
@@ -39,8 +40,15 @@ protected:
   // if it has documentation in the cache
   bool CheckForVariableInCache();
 
+  void NormalizeFindResult();
+  void StoreFindResult(const std::string& value);
+
+  // actual find command name
+  std::string FindCommandName;
+
   // use by command during find
   std::string VariableDocumentation;
+  cmStateEnums::CacheEntryType VariableType = cmStateEnums::UNINITIALIZED;
   std::string VariableName;
   std::vector<std::string> Names;
   bool NamesPerDir = false;
diff --git a/Source/cmFindFileCommand.cxx b/Source/cmFindFileCommand.cxx
index 29a2bc49e9..a88c1b10c1 100644
--- a/Source/cmFindFileCommand.cxx
+++ b/Source/cmFindFileCommand.cxx
@@ -2,12 +2,15 @@
    file Copyright.txt or https://cmake.org/licensing for details.  */
 #include "cmFindFileCommand.h"
 
+#include "cmStateTypes.h"
+
 class cmExecutionStatus;
 
 cmFindFileCommand::cmFindFileCommand(cmExecutionStatus& status)
-  : cmFindPathCommand(status)
+  : cmFindPathCommand("find_file", status)
 {
   this->IncludeFileInPath = true;
+  this->VariableType = cmStateEnums::FILEPATH;
 }
 
 bool cmFindFile(std::vector<std::string> const& args,
diff --git a/Source/cmFindLibraryCommand.cxx b/Source/cmFindLibraryCommand.cxx
index d85ba8fd9f..b1f4275951 100644
--- a/Source/cmFindLibraryCommand.cxx
+++ b/Source/cmFindLibraryCommand.cxx
@@ -12,7 +12,6 @@
 
 #include "cmGlobalGenerator.h"
 #include "cmMakefile.h"
-#include "cmMessageType.h"
 #include "cmProperty.h"
 #include "cmState.h"
 #include "cmStateTypes.h"
@@ -22,30 +21,26 @@
 class cmExecutionStatus;
 
 cmFindLibraryCommand::cmFindLibraryCommand(cmExecutionStatus& status)
-  : cmFindBase(status)
+  : cmFindBase("find_library", status)
 {
   this->EnvironmentPath = "LIB";
   this->NamesPerDirAllowed = true;
+  this->VariableDocumentation = "Path to a library.";
+  this->VariableType = cmStateEnums::FILEPATH;
 }
 
 // cmFindLibraryCommand
 bool cmFindLibraryCommand::InitialPass(std::vector<std::string> const& argsIn)
 {
   this->DebugMode = this->ComputeIfDebugModeWanted();
-  this->VariableDocumentation = "Path to a library.";
   this->CMakePathName = "LIBRARY";
+
   if (!this->ParseArguments(argsIn)) {
     return false;
   }
+
   if (this->AlreadyInCache) {
-    // If the user specifies the entry on the command line without a
-    // type we should add the type and docstring but keep the original
-    // value.
-    if (this->AlreadyInCacheWithoutMetaInfo) {
-      this->Makefile->AddCacheDefinition(this->VariableName, "",
-                                         this->VariableDocumentation.c_str(),
-                                         cmStateEnums::FILEPATH);
-    }
+    this->NormalizeFindResult();
     return true;
   }
 
@@ -75,24 +70,7 @@ bool cmFindLibraryCommand::InitialPass(std::vector<std::string> const& argsIn)
   }
 
   std::string const library = this->FindLibrary();
-  if (!library.empty()) {
-    // Save the value in the cache
-    this->Makefile->AddCacheDefinition(this->VariableName, library,
-                                       this->VariableDocumentation.c_str(),
-                                       cmStateEnums::FILEPATH);
-    return true;
-  }
-  std::string notfound = this->VariableName + "-NOTFOUND";
-  this->Makefile->AddCacheDefinition(this->VariableName, notfound,
-                                     this->VariableDocumentation.c_str(),
-                                     cmStateEnums::FILEPATH);
-  if (this->Required) {
-    this->Makefile->IssueMessage(
-      MessageType::FATAL_ERROR,
-      "Could not find " + this->VariableName +
-        " using the following names: " + cmJoin(this->Names, ", "));
-    cmSystemTools::SetFatalErrorOccured();
-  }
+  this->StoreFindResult(library);
   return true;
 }
 
@@ -208,7 +186,8 @@ std::string cmFindLibraryCommand::FindLibrary()
 
 struct cmFindLibraryHelper
 {
-  cmFindLibraryHelper(cmMakefile* mf, cmFindBase const* findBase);
+  cmFindLibraryHelper(std::string debugName, cmMakefile* mf,
+                      cmFindBase const* findBase);
 
   // Context information.
   cmMakefile* Makefile;
@@ -280,11 +259,11 @@ struct cmFindLibraryHelper
   };
 };
 
-cmFindLibraryHelper::cmFindLibraryHelper(cmMakefile* mf,
+cmFindLibraryHelper::cmFindLibraryHelper(std::string debugName, cmMakefile* mf,
                                          cmFindBase const* base)
   : Makefile(mf)
   , DebugMode(base->DebugModeEnabled())
-  , DebugSearches("find_library", base)
+  , DebugSearches(std::move(debugName), base)
 {
   this->GG = this->Makefile->GetGlobalGenerator();
 
@@ -485,7 +464,7 @@ std::string cmFindLibraryCommand::FindNormalLibrary()
 std::string cmFindLibraryCommand::FindNormalLibraryNamesPerDir()
 {
   // Search for all names in each directory.
-  cmFindLibraryHelper helper(this->Makefile, this);
+  cmFindLibraryHelper helper(this->FindCommandName, this->Makefile, this);
   for (std::string const& n : this->Names) {
     helper.AddName(n);
   }
@@ -502,7 +481,7 @@ std::string cmFindLibraryCommand::FindNormalLibraryNamesPerDir()
 std::string cmFindLibraryCommand::FindNormalLibraryDirsPerName()
 {
   // Search the entire path for each name.
-  cmFindLibraryHelper helper(this->Makefile, this);
+  cmFindLibraryHelper helper(this->FindCommandName, this->Makefile, this);
   for (std::string const& n : this->Names) {
     // Switch to searching for this name.
     helper.SetName(n);
diff --git a/Source/cmFindPathCommand.cxx b/Source/cmFindPathCommand.cxx
index 3fb082603e..126cc2f213 100644
--- a/Source/cmFindPathCommand.cxx
+++ b/Source/cmFindPathCommand.cxx
@@ -2,70 +2,53 @@
    file Copyright.txt or https://cmake.org/licensing for details.  */
 #include "cmFindPathCommand.h"
 
+#include <utility>
+
 #include "cmsys/Glob.hxx"
 
-#include "cmMakefile.h"
-#include "cmMessageType.h"
 #include "cmStateTypes.h"
 #include "cmStringAlgorithms.h"
 #include "cmSystemTools.h"
 
 class cmExecutionStatus;
 
-cmFindPathCommand::cmFindPathCommand(cmExecutionStatus& status)
-  : cmFindBase(status)
+cmFindPathCommand::cmFindPathCommand(std::string findCommandName,
+                                     cmExecutionStatus& status)
+  : cmFindBase(std::move(findCommandName), status)
 {
   this->EnvironmentPath = "INCLUDE";
   this->IncludeFileInPath = false;
+  this->VariableDocumentation = "Path to a file.";
+  this->VariableType = cmStateEnums::PATH;
+}
+cmFindPathCommand::cmFindPathCommand(cmExecutionStatus& status)
+  : cmFindPathCommand("find_path", status)
+{
 }
 
 // cmFindPathCommand
 bool cmFindPathCommand::InitialPass(std::vector<std::string> const& argsIn)
 {
   this->DebugMode = this->ComputeIfDebugModeWanted();
-  this->VariableDocumentation = "Path to a file.";
   this->CMakePathName = "INCLUDE";
+
   if (!this->ParseArguments(argsIn)) {
     return false;
   }
+
   if (this->AlreadyInCache) {
-    // If the user specifies the entry on the command line without a
-    // type we should add the type and docstring but keep the original
-    // value.
-    if (this->AlreadyInCacheWithoutMetaInfo) {
-      this->Makefile->AddCacheDefinition(
-        this->VariableName, "", this->VariableDocumentation.c_str(),
-        (this->IncludeFileInPath ? cmStateEnums::FILEPATH
-                                 : cmStateEnums::PATH));
-    }
+    this->NormalizeFindResult();
     return true;
   }
 
   std::string result = this->FindHeader();
-  if (!result.empty()) {
-    this->Makefile->AddCacheDefinition(
-      this->VariableName, result, this->VariableDocumentation.c_str(),
-      (this->IncludeFileInPath) ? cmStateEnums::FILEPATH : cmStateEnums::PATH);
-    return true;
-  }
-  this->Makefile->AddCacheDefinition(
-    this->VariableName, this->VariableName + "-NOTFOUND",
-    this->VariableDocumentation.c_str(),
-    (this->IncludeFileInPath) ? cmStateEnums::FILEPATH : cmStateEnums::PATH);
-  if (this->Required) {
-    this->Makefile->IssueMessage(
-      MessageType::FATAL_ERROR,
-      "Could not find " + this->VariableName +
-        " using the following files: " + cmJoin(this->Names, ", "));
-    cmSystemTools::SetFatalErrorOccured();
-  }
+  this->StoreFindResult(result);
   return true;
 }
 
 std::string cmFindPathCommand::FindHeader()
 {
-  std::string debug_name = this->IncludeFileInPath ? "find_file" : "find_path";
-  cmFindBaseDebugState debug(debug_name, this);
+  cmFindBaseDebugState debug(this->FindCommandName, this);
   std::string header;
   if (this->SearchFrameworkFirst || this->SearchFrameworkOnly) {
     header = this->FindFrameworkHeader(debug);
diff --git a/Source/cmFindPathCommand.h b/Source/cmFindPathCommand.h
index 6101ea1c00..c7281f1f83 100644
--- a/Source/cmFindPathCommand.h
+++ b/Source/cmFindPathCommand.h
@@ -22,6 +22,7 @@ class cmFindPathCommand : public cmFindBase
 {
 public:
   cmFindPathCommand(cmExecutionStatus& status);
+  cmFindPathCommand(std::string findCommandName, cmExecutionStatus& status);
 
   bool InitialPass(std::vector<std::string> const& args);
 
diff --git a/Source/cmFindProgramCommand.cxx b/Source/cmFindProgramCommand.cxx
index c22462e84a..76fc4a42a3 100644
--- a/Source/cmFindProgramCommand.cxx
+++ b/Source/cmFindProgramCommand.cxx
@@ -4,6 +4,7 @@
 
 #include <algorithm>
 #include <string>
+#include <utility>
 
 #include "cmMakefile.h"
 #include "cmMessageType.h"
@@ -20,8 +21,9 @@ class cmExecutionStatus;
 
 struct cmFindProgramHelper
 {
-  cmFindProgramHelper(cmMakefile* makefile, cmFindBase const* base)
-    : DebugSearches("find_program", base)
+  cmFindProgramHelper(std::string debugName, cmMakefile* makefile,
+                      cmFindBase const* base)
+    : DebugSearches(std::move(debugName), base)
     , Makefile(makefile)
     , PolicyCMP0109(makefile->GetPolicyStatus(cmPolicies::CMP0109))
   {
@@ -145,52 +147,31 @@ struct cmFindProgramHelper
 };
 
 cmFindProgramCommand::cmFindProgramCommand(cmExecutionStatus& status)
-  : cmFindBase(status)
+  : cmFindBase("find_program", status)
 {
   this->NamesPerDirAllowed = true;
+  this->VariableDocumentation = "Path to a program.";
+  this->VariableType = cmStateEnums::FILEPATH;
 }
 
 // cmFindProgramCommand
 bool cmFindProgramCommand::InitialPass(std::vector<std::string> const& argsIn)
 {
   this->DebugMode = this->ComputeIfDebugModeWanted();
-  this->VariableDocumentation = "Path to a program.";
   this->CMakePathName = "PROGRAM";
+
   // call cmFindBase::ParseArguments
   if (!this->ParseArguments(argsIn)) {
     return false;
   }
+
   if (this->AlreadyInCache) {
-    // If the user specifies the entry on the command line without a
-    // type we should add the type and docstring but keep the original
-    // value.
-    if (this->AlreadyInCacheWithoutMetaInfo) {
-      this->Makefile->AddCacheDefinition(this->VariableName, "",
-                                         this->VariableDocumentation.c_str(),
-                                         cmStateEnums::FILEPATH);
-    }
+    this->NormalizeFindResult();
     return true;
   }
 
   std::string const result = this->FindProgram();
-  if (!result.empty()) {
-    // Save the value in the cache
-    this->Makefile->AddCacheDefinition(this->VariableName, result,
-                                       this->VariableDocumentation.c_str(),
-                                       cmStateEnums::FILEPATH);
-
-    return true;
-  }
-  this->Makefile->AddCacheDefinition(
-    this->VariableName, this->VariableName + "-NOTFOUND",
-    this->VariableDocumentation.c_str(), cmStateEnums::FILEPATH);
-  if (this->Required) {
-    this->Makefile->IssueMessage(
-      MessageType::FATAL_ERROR,
-      "Could not find " + this->VariableName +
-        " using the following names: " + cmJoin(this->Names, ", "));
-    cmSystemTools::SetFatalErrorOccured();
-  }
+  this->StoreFindResult(result);
   return true;
 }
 
@@ -222,7 +203,7 @@ std::string cmFindProgramCommand::FindNormalProgram()
 std::string cmFindProgramCommand::FindNormalProgramNamesPerDir()
 {
   // Search for all names in each directory.
-  cmFindProgramHelper helper(this->Makefile, this);
+  cmFindProgramHelper helper(this->FindCommandName, this->Makefile, this);
   for (std::string const& n : this->Names) {
     helper.AddName(n);
   }
@@ -245,7 +226,7 @@ std::string cmFindProgramCommand::FindNormalProgramNamesPerDir()
 std::string cmFindProgramCommand::FindNormalProgramDirsPerName()
 {
   // Search the entire path for each name.
-  cmFindProgramHelper helper(this->Makefile, this);
+  cmFindProgramHelper helper(this->FindCommandName, this->Makefile, this);
   for (std::string const& n : this->Names) {
     // Switch to searching for this name.
     helper.SetName(n);
diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx
index 29f9e36b01..4ffd47bec6 100644
--- a/Source/cmMakefile.cxx
+++ b/Source/cmMakefile.cxx
@@ -1963,6 +1963,8 @@ void cmMakefile::AddCacheDefinition(const std::string& name, const char* value,
   }
   this->GetCMakeInstance()->AddCacheEntry(name, value, doc, type);
   // if there was a definition then remove it
+  // The method cmFindBase::NormalizeFindResult also apply same workflow.
+  // See #22038 for problems raised by this behavior.
   this->StateSnapshot.RemoveDefinition(name);
 }
 
diff --git a/Source/cmPolicies.h b/Source/cmPolicies.h
index 0ff12d57b1..3ebb17d626 100644
--- a/Source/cmPolicies.h
+++ b/Source/cmPolicies.h
@@ -372,7 +372,11 @@ class cmMakefile;
          3, 21, 0, cmPolicies::WARN)                                          \
   SELECT(POLICY, CMP0124,                                                     \
          "foreach() loop variables are only available in the loop scope.", 3, \
-         21, 0, cmPolicies::WARN)
+         21, 0, cmPolicies::WARN)                                             \
+  SELECT(POLICY, CMP0125,                                                     \
+         "find_(path|file|library|program) have consistent behavior for "     \
+         "cache variables.",                                                  \
+         3, 21, 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/Source/cmake.cxx b/Source/cmake.cxx
index 61234971ba..5440984315 100644
--- a/Source/cmake.cxx
+++ b/Source/cmake.cxx
@@ -157,7 +157,8 @@ static void cmWarnUnusedCliWarning(const std::string& variable, int /*unused*/,
 #endif
 
 cmake::cmake(Role role, cmState::Mode mode)
-  : FileTimeCache(cm::make_unique<cmFileTimeCache>())
+  : CMakeWorkingDirectory(cmSystemTools::GetCurrentWorkingDirectory())
+  , FileTimeCache(cm::make_unique<cmFileTimeCache>())
 #ifndef CMAKE_BOOTSTRAP
   , VariableWatch(cm::make_unique<cmVariableWatch>())
 #endif
diff --git a/Source/cmake.h b/Source/cmake.h
index 9b29098ff6..e845662911 100644
--- a/Source/cmake.h
+++ b/Source/cmake.h
@@ -194,6 +194,14 @@ public:
   //@}
 
   /**
+   * Working directory at CMake launch
+   */
+  std::string const& GetCMakeWorkingDirectory() const
+  {
+    return this->CMakeWorkingDirectory;
+  }
+
+  /**
    * Handle a command line invocation of cmake.
    */
   int Run(const std::vector<std::string>& args)
@@ -628,6 +636,7 @@ protected:
   void GenerateGraphViz(const std::string& fileName) const;
 
 private:
+  std::string CMakeWorkingDirectory;
   ProgressCallbackType ProgressCallback;
   WorkingMode CurrentWorkingMode = NORMAL_MODE;
   bool DebugOutput = false;