- bug fix where paths weren't being output when LIB_OUT_PATH *isn't* used

- test case for above mentioned bug
- more comments. Comments are good.

4 files changed, 98 insertions, 39 deletions

diff --git a/Source/CMakeLists.txt b/Source/CMakeLists.txt
index 589c52d..56713da 100644
--- a/Source/CMakeLists.txt
+++ b/Source/CMakeLists.txt
@@ -163,11 +163,18 @@ IF(BUILD_TESTING)
       ${CMake_BINARY_DIR}/Tests/Wrapping/bin
       TestDriverTest subdir/test3)
 
-    ADD_TEST(dependency ${CMake_BINARY_DIR}/Source/cmaketest 
+    ADD_TEST(dependency_w_libout ${CMake_BINARY_DIR}/Source/cmaketest 
       ${CMake_SOURCE_DIR}/Tests/Dependency 
-      ${CMake_BINARY_DIR}/Tests/Dependency
+      ${CMake_BINARY_DIR}/Tests/Dependency/WithLibOut
       exec
-      ${CMake_BINARY_DIR}/Tests/Dependency/Exec
+      ${CMake_BINARY_DIR}/Tests/Dependency/WithLibOut/Exec
+      Dependency CMAKE_ARGS -DLIBRARY_OUTPUT_PATH:PATH=${CMake_BINARY_DIR}/Tests/Dependency/WithLibOut/Lib)
+
+    ADD_TEST(dependency_wo_lib_out ${CMake_BINARY_DIR}/Source/cmaketest 
+      ${CMake_SOURCE_DIR}/Tests/Dependency 
+      ${CMake_BINARY_DIR}/Tests/Dependency/WOLibOut
+      exec
+      ${CMake_BINARY_DIR}/Tests/Dependency/WOLibOut/Exec
       Dependency)
 
   ENDIF (DART_ROOT)
diff --git a/Source/cmTarget.cxx b/Source/cmTarget.cxx
index d3f2726..9142f1f 100644
--- a/Source/cmTarget.cxx
+++ b/Source/cmTarget.cxx
@@ -139,6 +139,32 @@ bool cmTarget::HasCxx() const
 void
 cmTarget::AnalyzeLibDependencies( const cmMakefile& mf )
 {
+  // There are two key parts of the dependency analysis: (1)
+  // determining the libraries in the link line, and (2) constructing
+  // the dependency graph for those libraries.
+  //
+  // The latter is done using the cache entries that record the
+  // dependencies of each library.
+  //
+  // The former is a more thorny issue, since it is not clear how to
+  // determine if two libraries listed on the link line refer to the a
+  // single library or not. For example, consider the link "libraries"
+  //    /usr/lib/libtiff.so -ltiff 
+  // Is this one library or two? The solution implemented here is the
+  // simplest (and probably the only practical) one: two libraries are
+  // the same if their "link strings" are identical. Thus, the two
+  // libraries above are considered distinct. This also means that for
+  // dependency analysis to be effective, the CMake user must specify
+  // libraries build by his project without using any linker flags or
+  // file extensions. That is,
+  //    LINK_LIBRARIES( One Two )
+  // instead of
+  //    LINK_LIBRARIES( -lOne ${binarypath}/libTwo.a )
+  // The former is probably what most users would do, but it never
+  // hurts to document the assumptions. :-) Therefore, in the analysis
+  // code, the "canonical name" of a library is simply its name as
+  // given to a LINK_LIBRARIES command.
+
   typedef std::vector< std::string > LinkLine;
 
   // Maps the canonical names to the full objects of m_LinkLibraries.
@@ -146,7 +172,8 @@ cmTarget::AnalyzeLibDependencies( const cmMakefile& mf )
 
   // The unique list of libraries on the orginal link line. They
   // correspond to lib_map keys. However, lib_map will also get
-  // further populated by the dependency analysis.
+  // further populated by the dependency analysis, while this will be
+  // unchanged.
   LinkLine orig_libs;
 
   // The list canonical names in the order they were orginally
@@ -156,6 +183,10 @@ cmTarget::AnalyzeLibDependencies( const cmMakefile& mf )
   // The dependency maps.
   DependencyMap dep_map, dep_map_implicit;
 
+
+  // 1. Determine the list of unique libraries in the original link
+  // line.
+
   LinkLibraries::iterator lib;
   for(lib = m_LinkLibraries.begin(); lib != m_LinkLibraries.end(); ++lib)
     {
@@ -163,7 +194,7 @@ cmTarget::AnalyzeLibDependencies( const cmMakefile& mf )
     // if a variable expands to nothing.
     if (lib->first.size() == 0) continue;
 
-    std::string cname = lib->first;
+    const std::string& cname = lib->first;
     lib_order.push_back( cname );
     if( lib_map.end() == lib_map.find( cname ) )
       {
@@ -172,11 +203,18 @@ cmTarget::AnalyzeLibDependencies( const cmMakefile& mf )
       }
     }
 
+  // 2. Gather the dependencies.
+
+  // If LIBRARY_OUTPUT_PATH is not set, then we must add search paths
+  // for all the new libraries added by the dependency analysis.
+  const char* libOutPath = mf.GetDefinition("LIBRARY_OUTPUT_PATH");
+  bool addLibDirs = (libOutPath==0 || strcmp(libOutPath,"")==0);
+
   // First, get the explicit dependencies for those libraries that
-  // have specified them
+  // have specified them.
   for( LinkLine::iterator i = orig_libs.begin(); i != orig_libs.end(); ++i )
     {
-    this->GatherDependencies( mf, *i, dep_map, lib_map );
+    this->GatherDependencies( mf, *i, dep_map, lib_map, addLibDirs );
     }
 
   // For the rest, get implicit dependencies. A library x depends
@@ -216,7 +254,10 @@ cmTarget::AnalyzeLibDependencies( const cmMakefile& mf )
 	dep_map[ i->first ] = i->second;
 	}
 
-  // Create a new link line order.
+  // 3. Create a new link line, trying as much as possible to keep the
+  // original link line order.
+
+  // Get the link line as canonical names.
   std::set<std::string> done, visited;
   std::vector<std::string> link_line;
   for( LinkLine::iterator i = orig_libs.begin(); i != orig_libs.end(); ++i )
@@ -224,19 +265,14 @@ cmTarget::AnalyzeLibDependencies( const cmMakefile& mf )
     Emit( *i, dep_map, done, visited, link_line );
     }
 
-
-  // If LIBRARY_OUTPUT_PATH is not set, then we must add search paths
-  // for all the new libraries added by the dependency analysis.
-  const char* libOutPath = mf.GetDefinition("LIBRARY_OUTPUT_PATH");
-  bool addLibDirs = (libOutPath==0 || strcmp(libOutPath,"")==0);
+  // Translate the canonical names into internal objects.
   m_LinkLibraries.clear();
   for( std::vector<std::string>::reverse_iterator i = link_line.rbegin();
        i != link_line.rend(); ++i )
     {
     // Some of the libraries in the new link line may not have been in
     // the orginal link line, but were added by the dependency
-    // analysis. For these libraries, we assume the GENERAL type and
-    // add the name of the library.
+    // analysis.
     if( lib_map.find(*i) == lib_map.end() )
       {
       if( addLibDirs )
@@ -317,7 +353,8 @@ void cmTarget::Emit( const std::string& lib,
 void cmTarget::GatherDependencies( const cmMakefile& mf,
                                    const std::string& lib,
                                    DependencyMap& dep_map,
-                                   LibTypeMap& lib_map ) const
+                                   LibTypeMap& lib_map,
+                                   bool addLibDirs )
 {
   // If the library is already in the dependency map, then it has
   // already been fully processed.
@@ -343,7 +380,20 @@ void cmTarget::GatherDependencies( const cmMakefile& mf,
       std::string l = depline.substr( start, end-start );
       if( l.size() != 0 )
         {
-        const std::string cname = l; 
+        if( addLibDirs )
+          {
+          const char* libpath = mf.GetDefinition( l.c_str() );
+          if( libpath )
+            {
+            // Don't add a link directory that is already present.
+              if(std::find(m_LinkDirectories.begin(),
+                           m_LinkDirectories.end(), libpath) == m_LinkDirectories.end())
+                {
+                m_LinkDirectories.push_back(libpath);
+                }
+            }
+          }
+        const std::string& cname = l; 
         std::string linkType = l;
         linkType += "_LINK_TYPE";
         cmTarget::LinkLibraryType llt = cmTarget::GENERAL;
@@ -361,7 +411,7 @@ void cmTarget::GatherDependencies( const cmMakefile& mf,
           }
         lib_map[ cname ] = std::make_pair(l,llt); 
         dep_map[ lib ].insert( cname );
-        GatherDependencies( mf, cname, dep_map, lib_map );
+        GatherDependencies( mf, cname, dep_map, lib_map, addLibDirs );
         }
       start = end+1; // skip the ;
       end = depline.find( ";", start );
diff --git a/Source/cmTarget.h b/Source/cmTarget.h
index 516d16b..0fcf353 100644
--- a/Source/cmTarget.h
+++ b/Source/cmTarget.h
@@ -121,7 +121,7 @@ private:
   typedef std::map< std::string, std::set< std::string > > DependencyMap;
 
   /**
-   * Maps a canonical library name to it's proper type
+   * Maps a library name to its internal structure
    */
   typedef std::map< std::string, std::pair<std::string,LinkLibraryType> > LibTypeMap;
 
@@ -141,11 +141,15 @@ private:
 
   /**
    * Finds the explicit dependencies for \param lib, if they have been
-   * specified, and inserts them into \param dep_map.
+   * specified, and inserts them into \param dep_map. It also adds the
+   * maps from the library names to internal structures for any
+   * libraries introduced by the analysis. \param addLibDirs is true
+   * if paths to newly found libraries should be added to the search
+   * path.
    */
   void GatherDependencies( const cmMakefile& mf, const std::string& lib,
                            DependencyMap& dep_map,
-                           LibTypeMap& lib_map ) const;
+                           LibTypeMap& lib_map, bool addLibDirs );
 
   /**
    * Returns true if lib1 depends on lib2 according to \param
diff --git a/Tests/Dependency/CMakeLists.txt b/Tests/Dependency/CMakeLists.txt
index 3f2bee8..b7debc6 100644
--- a/Tests/Dependency/CMakeLists.txt
+++ b/Tests/Dependency/CMakeLists.txt
@@ -1,27 +1,25 @@
 PROJECT( Dependency )
 
-SET( LIBRARY_OUTPUT_PATH ${Dependency_BINARY_DIR}/Lib )
-SET( CMAKE_ANALYZE_LIB_DEPENDS "ON" )
-
 # There is one executable that depends on eight libraries. The
 # system has the following dependency graph:
 #
-#                    +----------- NoDepC  <----  EXECUTABLE
-#                    |                            |    |
-#                    V                            |    |
-#                                                 |    |
-#                  NoDepA  <----- NoDepB  <-------+    |
-#                                                      |
-#                    ^                                 |
+#                    +----------- NoDepC  <----  EXECUTABLE ---+
+#                    |                            |    |       |
+#                    V                            |    |       |
+#                                                 |    |       |
+#                  NoDepA  <----- NoDepB  <-------+    |       |
+#                                                      |       |
+#                    ^                                 |       V
 #                    |                                 |
-#      One  <------ Four ----->  Two  <----- Five  <---+
-#                                |           
-#       ^           ^  ^         | ^         ^  ^
-#       |           |  +-----+   | |         |  +----+
-#       |           |        |   | |         |       |
-#       +--------- Three  <------+ +------- SixA    SixB
-#                            |                       |
-#                            +-----------------------+
+#      One  <------ Four ----->  Two  <----- Five  <---|----- SixB
+#                                |                     |       |
+#       ^           ^  ^         |  ^         ^        |       |
+#       |           |  +-----+   |   \        |        |       |
+#       |           |        |   |    \       |        |       |
+#       +--------- Three  <------+      --- SixA  <----+       |
+#                            |                                 |
+#                            |                                 |
+#                            +---------------------------------+
 # NoDepA:
 # NoDepB: NoDepA
 # NoDepC: NoDepA