target_link_libraries: Fix out-of-dir linking of a list of targets

In a case like

    target_link_libraries(targetInOtherDir PUBLIC "$<1:a;b>")

then all entries in the list need to be looked up in the caller's
scope.  Previously our `::@(directory-id)` suffix would apply only
to the last entry.  Instead surround the entire entry by a pair
`::@(directory-id);...;::@` so that the `::@` syntax can encode
a directory lookup scope change evaluated as the list is processed.

Fixes: #20204

Help/prop_tgt/LINK_LIBRARIES_INDIRECTION.txt

@@ -1,9 +1,9 @@
 .. note::
   A call to :command:`target_link_libraries(<target> ...)` may update this
   property on ``<target>``.  If ``<target>`` was not created in the same
-  directory as the call then :command:`target_link_libraries` will add a
-  suffix of the form ``::@(directory-id)`` to each entry, where the
-  ``::@`` is a separator and the ``(directory-id)`` is unspecified.
+  directory as the call then :command:`target_link_libraries` will wrap each
+  entry with the form ``::@(directory-id);...;::@``, where the ``::@`` is
+  literal and the ``(directory-id)`` is unspecified.
   This tells the generators that the named libraries must be looked up in
   the scope of the caller rather than in the scope in which the
   ``<target>`` was created.  Valid directory ids are stripped on export

Source/cmExportFileGenerator.cxx

@@ -14,6 +14,7 @@
 #include "cmComputeLinkInformation.h"
 #include "cmGeneratedFileStream.h"
 #include "cmGeneratorTarget.h"
+#include "cmGlobalGenerator.h"
 #include "cmLinkItem.h"
 #include "cmLocalGenerator.h"
 #include "cmMakefile.h"
@@ -640,6 +641,9 @@ void cmExportFileGenerator::ResolveTargetsInGeneratorExpressions(
   std::string sep;
   input.clear();
   for (std::string& li : parts) {
+    if (cmHasLiteralPrefix(li, CMAKE_DIRECTORY_ID_SEP)) {
+      continue;
+    }
     if (cmGeneratorExpression::Find(li) == std::string::npos) {
       this->AddTargetNamespace(li, target, missingTargets);
     } else {

Source/cmGeneratorTarget.cxx

@@ -5406,16 +5406,39 @@ void cmGeneratorTarget::ReportPropertyOrigin(
                                                          areport);
 }
 
+bool cmGeneratorTarget::IsLinkLookupScope(std::string const& n,
+                                          cmLocalGenerator const*& lg) const
+{
+  if (cmHasLiteralPrefix(n, CMAKE_DIRECTORY_ID_SEP)) {
+    cmDirectoryId const dirId = n.substr(sizeof(CMAKE_DIRECTORY_ID_SEP) - 1);
+    if (dirId.String.empty()) {
+      lg = this->LocalGenerator;
+      return true;
+    }
+    if (cmLocalGenerator const* otherLG =
+          this->GlobalGenerator->FindLocalGenerator(dirId)) {
+      lg = otherLG;
+      return true;
+    }
+  }
+  return false;
+}
+
 void cmGeneratorTarget::LookupLinkItems(std::vector<std::string> const& names,
                                         cmListFileBacktrace const& bt,
                                         std::vector<cmLinkItem>& items) const
 {
+  cmLocalGenerator const* lg = this->LocalGenerator;
   for (std::string const& n : names) {
+    if (this->IsLinkLookupScope(n, lg)) {
+      continue;
+    }
+
     std::string name = this->CheckCMP0004(n);
     if (name == this->GetName() || name.empty()) {
       continue;
     }
-    items.push_back(this->ResolveLinkItem(name, bt));
+    items.push_back(this->ResolveLinkItem(name, bt, lg));
   }
 }
 
@@ -5424,6 +5447,7 @@ void cmGeneratorTarget::ExpandLinkItems(
   cmGeneratorTarget const* headTarget, bool usage_requirements_only,
   std::vector<cmLinkItem>& items, bool& hadHeadSensitiveCondition) const
 {
+  // Keep this logic in sync with ComputeLinkImplementationLibraries.
   cmGeneratorExpression ge;
   cmGeneratorExpressionDAGChecker dagChecker(this, prop, nullptr, nullptr);
   // The $<LINK_ONLY> expression may be in a link interface to specify private
@@ -6499,6 +6523,7 @@ void cmGeneratorTarget::ComputeLinkImplementationLibraries(
   const std::string& config, cmOptionalLinkImplementation& impl,
   cmGeneratorTarget const* head) const
 {
+  cmLocalGenerator const* lg = this->LocalGenerator;
   cmStringRange entryRange = this->Target->GetLinkImplementationEntries();
   cmBacktraceRange btRange = this->Target->GetLinkImplementationBacktraces();
   cmBacktraceRange::const_iterator btIt = btRange.begin();
@@ -6507,6 +6532,7 @@ void cmGeneratorTarget::ComputeLinkImplementationLibraries(
                                      end = entryRange.end();
        le != end; ++le, ++btIt) {
     std::vector<std::string> llibs;
+    // Keep this logic in sync with ExpandLinkItems.
     cmGeneratorExpressionDAGChecker dagChecker(this, "LINK_LIBRARIES", nullptr,
                                                nullptr);
     cmGeneratorExpression ge(*btIt);
@@ -6519,6 +6545,10 @@ void cmGeneratorTarget::ComputeLinkImplementationLibraries(
     }
 
     for (std::string const& lib : llibs) {
+      if (this->IsLinkLookupScope(lib, lg)) {
+        continue;
+      }
+
       // Skip entries that resolve to the target itself or are empty.
       std::string name = this->CheckCMP0004(lib);
       if (name == this->GetName() || name.empty()) {
@@ -6553,7 +6583,7 @@ void cmGeneratorTarget::ComputeLinkImplementationLibraries(
       }
 
       // The entry is meant for this configuration.
-      impl.Libraries.emplace_back(this->ResolveLinkItem(name, *btIt),
+      impl.Libraries.emplace_back(this->ResolveLinkItem(name, *btIt, lg),
                                   evaluated != *le);
     }
 
@@ -6590,38 +6620,16 @@ void cmGeneratorTarget::ComputeLinkImplementationLibraries(
 cmGeneratorTarget::TargetOrString cmGeneratorTarget::ResolveTargetReference(
   std::string const& name) const
 {
-  cmLocalGenerator const* lg = this->LocalGenerator;
-  std::string const* lookupName = &name;
-
-  // When target_link_libraries() is called with a LHS target that is
-  // not created in the calling directory it adds a directory id suffix
-  // that we can use to look up the calling directory.  It is that scope
-  // in which the item name is meaningful.  This case is relatively rare
-  // so we allocate a separate string only when the directory id is present.
-  std::string::size_type pos = name.find(CMAKE_DIRECTORY_ID_SEP);
-  std::string plainName;
-  if (pos != std::string::npos) {
-    // We will look up the plain name without the directory id suffix.
-    plainName = name.substr(0, pos);
-
-    // We will look up in the scope of the directory id.
-    // If we do not recognize the id then leave the original
-    // syntax in place to produce an indicative error later.
-    cmDirectoryId const dirId =
-      name.substr(pos + sizeof(CMAKE_DIRECTORY_ID_SEP) - 1);
-    if (cmLocalGenerator const* otherLG =
-          this->GlobalGenerator->FindLocalGenerator(dirId)) {
-      lg = otherLG;
-      lookupName = &plainName;
-    }
-  }
+  return this->ResolveTargetReference(name, this->LocalGenerator);
+}
 
+cmGeneratorTarget::TargetOrString cmGeneratorTarget::ResolveTargetReference(
+  std::string const& name, cmLocalGenerator const* lg) const
+{
   TargetOrString resolved;
 
-  if (cmGeneratorTarget* tgt = lg->FindGeneratorTargetToUse(*lookupName)) {
+  if (cmGeneratorTarget* tgt = lg->FindGeneratorTargetToUse(name)) {
     resolved.Target = tgt;
-  } else if (lookupName == &plainName) {
-    resolved.String = std::move(plainName);
   } else {
     resolved.String = name;
   }
@@ -6632,7 +6640,14 @@ cmGeneratorTarget::TargetOrString cmGeneratorTarget::ResolveTargetReference(
 cmLinkItem cmGeneratorTarget::ResolveLinkItem(
   std::string const& name, cmListFileBacktrace const& bt) const
 {
-  TargetOrString resolved = this->ResolveTargetReference(name);
+  return this->ResolveLinkItem(name, bt, this->LocalGenerator);
+}
+
+cmLinkItem cmGeneratorTarget::ResolveLinkItem(std::string const& name,
+                                              cmListFileBacktrace const& bt,
+                                              cmLocalGenerator const* lg) const
+{
+  TargetOrString resolved = this->ResolveTargetReference(name, lg);
 
   if (!resolved.Target) {
     return cmLinkItem(resolved.String, bt);

Source/cmGeneratorTarget.h

@@ -378,9 +378,14 @@ public:
     cmGeneratorTarget* Target = nullptr;
   };
   TargetOrString ResolveTargetReference(std::string const& name) const;
+  TargetOrString ResolveTargetReference(std::string const& name,
+                                        cmLocalGenerator const* lg) const;
 
   cmLinkItem ResolveLinkItem(std::string const& name,
                              cmListFileBacktrace const& bt) const;
+  cmLinkItem ResolveLinkItem(std::string const& name,
+                             cmListFileBacktrace const& bt,
+                             cmLocalGenerator const* lg) const;
 
   // Compute the set of languages compiled by the target.  This is
   // computed every time it is called because the languages can change
@@ -919,6 +924,9 @@ private:
 
   std::unordered_set<std::string> UnityBatchedSourceFiles;
 
+  bool IsLinkLookupScope(std::string const& n,
+                         cmLocalGenerator const*& lg) const;
+
   void ExpandLinkItems(std::string const& prop, std::string const& value,
                        std::string const& config,
                        const cmGeneratorTarget* headTarget,

Source/cmTarget.cxx

@@ -937,14 +937,7 @@ cmTarget::LinkLibraryVectorType const& cmTarget::GetOriginalLinkLibraries()
   return impl->OriginalLinkLibraries;
 }
 
-void cmTarget::AddLinkLibrary(cmMakefile& mf, const std::string& lib,
-                              cmTargetLinkLibraryType llt)
-{
-  this->AddLinkLibrary(mf, lib, lib, llt);
-}
-
 void cmTarget::AddLinkLibrary(cmMakefile& mf, std::string const& lib,
-                              std::string const& libRef,
                               cmTargetLinkLibraryType llt)
 {
   cmTarget* tgt = mf.FindTargetToUse(lib);
@@ -953,13 +946,13 @@ void cmTarget::AddLinkLibrary(cmMakefile& mf, std::string const& lib,
 
     const std::string libName =
       (isNonImportedTarget && llt != GENERAL_LibraryType)
-      ? targetNameGenex(libRef)
-      : libRef;
+      ? targetNameGenex(lib)
+      : lib;
     this->AppendProperty("LINK_LIBRARIES",
                          this->GetDebugGeneratorExpressions(libName, llt));
   }
 
-  if (cmGeneratorExpression::Find(lib) != std::string::npos || lib != libRef ||
+  if (cmGeneratorExpression::Find(lib) != std::string::npos ||
       (tgt &&
        (tgt->GetType() == cmStateEnums::INTERFACE_LIBRARY ||
         tgt->GetType() == cmStateEnums::OBJECT_LIBRARY)) ||

Source/cmTarget.h

@@ -110,10 +110,8 @@ public:
   //! Clear the dependency information recorded for this target, if any.
   void ClearDependencyInformation(cmMakefile& mf);
 
-  void AddLinkLibrary(cmMakefile& mf, const std::string& lib,
-                      cmTargetLinkLibraryType llt);
   void AddLinkLibrary(cmMakefile& mf, std::string const& lib,
-                      std::string const& libRef, cmTargetLinkLibraryType llt);
+                      cmTargetLinkLibraryType llt);
 
   enum TLLSignature
   {

Source/cmTargetLinkLibrariesCommand.cxx

@@ -4,6 +4,8 @@
 
 #include <cstring>
 #include <sstream>
+#include <unordered_set>
+#include <utility>
 
 #include "cmExecutionStatus.h"
 #include "cmGeneratorExpression.h"
@@ -41,9 +43,16 @@ struct TLL
   bool WarnRemoteInterface = false;
   bool RejectRemoteLinking = false;
   bool EncodeRemoteReference = false;
+  std::string DirectoryId;
+  std::unordered_set<std::string> Props;
+
   TLL(cmMakefile& mf, cmTarget* target);
+  ~TLL();
+
   bool HandleLibrary(ProcessingState currentProcessingState,
                      const std::string& lib, cmTargetLinkLibraryType llt);
+  void AppendProperty(std::string const& prop, std::string const& value);
+  void AffectsProperty(std::string const& prop);
 };
 
 } // namespace
@@ -343,6 +352,10 @@ TLL::TLL(cmMakefile& mf, cmTarget* target)
         break;
     }
   }
+  if (this->EncodeRemoteReference) {
+    cmDirectoryId const dirId = this->Makefile.GetDirectoryId();
+    this->DirectoryId = cmStrCat(CMAKE_DIRECTORY_ID_SEP, dirId.String);
+  }
 }
 
 bool TLL::HandleLibrary(ProcessingState currentProcessingState,
@@ -413,25 +426,6 @@ bool TLL::HandleLibrary(ProcessingState currentProcessingState,
     }
   }
 
-  std::string libRef;
-  if (this->EncodeRemoteReference && !cmSystemTools::FileIsFullPath(lib)) {
-    // This is a library name added by a caller that is not in the
-    // same directory as the target was created.  Add a suffix to
-    // the name to tell ResolveLinkItem to look up the name in the
-    // caller's directory.
-    cmDirectoryId const dirId = this->Makefile.GetDirectoryId();
-    // FIXME: The "lib" may be a genex with a list inside it.
-    // After expansion this id will only attach to the last entry,
-    // or may attach to an empty string!  We will need another way
-    // to encode this that can apply to a whole list.  See issue #20204.
-    libRef = lib + CMAKE_DIRECTORY_ID_SEP + dirId.String;
-  } else {
-    // This is an absolute path or a library name added by a caller
-    // in the same directory as the target was created.  We can use
-    // the original name directly.
-    libRef = lib;
-  }
-
   // Handle normal case where the command was called with another keyword than
   // INTERFACE / LINK_INTERFACE_LIBRARIES or none at all. (The "LINK_LIBRARIES"
   // property of the target on the LHS shall be populated.)
@@ -467,7 +461,8 @@ bool TLL::HandleLibrary(ProcessingState currentProcessingState,
           "executables with the ENABLE_EXPORTS property set."));
     }
 
-    this->Target->AddLinkLibrary(this->Makefile, lib, libRef, llt);
+    this->AffectsProperty("LINK_LIBRARIES");
+    this->Target->AddLinkLibrary(this->Makefile, lib, llt);
   }
 
   if (this->WarnRemoteInterface) {
@@ -493,12 +488,12 @@ bool TLL::HandleLibrary(ProcessingState currentProcessingState,
     if (this->Target->GetType() == cmStateEnums::STATIC_LIBRARY ||
         this->Target->GetType() == cmStateEnums::OBJECT_LIBRARY) {
       std::string configLib =
-        this->Target->GetDebugGeneratorExpressions(libRef, llt);
+        this->Target->GetDebugGeneratorExpressions(lib, llt);
       if (cmGeneratorExpression::IsValidTargetName(lib) ||
           cmGeneratorExpression::Find(lib) != std::string::npos) {
         configLib = "$<LINK_ONLY:" + configLib + ">";
       }
-      this->Target->AppendProperty("INTERFACE_LINK_LIBRARIES", configLib);
+      this->AppendProperty("INTERFACE_LINK_LIBRARIES", configLib);
     }
     return true;
   }
@@ -506,9 +501,8 @@ bool TLL::HandleLibrary(ProcessingState currentProcessingState,
   // Handle general case where the command was called with another keyword than
   // PRIVATE / LINK_PRIVATE or none at all. (The "INTERFACE_LINK_LIBRARIES"
   // property of the target on the LHS shall be populated.)
-  this->Target->AppendProperty(
-    "INTERFACE_LINK_LIBRARIES",
-    this->Target->GetDebugGeneratorExpressions(libRef, llt));
+  this->AppendProperty("INTERFACE_LINK_LIBRARIES",
+                       this->Target->GetDebugGeneratorExpressions(lib, llt));
 
   // Stop processing if called without any keyword.
   if (currentProcessingState == ProcessingLinkLibraries) {
@@ -540,12 +534,12 @@ bool TLL::HandleLibrary(ProcessingState currentProcessingState,
       // Put in the DEBUG configuration interfaces.
       for (std::string const& dc : debugConfigs) {
         prop = cmStrCat("LINK_INTERFACE_LIBRARIES_", dc);
-        this->Target->AppendProperty(prop, libRef);
+        this->AppendProperty(prop, lib);
       }
     }
     if (llt == OPTIMIZED_LibraryType || llt == GENERAL_LibraryType) {
       // Put in the non-DEBUG configuration interfaces.
-      this->Target->AppendProperty("LINK_INTERFACE_LIBRARIES", libRef);
+      this->AppendProperty("LINK_INTERFACE_LIBRARIES", lib);
 
       // Make sure the DEBUG configuration interfaces exist so that the
       // general one will not be used as a fall-back.
@@ -560,4 +554,29 @@ bool TLL::HandleLibrary(ProcessingState currentProcessingState,
   return true;
 }
 
+void TLL::AppendProperty(std::string const& prop, std::string const& value)
+{
+  this->AffectsProperty(prop);
+  this->Target->AppendProperty(prop, value);
+}
+
+void TLL::AffectsProperty(std::string const& prop)
+{
+  if (!this->EncodeRemoteReference) {
+    return;
+  }
+  // Add a wrapper to the expression to tell LookupLinkItems to look up
+  // names in the caller's directory.
+  if (this->Props.insert(prop).second) {
+    this->Target->AppendProperty(prop, this->DirectoryId);
+  }
+}
+
+TLL::~TLL()
+{
+  for (std::string const& prop : this->Props) {
+    this->Target->AppendProperty(prop, CMAKE_DIRECTORY_ID_SEP);
+  }
+}
+
 } // namespace

Tests/CMakeCommands/target_link_libraries/SubDirB/CMakeLists.txt

@@ -4,8 +4,9 @@ add_executable(SubDirB SubDirB.c)
 # be visible to the directory in which TopDir is defined.
 target_link_libraries(TopDir PUBLIC debug SameNameImported optimized SameNameImported)
 
-#FIXME: Demonstrate known issue #20204.
-#target_link_libraries(TopDir PUBLIC "$<1:SameNameImported;SameNameImported>")
+# Link to a list of targets imported in this directory that would not
+# normally be visible to the directory in which TopDir is defined.
+target_link_libraries(TopDir PUBLIC "$<1:SameNameImported;SameNameImported>")
 
 # Link SubDirA to a target imported in this directory that has the same
 # name as a target imported in SubDirA's directory.  We verify when

Tests/ExportImport/Export/SubDirLinkA/CMakeLists.txt

@@ -1,6 +1,6 @@
 add_library(SubDirLinkAImported IMPORTED INTERFACE)
 target_compile_definitions(SubDirLinkAImported INTERFACE DEF_SubDirLinkAImportedForExport)
 
-target_link_libraries(TopDirLib PUBLIC SubDirLinkAImported)
+target_link_libraries(TopDirLib PUBLIC debug "$<1:SubDirLinkAImported;SubDirLinkAImported>" optimized "$<1:SubDirLinkAImported;SubDirLinkAImported>")
 
 add_library(SubDirLinkA STATIC SubDirLinkA.c)

Tests/RunCMake/target_link_libraries/CMP0079-iface-NEW-stdout.txt

@@ -1 +1 @@
--- INTERFACE_LINK_LIBRARIES='foo::@\([Xx0-9A-Fa-f]+\)'
+-- INTERFACE_LINK_LIBRARIES='::@\([Xx0-9A-Fa-f]+\);\$<\$<CONFIG:DEBUG>:\$<1:foo;foo>>;\$<\$<NOT:\$<CONFIG:DEBUG>>:\$<1:foo;foo>>;::@'

Tests/RunCMake/target_link_libraries/CMP0079-iface-OLD-stdout.txt

@@ -1 +1 @@
--- INTERFACE_LINK_LIBRARIES='foo'
+-- INTERFACE_LINK_LIBRARIES='\$<\$<CONFIG:DEBUG>:\$<1:foo;foo>>;\$<\$<NOT:\$<CONFIG:DEBUG>>:\$<1:foo;foo>>'

Tests/RunCMake/target_link_libraries/CMP0079-iface-WARN-stderr.txt

@@ -10,7 +10,25 @@
   is not created in this directory.  For compatibility with older versions of
   CMake, link library
 
-    foo
+    \$<1:foo;foo>
+
+  will be looked up in the directory in which the target was created rather
+  than in this calling directory.
+This warning is for project developers.  Use -Wno-dev to suppress it.
++
+CMake Warning \(dev\) at CMP0079-iface/CMakeLists.txt:[0-9]+ \(target_link_libraries\):
+  Policy CMP0079 is not set: target_link_libraries allows use with targets in
+  other directories.  Run "cmake --help-policy CMP0079" for policy details.
+  Use the cmake_policy command to set the policy and suppress this warning.
+
+  Target
+
+    top
+
+  is not created in this directory.  For compatibility with older versions of
+  CMake, link library
+
+    \$<1:foo;foo>
 
   will be looked up in the directory in which the target was created rather
   than in this calling directory.

Tests/RunCMake/target_link_libraries/CMP0079-iface-WARN-stdout.txt

@@ -1 +1 @@
--- INTERFACE_LINK_LIBRARIES='foo'
+-- INTERFACE_LINK_LIBRARIES='\$<\$<CONFIG:DEBUG>:\$<1:foo;foo>>;\$<\$<NOT:\$<CONFIG:DEBUG>>:\$<1:foo;foo>>'

Tests/RunCMake/target_link_libraries/CMP0079-iface/CMakeLists.txt

@@ -1 +1 @@
-target_link_libraries(top INTERFACE foo)
+target_link_libraries(top INTERFACE debug "$<1:foo;foo>" optimized "$<1:foo;foo>")

Tests/RunCMake/target_link_libraries/CMP0079-link-NEW-bogus-stderr.txt

@@ -1,5 +1,5 @@
 ^CMake Error at CMP0079-link-NEW-bogus.cmake:[0-9]+ \(add_executable\):
-  Target "top" links to target "foo::@\(0xdeadbeef\)" but the target was not
+  Target "top" links to target "::@\(0xdeadbeef\)" but the target was not
   found.  Perhaps a find_package\(\) call is missing for an IMPORTED target, or
   an ALIAS target is missing\?
 Call Stack \(most recent call first\):

Tests/RunCMake/target_link_libraries/CMP0079-link-NEW-bogus.cmake

@@ -3,4 +3,4 @@ cmake_policy(SET CMP0079 NEW)
 enable_language(C)
 
 add_executable(top empty.c)
-set_property(TARGET top APPEND PROPERTY LINK_LIBRARIES "foo::@(0xdeadbeef)")
+set_property(TARGET top APPEND PROPERTY LINK_LIBRARIES "::@(0xdeadbeef);foo;::@")

Tests/RunCMake/target_link_libraries/CMP0079-link-NEW-stdout.txt

@@ -1 +1 @@
--- LINK_LIBRARIES='foo::@\([Xx0-9A-Fa-f]+\)'
+-- LINK_LIBRARIES='::@\([Xx0-9A-Fa-f]+\);foo;::@'