ENH: fix library ordering stuff to use a topological sort

Source/cmOrderLinkDirectories.cxx

@@ -52,7 +52,7 @@ void cmOrderLinkDirectories::FindLibrariesInSeachPaths()
       {
       if(lib->second.Path != *dir)
         {
-        if(LibraryInDirectory(dir->c_str(), lib->second.File.c_str()))
+        if(this->LibraryInDirectory(dir->c_str(), lib->second.File.c_str()))
           {
           m_LibraryToDirectories[lib->second.FullPath].push_back(*dir);
           }
@@ -120,80 +120,68 @@ void cmOrderLinkDirectories::PrepareLinkTargets()
 }
 
 //-------------------------------------------------------------------
-bool cmOrderLinkDirectories::CanBeBefore(const cmStdString& d1,
+bool cmOrderLinkDirectories::FindPathNotInDirectoryToAfterList(
-                                         const cmStdString& d2)
+  cmStdString& path)
 {
-  if(m_DirectoryToAfterList.count(d2) == 0)
+  for(std::map<cmStdString, std::vector<cmStdString> >::iterator i
+        = m_DirectoryToAfterList.begin();
+      i != m_DirectoryToAfterList.end(); ++i)
     {
-    return true;
+    const cmStdString& p = i->first;
-    }
+    bool found = false;
-  std::vector<cmStdString>& d2dirs = m_DirectoryToAfterList[d2];
+    for(std::map<cmStdString, std::vector<cmStdString> >::iterator j 
-  // is d1 in the d2's list of directories that d2 must be before
+          = m_DirectoryToAfterList.begin(); j != m_DirectoryToAfterList.end() 
-  // if so, then d1 can not come before d2
+          && !found; ++j)
-  for(std::vector<cmStdString>::iterator i = d2dirs.begin();
-      i != d2dirs.end(); ++i)
-    {
-    if(*i == d1)
       {
-      return false;
+      if(j != i)
+        {
+        found = (std::find(j->second.begin(), j->second.end(), p) != j->second.end());
+        }
+      }
+    if(!found)
+      {
+      path = p;
+      m_DirectoryToAfterList.erase(i);
+      return true;
       }
     }
-  return true;
+  return false;
 }
 
-// This is a stl function object used to sort
-// the vector of library paths.  It returns true
-// if left directory can be before right directory (no swap).
-// It also checks for the impossible case of two libraries and
-// two directories that have both libraries.
-struct cmOrderLinkDirectoriesCompare
-  : public std::binary_function <cmStdString, cmStdString, bool> 
-{
-  cmOrderLinkDirectoriesCompare() 
-  {
-    This = 0;
-  }
-  bool operator()(
-                  const cmStdString& left, 
-                  const cmStdString& right
-                  ) const
-  {
-    bool LBeforeR= This->CanBeBefore(left, right);
-    bool RBeforeL= This->CanBeBefore(right, left);
-
-    if ( !LBeforeR && !RBeforeL )
-      {
-      // check for the case when both libraries have to come
-      // before each other 
-      This->AddImpossible(right, left);
-      }
-    if ( LBeforeR == RBeforeL )
-      {
-      return strcmp(left.c_str(), right.c_str()) < 0;
-      }
-    return LBeforeR;
-  }
-  cmOrderLinkDirectories* This;
-};
-
-//-------------------------------------------------------------------
-void cmOrderLinkDirectories::AddImpossible(const cmStdString& d1,
-                                           const cmStdString& d2)
-{
-  m_ImposibleDirectories.insert(d1);
-  m_ImposibleDirectories.insert(d2);
-}
 
 //-------------------------------------------------------------------
 void cmOrderLinkDirectories::OrderPaths(std::vector<cmStdString>&
                                         orderedPaths)
 {
-  cmOrderLinkDirectoriesCompare comp;
+  cmStdString path;
-  comp.This = this;
+  // This is a topological sort implementation
-  // for some reason when cmake is run on InsightApplication 
+  // One at a time find paths that are not in any other paths after list
-  // if std::sort is used here cmake crashes, but stable_sort works
+  // and put them into the orderedPaths vector in that order
-  //
+  // FindPathNotInDirectoryToAfterList removes the path from the
-  std::sort(orderedPaths.begin(), orderedPaths.end(), comp);
+  // m_DirectoryToAfterList once it is found
+  while(this->FindPathNotInDirectoryToAfterList(path))
+    {
+    orderedPaths.push_back(path);
+    }
+  // at this point if there are still paths in m_DirectoryToAfterList
+  // then there is a cycle and we are stuck
+  if(m_DirectoryToAfterList.size())
+    {
+    for(std::map<cmStdString, std::vector<cmStdString> >::iterator i
+          = m_DirectoryToAfterList.begin();
+        i != m_DirectoryToAfterList.end(); ++i)
+      {
+      m_ImposibleDirectories.insert(i->first);
+      }
+    // if it failed, then fall back to original order
+    orderedPaths.clear();
+    for(std::set<cmStdString>::iterator dir = m_LinkPathSet.begin();
+        dir != m_LinkPathSet.end(); ++dir)
+      {
+      orderedPaths.push_back(*dir);
+      }
+    
+    }
 }
 
 //-------------------------------------------------------------------
@@ -204,9 +192,11 @@ void cmOrderLinkDirectories::SetLinkInformation(const cmTarget& target,
 {
     // collect the search paths from the target into paths set
   const std::vector<std::string>& searchPaths = target.GetLinkDirectories();
+  std::vector<cmStdString> empty;
   for(std::vector<std::string>::const_iterator p = searchPaths.begin();
       p != searchPaths.end(); ++p)
     {
+    m_DirectoryToAfterList[*p] = empty;
     m_LinkPathSet.insert(*p);
     }
   // collect the link items from the target and put it into libs
@@ -284,12 +274,6 @@ bool cmOrderLinkDirectories::DetermineLibraryPathOrder()
     }
   this->FindIndividualLibraryOrders();
   m_SortedSearchPaths.clear();
-  for(std::set<cmStdString>::iterator i = m_LinkPathSet.begin();
-      i != m_LinkPathSet.end(); ++i)
-    {
-    m_SortedSearchPaths.push_back(*i);
-    }
-  
   this->OrderPaths(m_SortedSearchPaths);
   // now turn libfoo.a into foo and foo.a into foo
   // This will prepare the link items for -litem 
@@ -310,7 +294,7 @@ std::string cmOrderLinkDirectories::GetWarnings()
     {
     warning += "Directory: ";
     warning += *i;
-    warning += " contains ";
+    warning += " contains:\n";
     std::map<cmStdString, std::vector<cmStdString> >::iterator j;
     for(j = m_LibraryToDirectories.begin(); 
         j != m_LibraryToDirectories.end(); ++j)
@@ -323,6 +307,7 @@ std::string cmOrderLinkDirectories::GetWarnings()
         warning += "\n";
         }
       }
+    warning += "\n";
     }
   warning += "\n";
   return warning;

Source/cmOrderLinkDirectories.h

@@ -91,10 +91,7 @@ private:
   void PrintMap(const char* name,
                 std::map<cmStdString, std::vector<cmStdString> >& m);
   void OrderPaths(std::vector<cmStdString>& paths);
-  bool CanBeBefore(const cmStdString& d1,
+  bool FindPathNotInDirectoryToAfterList(cmStdString& path);
-                   const cmStdString& d2);
-  void AddImpossible(const cmStdString& ,
-                     const cmStdString& );
 private:
   // map from library to directories that it is in other than its full path
   std::map<cmStdString, std::vector<cmStdString> > m_LibraryToDirectories;

Tests/Complex/Executable/CMakeLists.txt

@@ -81,3 +81,9 @@ SOURCE_GROUP(A_GROUP ".cxx")
 SOURCE_GROUP(B_GROUP REGULAR_EXPRESSION "cxx")
 SOURCE_GROUP(C_GROUP FILES complex.cxx)
 
+FILE(WRITE ${Complex_BINARY_DIR}/A/libA.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/A/libC.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/B/libB.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/B/libA.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/C/libC.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/C/libB.a "test")

Tests/Complex/Executable/complex.cxx

@@ -11,6 +11,8 @@ extern "C" {
 #include "cmStandardIncludes.h"
 #include "cmSystemTools.h"
 #include "cmDynamicLoader.h"
+#include "cmSystemTools.h"
+#include "cmOrderLinkDirectories.h"
 
 int cm_passed = 0;
 int cm_failed = 0;
@@ -21,6 +23,77 @@ This is a problem. Looks like ADD_DEFINITIONS and REMOVE_DEFINITIONS does not wo
 // Here is a stupid function that tries to use std::string methods
 // so that the dec cxx compiler will instantiate the stuff that
 // we are using from the CMakeLib library....
+bool TestLibraryOrder(bool shouldFail)
+{ 
+  std::string Adir = std::string(BINARY_DIR) + std::string("/A");
+  std::string Bdir = std::string(BINARY_DIR) + std::string("/B");
+  std::string Cdir = std::string(BINARY_DIR) + std::string("/C");
+  
+  if(!shouldFail)
+    {
+    std::string rm = Bdir;
+    rm += "/libA.a";
+    cmSystemTools::RemoveFile(rm.c_str());
+    }
+  cmTarget target;
+  target.AddLinkDirectory(Adir.c_str());
+  target.AddLinkDirectory(Bdir.c_str());
+  target.AddLinkDirectory(Cdir.c_str());
+  target.AddLinkDirectory("/lib/extra/stuff");
+  
+  Adir += "/libA.a";
+  Bdir += "/libB.a";
+  Cdir += "/libC.a";
+  
+  target.AddLinkLibrary(Adir.c_str(), cmTarget::GENERAL);
+  target.AddLinkLibrary(Bdir.c_str(), cmTarget::GENERAL);
+  target.AddLinkLibrary(Cdir.c_str(), cmTarget::GENERAL);
+  target.AddLinkLibrary("-lm", cmTarget::GENERAL);
+  std::vector<cmStdString> sortedpaths;
+  std::vector<cmStdString> linkItems;
+  cmOrderLinkDirectories orderLibs;
+  orderLibs.AddLinkExtension(".so");
+  orderLibs.AddLinkExtension(".a");
+  orderLibs.SetLinkInformation(target, cmTarget::GENERAL, "A");
+  bool ret = orderLibs.DetermineLibraryPathOrder();
+  orderLibs.GetLinkerInformation(sortedpaths, linkItems);
+  std::cerr << "Sorted Link Paths:\n";
+  for(std::vector<cmStdString>::iterator i = sortedpaths.begin();
+      i != sortedpaths.end(); ++i)
+    {
+    std::cerr << *i << "\n";
+    }
+  std::cerr << "Link Items: \n";
+  for(std::vector<cmStdString>::iterator i = linkItems.begin();
+      i != linkItems.end(); ++i)
+    {
+    std::cerr << *i << "\n";
+    }
+  if(!(linkItems[0] == "A" && 
+       linkItems[1] == "B" && 
+       linkItems[2] == "C" && 
+       linkItems[3] == "-lm" ))
+    {
+    std::cerr << "fail because link items should be A B C -lm and the are not\n";
+    return shouldFail;
+    }
+  
+     
+  // if this is not the fail test then the order should be f B C A
+  if(!shouldFail)
+    {
+    if(!(sortedpaths[0][sortedpaths[0].size()-1] == 'f' &&
+         sortedpaths[1][sortedpaths[1].size()-1] == 'B' &&
+         sortedpaths[2][sortedpaths[2].size()-1] == 'C' &&
+         sortedpaths[3][sortedpaths[3].size()-1] == 'A' ))
+      {
+      std::cerr << "fail because order should be /lib/extra/stuff B C A and it is not\n";
+      return false;
+      }
+    }
+  
+  return ret;
+}
 
 void ForceStringUse()
 {
@@ -927,6 +1000,28 @@ int main()
   cmPassed("CMake SET CACHE FORCE");
 #endif
 
+  // first run with shouldFail = true, this will
+  // run with A B C as set by the CMakeList.txt file.
+  if(!TestLibraryOrder(true))
+    {
+    cmPassed("CMake cmOrderLinkDirectories worked.");
+    }
+  else
+    {
+    cmFailed("CMake cmOrderLinkDirectories failed to fail when given an impossible set of paths.");
+    }
+  // next run with shouldPass = true, this will 
+  // run with B/libA.a removed and should create the order
+  // B C A
+  if(TestLibraryOrder(false))
+    {
+    cmPassed("CMake cmOrderLinkDirectories worked.");
+    }
+  else
+    {
+    cmFailed("CMake cmOrderLinkDirectories failed.");
+    }
+  
   // ----------------------------------------------------------------------
   // Summary
 
@@ -936,6 +1031,5 @@ int main()
     std::cout << "Failed: " << cm_failed << "\n";
     return cm_failed;
     }
-
   return 0;
 }

Tests/ComplexOneConfig/Executable/CMakeLists.txt

@@ -81,3 +81,9 @@ SOURCE_GROUP(A_GROUP ".cxx")
 SOURCE_GROUP(B_GROUP REGULAR_EXPRESSION "cxx")
 SOURCE_GROUP(C_GROUP FILES complex.cxx)
 
+FILE(WRITE ${Complex_BINARY_DIR}/A/libA.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/A/libC.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/B/libB.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/B/libA.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/C/libC.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/C/libB.a "test")

Tests/ComplexOneConfig/Executable/complex.cxx

@@ -11,6 +11,8 @@ extern "C" {
 #include "cmStandardIncludes.h"
 #include "cmSystemTools.h"
 #include "cmDynamicLoader.h"
+#include "cmSystemTools.h"
+#include "cmOrderLinkDirectories.h"
 
 int cm_passed = 0;
 int cm_failed = 0;
@@ -21,6 +23,77 @@ This is a problem. Looks like ADD_DEFINITIONS and REMOVE_DEFINITIONS does not wo
 // Here is a stupid function that tries to use std::string methods
 // so that the dec cxx compiler will instantiate the stuff that
 // we are using from the CMakeLib library....
+bool TestLibraryOrder(bool shouldFail)
+{ 
+  std::string Adir = std::string(BINARY_DIR) + std::string("/A");
+  std::string Bdir = std::string(BINARY_DIR) + std::string("/B");
+  std::string Cdir = std::string(BINARY_DIR) + std::string("/C");
+  
+  if(!shouldFail)
+    {
+    std::string rm = Bdir;
+    rm += "/libA.a";
+    cmSystemTools::RemoveFile(rm.c_str());
+    }
+  cmTarget target;
+  target.AddLinkDirectory(Adir.c_str());
+  target.AddLinkDirectory(Bdir.c_str());
+  target.AddLinkDirectory(Cdir.c_str());
+  target.AddLinkDirectory("/lib/extra/stuff");
+  
+  Adir += "/libA.a";
+  Bdir += "/libB.a";
+  Cdir += "/libC.a";
+  
+  target.AddLinkLibrary(Adir.c_str(), cmTarget::GENERAL);
+  target.AddLinkLibrary(Bdir.c_str(), cmTarget::GENERAL);
+  target.AddLinkLibrary(Cdir.c_str(), cmTarget::GENERAL);
+  target.AddLinkLibrary("-lm", cmTarget::GENERAL);
+  std::vector<cmStdString> sortedpaths;
+  std::vector<cmStdString> linkItems;
+  cmOrderLinkDirectories orderLibs;
+  orderLibs.AddLinkExtension(".so");
+  orderLibs.AddLinkExtension(".a");
+  orderLibs.SetLinkInformation(target, cmTarget::GENERAL, "A");
+  bool ret = orderLibs.DetermineLibraryPathOrder();
+  orderLibs.GetLinkerInformation(sortedpaths, linkItems);
+  std::cerr << "Sorted Link Paths:\n";
+  for(std::vector<cmStdString>::iterator i = sortedpaths.begin();
+      i != sortedpaths.end(); ++i)
+    {
+    std::cerr << *i << "\n";
+    }
+  std::cerr << "Link Items: \n";
+  for(std::vector<cmStdString>::iterator i = linkItems.begin();
+      i != linkItems.end(); ++i)
+    {
+    std::cerr << *i << "\n";
+    }
+  if(!(linkItems[0] == "A" && 
+       linkItems[1] == "B" && 
+       linkItems[2] == "C" && 
+       linkItems[3] == "-lm" ))
+    {
+    std::cerr << "fail because link items should be A B C -lm and the are not\n";
+    return shouldFail;
+    }
+  
+     
+  // if this is not the fail test then the order should be f B C A
+  if(!shouldFail)
+    {
+    if(!(sortedpaths[0][sortedpaths[0].size()-1] == 'f' &&
+         sortedpaths[1][sortedpaths[1].size()-1] == 'B' &&
+         sortedpaths[2][sortedpaths[2].size()-1] == 'C' &&
+         sortedpaths[3][sortedpaths[3].size()-1] == 'A' ))
+      {
+      std::cerr << "fail because order should be /lib/extra/stuff B C A and it is not\n";
+      return false;
+      }
+    }
+  
+  return ret;
+}
 
 void ForceStringUse()
 {
@@ -927,6 +1000,28 @@ int main()
   cmPassed("CMake SET CACHE FORCE");
 #endif
 
+  // first run with shouldFail = true, this will
+  // run with A B C as set by the CMakeList.txt file.
+  if(!TestLibraryOrder(true))
+    {
+    cmPassed("CMake cmOrderLinkDirectories worked.");
+    }
+  else
+    {
+    cmFailed("CMake cmOrderLinkDirectories failed to fail when given an impossible set of paths.");
+    }
+  // next run with shouldPass = true, this will 
+  // run with B/libA.a removed and should create the order
+  // B C A
+  if(TestLibraryOrder(false))
+    {
+    cmPassed("CMake cmOrderLinkDirectories worked.");
+    }
+  else
+    {
+    cmFailed("CMake cmOrderLinkDirectories failed.");
+    }
+  
   // ----------------------------------------------------------------------
   // Summary
 
@@ -936,6 +1031,5 @@ int main()
     std::cout << "Failed: " << cm_failed << "\n";
     return cm_failed;
     }
-
   return 0;
 }

Tests/ComplexRelativePaths/Executable/CMakeLists.txt

@@ -81,3 +81,9 @@ SOURCE_GROUP(A_GROUP ".cxx")
 SOURCE_GROUP(B_GROUP REGULAR_EXPRESSION "cxx")
 SOURCE_GROUP(C_GROUP FILES complex.cxx)
 
+FILE(WRITE ${Complex_BINARY_DIR}/A/libA.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/A/libC.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/B/libB.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/B/libA.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/C/libC.a "test")
+FILE(WRITE ${Complex_BINARY_DIR}/C/libB.a "test")

Tests/ComplexRelativePaths/Executable/complex.cxx

@@ -11,6 +11,8 @@ extern "C" {
 #include "cmStandardIncludes.h"
 #include "cmSystemTools.h"
 #include "cmDynamicLoader.h"
+#include "cmSystemTools.h"
+#include "cmOrderLinkDirectories.h"
 
 int cm_passed = 0;
 int cm_failed = 0;
@@ -21,6 +23,77 @@ This is a problem. Looks like ADD_DEFINITIONS and REMOVE_DEFINITIONS does not wo
 // Here is a stupid function that tries to use std::string methods
 // so that the dec cxx compiler will instantiate the stuff that
 // we are using from the CMakeLib library....
+bool TestLibraryOrder(bool shouldFail)
+{ 
+  std::string Adir = std::string(BINARY_DIR) + std::string("/A");
+  std::string Bdir = std::string(BINARY_DIR) + std::string("/B");
+  std::string Cdir = std::string(BINARY_DIR) + std::string("/C");
+  
+  if(!shouldFail)
+    {
+    std::string rm = Bdir;
+    rm += "/libA.a";
+    cmSystemTools::RemoveFile(rm.c_str());
+    }
+  cmTarget target;
+  target.AddLinkDirectory(Adir.c_str());
+  target.AddLinkDirectory(Bdir.c_str());
+  target.AddLinkDirectory(Cdir.c_str());
+  target.AddLinkDirectory("/lib/extra/stuff");
+  
+  Adir += "/libA.a";
+  Bdir += "/libB.a";
+  Cdir += "/libC.a";
+  
+  target.AddLinkLibrary(Adir.c_str(), cmTarget::GENERAL);
+  target.AddLinkLibrary(Bdir.c_str(), cmTarget::GENERAL);
+  target.AddLinkLibrary(Cdir.c_str(), cmTarget::GENERAL);
+  target.AddLinkLibrary("-lm", cmTarget::GENERAL);
+  std::vector<cmStdString> sortedpaths;
+  std::vector<cmStdString> linkItems;
+  cmOrderLinkDirectories orderLibs;
+  orderLibs.AddLinkExtension(".so");
+  orderLibs.AddLinkExtension(".a");
+  orderLibs.SetLinkInformation(target, cmTarget::GENERAL, "A");
+  bool ret = orderLibs.DetermineLibraryPathOrder();
+  orderLibs.GetLinkerInformation(sortedpaths, linkItems);
+  std::cerr << "Sorted Link Paths:\n";
+  for(std::vector<cmStdString>::iterator i = sortedpaths.begin();
+      i != sortedpaths.end(); ++i)
+    {
+    std::cerr << *i << "\n";
+    }
+  std::cerr << "Link Items: \n";
+  for(std::vector<cmStdString>::iterator i = linkItems.begin();
+      i != linkItems.end(); ++i)
+    {
+    std::cerr << *i << "\n";
+    }
+  if(!(linkItems[0] == "A" && 
+       linkItems[1] == "B" && 
+       linkItems[2] == "C" && 
+       linkItems[3] == "-lm" ))
+    {
+    std::cerr << "fail because link items should be A B C -lm and the are not\n";
+    return shouldFail;
+    }
+  
+     
+  // if this is not the fail test then the order should be f B C A
+  if(!shouldFail)
+    {
+    if(!(sortedpaths[0][sortedpaths[0].size()-1] == 'f' &&
+         sortedpaths[1][sortedpaths[1].size()-1] == 'B' &&
+         sortedpaths[2][sortedpaths[2].size()-1] == 'C' &&
+         sortedpaths[3][sortedpaths[3].size()-1] == 'A' ))
+      {
+      std::cerr << "fail because order should be /lib/extra/stuff B C A and it is not\n";
+      return false;
+      }
+    }
+  
+  return ret;
+}
 
 void ForceStringUse()
 {
@@ -927,6 +1000,28 @@ int main()
   cmPassed("CMake SET CACHE FORCE");
 #endif
 
+  // first run with shouldFail = true, this will
+  // run with A B C as set by the CMakeList.txt file.
+  if(!TestLibraryOrder(true))
+    {
+    cmPassed("CMake cmOrderLinkDirectories worked.");
+    }
+  else
+    {
+    cmFailed("CMake cmOrderLinkDirectories failed to fail when given an impossible set of paths.");
+    }
+  // next run with shouldPass = true, this will 
+  // run with B/libA.a removed and should create the order
+  // B C A
+  if(TestLibraryOrder(false))
+    {
+    cmPassed("CMake cmOrderLinkDirectories worked.");
+    }
+  else
+    {
+    cmFailed("CMake cmOrderLinkDirectories failed.");
+    }
+  
   // ----------------------------------------------------------------------
   // Summary
 
@@ -936,6 +1031,5 @@ int main()
     std::cout << "Failed: " << cm_failed << "\n";
     return cm_failed;
     }
-
   return 0;
 }