Fix find_package() when <pkg>_DIR is wrong

When <pkg>_DIR is set to an incorrect version we search again and store
the result in the variable, even if it is <pkg>_DIR-NOTFOUND.

There was a bug in the case when the new search does not find anything
and the old value came from a cache entry with UNINITALIZED type.  The
command used to try to load a package configuration file from the last
place searched, and would leave the old wrong value in the entry.  This
commit fixes the behavior to avoid trying to load a missing file and to
set the value to <pkg>_DIR-NOTFOUND as expected.

3 files changed, 51 insertions, 14 deletions

diff --git a/Source/cmFindPackageCommand.cxx b/Source/cmFindPackageCommand.cxx
index 165dbc2..7dbbf6d 100644
--- a/Source/cmFindPackageCommand.cxx
+++ b/Source/cmFindPackageCommand.cxx
@@ -736,7 +736,7 @@ bool cmFindPackageCommand::HandlePackageMode()
     }
 
   // Try to load the config file if the directory is known
-  bool cachedDirectoryOk = false;
+  bool fileFound = false;
   if(!cmSystemTools::IsOff(def))
     {
     // Get the directory from the variable value.
@@ -754,25 +754,30 @@ bool cmFindPackageCommand::HandlePackageMode()
     if (this->FindConfigFile(dir, file))
       {
       this->FileFound = file;
-      cachedDirectoryOk = true;
+      fileFound = true;
       }
     def = this->Makefile->GetDefinition(this->Variable.c_str());
     }
 
   // Search for the config file if it is not already found.
-  if(cmSystemTools::IsOff(def) || !cachedDirectoryOk)
+  if(cmSystemTools::IsOff(def) || !fileFound)
     {
-    this->FindConfig();
+    fileFound = this->FindConfig();
     def = this->Makefile->GetDefinition(this->Variable.c_str());
     }
 
+  // Sanity check.
+  if(fileFound && this->FileFound.empty())
+    {
+    this->Makefile->IssueMessage(
+      cmake::INTERNAL_ERROR, "fileFound is true but FileFound is empty!");
+    fileFound = false;
+    }
+
   // If the directory for the config file was found, try to read the file.
   bool result = true;
   bool found = false;
-  // in the following test FileFound should never be empty if def is valid
-  // but I don't want to put an assert() in there now, in case this still
-  // makes it into 2.6.3
-  if(!cmSystemTools::IsOff(def) && (!this->FileFound.empty()))
+  if(fileFound)
     {
     // Set the version variables before loading the config file.
     // It may override them.
@@ -886,7 +891,7 @@ bool cmFindPackageCommand::HandlePackageMode()
 }
 
 //----------------------------------------------------------------------------
-void cmFindPackageCommand::FindConfig()
+bool cmFindPackageCommand::FindConfig()
 {
   // Compute the set of search prefixes.
   this->ComputePrefixes();
@@ -938,9 +943,11 @@ void cmFindPackageCommand::FindConfig()
     "The directory containing a CMake configuration file for ";
   help += this->Name;
   help += ".";
+  // We force the value since we do not get here if it was already set.
   this->Makefile->AddCacheDefinition(this->Variable.c_str(),
                                      init.c_str(), help.c_str(),
-                                     cmCacheManager::PATH);
+                                     cmCacheManager::PATH, true);
+  return found;
 }
 
 //----------------------------------------------------------------------------
diff --git a/Source/cmFindPackageCommand.h b/Source/cmFindPackageCommand.h
index 139ca9b..63f4111 100644
--- a/Source/cmFindPackageCommand.h
+++ b/Source/cmFindPackageCommand.h
@@ -73,7 +73,7 @@ private:
   void AddFindDefinition(const char* var, const char* val);
   void RestoreFindDefinitions();
   bool HandlePackageMode();
-  void FindConfig();
+  bool FindConfig();
   bool FindPrefixedConfig();
   bool FindFrameworkConfig();
   bool FindAppBundleConfig();
diff --git a/Tests/FindPackageTest/CMakeLists.txt b/Tests/FindPackageTest/CMakeLists.txt
index a62756e..48ca548 100644
--- a/Tests/FindPackageTest/CMakeLists.txt
+++ b/Tests/FindPackageTest/CMakeLists.txt
@@ -37,6 +37,7 @@ FIND_PACKAGE(VersionTestD 1.2.3.4)
 SET(PACKAGES
   foo Foo Bar TFramework Tframework TApp Tapp Special
   VersionedA VersionedB VersionedC VersionedD VersionedE
+  WrongA WrongB WrongC WrongD
   wibbleA wibbleB
   RecursiveA RecursiveB RecursiveC
   )
@@ -67,6 +68,24 @@ FIND_PACKAGE(VersionedC 4.0 EXACT NAMES zot)
 FIND_PACKAGE(VersionedD 1.1 EXACT NAMES Baz)
 FIND_PACKAGE(VersionedE 1.2 EXACT NAMES Baz)
 
+# Test wrong initial path when result is present.
+SET(WrongA_DIR "${VersionedD_DIR}")
+FIND_PACKAGE(WrongA 1.2 EXACT NAMES Baz)
+
+# Test wrong initial cache entry of UNINITIALIZED type when result is present.
+SET(WrongB_DIR "${VersionedD_DIR}" CACHE UNINITIALIZED "Wrong Value" FORCE)
+GET_PROPERTY(type CACHE WrongB_DIR PROPERTY TYPE)
+FIND_PACKAGE(WrongB 1.2 EXACT NAMES Baz)
+
+# Test wrong initial path when result is missing.
+SET(WrongC_DIR "${VersionedD_DIR}")
+FIND_PACKAGE(WrongC 1.3 EXACT QUIET NAMES Baz)
+
+# Test wrong initial cache entry of UNINITIALIZED type when result is missing.
+SET(WrongD_DIR "${VersionedD_DIR}" CACHE UNINITIALIZED "Wrong Value" FORCE)
+GET_PROPERTY(type CACHE WrongD_DIR PROPERTY TYPE)
+FIND_PACKAGE(WrongD 1.3 EXACT QUIET NAMES Baz)
+
 # HINTS should override the system but PATHS should not
 LIST(INSERT CMAKE_SYSTEM_PREFIX_PATH 0 "${CMAKE_CURRENT_SOURCE_DIR}/A")
 FIND_PACKAGE(wibbleA NAMES wibble PATHS B)
@@ -95,6 +114,10 @@ SET(VersionedB_EXPECTED "lib/zot-3.1/zot-config.cmake")
 SET(VersionedC_EXPECTED "lib/cmake/zot-4.0/zot-config.cmake")
 SET(VersionedD_EXPECTED "Baz 1.1/BazConfig.cmake")
 SET(VersionedE_EXPECTED "Baz 1.2/CMake/BazConfig.cmake")
+SET(WrongA_EXPECTED "${VersionedE_EXPECTED}")
+SET(WrongB_EXPECTED "${VersionedE_EXPECTED}")
+SET(WrongC_MISSING "WrongC_DIR-NOTFOUND")
+SET(WrongD_MISSING "WrongD_DIR-NOTFOUND")
 SET(wibbleA_EXPECTED "A/wibble-config.cmake")
 SET(wibbleB_EXPECTED "B/wibble-config.cmake")
 SET(RecursiveA_EXPECTED "lib/RecursiveA/recursivea-config.cmake")
@@ -103,7 +126,14 @@ SET(RecursiveC_EXPECTED "lib/zot-3.1/zot-config.cmake")
 
 # Check the results.
 FOREACH(p ${PACKAGES})
-  IF(${p}_FOUND)
+  IF(DEFINED ${p}_MISSING)
+    # Check and report failure.
+    IF(NOT "${${p}_DIR}" STREQUAL "${${p}_MISSING}")
+      MESSAGE(SEND_ERROR
+        "Package ${p} should have been [${${p}_MISSING}] but "
+        "was [${${p}_DIR}]")
+    ENDIF()
+  ELSEIF(${p}_FOUND)
     # Convert to relative path for comparison to expected location.
     FILE(RELATIVE_PATH REL_${p}_CONFIG "${CMAKE_CURRENT_SOURCE_DIR}"
       "${${p}_CONFIG}")
@@ -119,9 +149,9 @@ FOREACH(p ${PACKAGES})
         "Package ${p} should have been [${${p}_EXPECTED}] but "
         "was [${REL_${p}_CONFIG}]")
     ENDIF(NOT "${REL_${p}_CONFIG}" STREQUAL "${${p}_EXPECTED}")
-  ELSE(${p}_FOUND)
+  ELSE()
     MESSAGE(SEND_ERROR "Package ${p} not found!")
-  ENDIF(${p}_FOUND)
+  ENDIF()
 ENDFOREACH(p)
 
 # Check that version information was extracted.