Merge topic 'file-set-install-fix'

d71b59a4f7 install(TARGETS): Don't ignore non-extant file sets

Acked-by: Kitware Robot <[email protected]>
Tested-by: buildbot <[email protected]>
Reviewed-by: Ben Boeckel <[email protected]>
Merge-request: !10878

Help/release/dev/install-nonextant-file-set.rst

@@ -0,0 +1,7 @@
+install-nonextant-file-set
+--------------------------
+
+* The :command:`install(TARGETS)` command no longer ignores file sets which
+  haven't been defined at the point it is called. The ordering of
+  :command:`target_sources(FILE_SET)` and ``install(TARGETS)`` is no longer
+  semantically relevant.

Source/cmExportInstallCMakeConfigGenerator.cxx

@@ -286,7 +286,8 @@ std::string cmExportInstallCMakeConfigGenerator::GetFileSetDirectories(
     gte->Makefile->GetGeneratorConfigs(cmMakefile::IncludeEmptyConfig);
 
   cmGeneratorExpression ge(*gte->Makefile->GetCMakeInstance());
-  auto cge = ge.Parse(te->FileSetGenerators.at(fileSet)->GetDestination());
+  auto cge =
+    ge.Parse(te->FileSetGenerators.at(fileSet->GetName())->GetDestination());
 
   for (auto const& config : configs) {
     auto unescapedDest = cge->Evaluate(gte->LocalGenerator, config, gte);
@@ -334,8 +335,8 @@ std::string cmExportInstallCMakeConfigGenerator::GetFileSetFiles(
   auto directoryEntries = fileSet->CompileDirectoryEntries();
 
   cmGeneratorExpression destGe(*gte->Makefile->GetCMakeInstance());
-  auto destCge =
-    destGe.Parse(te->FileSetGenerators.at(fileSet)->GetDestination());
+  auto destCge = destGe.Parse(
+    te->FileSetGenerators.at(fileSet->GetName())->GetDestination());
 
   for (auto const& config : configs) {
     auto directories = fileSet->EvaluateDirectoryEntries(

Source/cmExportSet.cxx

@@ -40,9 +40,8 @@ bool cmExportSet::Compute(cmLocalGenerator* lg)
       tgtExport->Target->Target->GetAllInterfaceFileSets();
     auto const fileSetInTargetExport =
       [&tgtExport, lg](std::string const& fileSetName) -> bool {
-      auto* fileSet = tgtExport->Target->Target->GetFileSet(fileSetName);
-
-      if (!tgtExport->FileSetGenerators.count(fileSet)) {
+      if (tgtExport->FileSetGenerators.find(fileSetName) ==
+          tgtExport->FileSetGenerators.end()) {
         lg->IssueMessage(MessageType::FATAL_ERROR,
                          cmStrCat("File set \"", fileSetName,
                                   "\" is listed in interface file sets of ",

Source/cmFileAPICodemodel.cxx

@@ -1071,7 +1071,7 @@ Json::Value DirectoryObject::DumpInstaller(cmInstallGenerator* gen)
     installer["type"] = "fileSet";
     installer["destination"] = installFileSet->GetDestination(this->Config);
 
-    auto* fileSet = installFileSet->GetFileSet();
+    auto const* fileSet = installFileSet->GetFileSet();
     auto* target = installFileSet->GetTarget();
 
     auto dirCges = fileSet->CompileDirectoryEntries();

Source/cmInstallCommand.cxx

@@ -24,7 +24,6 @@
 #include "cmExecutionStatus.h"
 #include "cmExperimental.h"
 #include "cmExportSet.h"
-#include "cmFileSet.h"
 #include "cmGeneratorExpression.h"
 #include "cmGlobalGenerator.h"
 #include "cmInstallAndroidMKExportGenerator.h"
@@ -198,15 +197,15 @@ std::unique_ptr<cmInstallFilesGenerator> CreateInstallFilesGenerator(
 }
 
 std::unique_ptr<cmInstallFileSetGenerator> CreateInstallFileSetGenerator(
-  Helper& helper, cmTarget& target, cmFileSet* fileSet,
-  std::string const& destination, cmInstallCommandArguments const& args)
+  Helper& helper, cmTarget& target, cmFileSetDestinations dests,
+  cmInstallCommandFileSetArguments const& args)
 {
   cmInstallGenerator::MessageLevel message =
     cmInstallGenerator::SelectMessageLevel(helper.Makefile);
   return cm::make_unique<cmInstallFileSetGenerator>(
-    target.GetName(), fileSet, destination, args.GetPermissions(),
-    args.GetConfigurations(), args.GetComponent(), message,
-    args.GetExcludeFromAll(), args.GetOptional(),
+    target.GetName(), args.GetFileSet(), std::move(dests),
+    args.GetPermissions(), args.GetConfigurations(), args.GetComponent(),
+    message, args.GetExcludeFromAll(), args.GetOptional(),
     helper.Makefile->GetBacktrace());
 }
 
@@ -809,7 +808,7 @@ bool HandleTargetsMode(std::vector<std::string> const& args,
         te->RuntimeGenerator = runtimeGenerator.get();
         te->ObjectsGenerator = objectGenerator.get();
         for (auto const& gen : fileSetGenerators) {
-          te->FileSetGenerators[gen->GetFileSet()] = gen.get();
+          te->FileSetGenerators[gen->GetFileSetName()] = gen.get();
         }
         te->CxxModuleBmiGenerator = cxxModuleBmiGenerator.get();
         target.AddInstallIncludeDirectories(
@@ -1145,31 +1144,12 @@ bool HandleTargetsMode(std::vector<std::string> const& args,
 
     if (!namelinkOnly) {
       for (std::size_t i = 0; i < fileSetArgs.size(); i++) {
-        if (auto* fileSet = target.GetFileSet(fileSetArgs[i].GetFileSet())) {
-          cmList interfaceFileSetEntries{ target.GetSafeProperty(
-            cmTarget::GetInterfaceFileSetsPropertyName(fileSet->GetType())) };
-          if (std::find(interfaceFileSetEntries.begin(),
-                        interfaceFileSetEntries.end(),
-                        fileSetArgs[i].GetFileSet()) !=
-              interfaceFileSetEntries.end()) {
-            std::string destination;
-            if (fileSet->GetType() == "HEADERS"_s) {
-              destination = helper.GetIncludeDestination(&fileSetArgs[i]);
-            } else {
-              destination = fileSetArgs[i].GetDestination();
-              if (destination.empty()) {
-                status.SetError(cmStrCat(
-                  "TARGETS given no FILE_SET DESTINATION for target \"",
-                  target.GetName(), "\" file set \"", fileSet->GetName(),
-                  "\"."));
-                return false;
-              }
-            }
-            fileSetGenerators.push_back(CreateInstallFileSetGenerator(
-              helper, target, fileSet, destination, fileSetArgs[i]));
-            installsFileSet[i] = true;
-          }
-        }
+        cmFileSetDestinations dests;
+        dests.Headers = helper.GetIncludeDestination(&fileSetArgs[i]);
+        dests.CXXModules = fileSetArgs[i].GetDestination();
+        fileSetGenerators.push_back(CreateInstallFileSetGenerator(
+          helper, target, std::move(dests), fileSetArgs[i]));
+        installsFileSet[i] = true;
       }
     }
 

Source/cmInstallFileSetGenerator.cxx

@@ -2,29 +2,38 @@
    file LICENSE.rst or https://cmake.org/licensing for details.  */
 #include "cmInstallFileSetGenerator.h"
 
+#include <algorithm>
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
 
+#include <cm/string_view>
+#include <cmext/string_view>
+
 #include "cmFileSet.h"
 #include "cmGeneratorExpression.h"
+#include "cmGeneratorTarget.h"
 #include "cmGlobalGenerator.h"
 #include "cmInstallType.h"
+#include "cmList.h"
 #include "cmListFileCache.h"
 #include "cmLocalGenerator.h"
+#include "cmMessageType.h"
 #include "cmStringAlgorithms.h"
+#include "cmTarget.h"
 
 cmInstallFileSetGenerator::cmInstallFileSetGenerator(
-  std::string targetName, cmFileSet* fileSet, std::string const& dest,
+  std::string targetName, std::string fileSetName, cmFileSetDestinations dests,
   std::string file_permissions, std::vector<std::string> const& configurations,
   std::string const& component, MessageLevel message, bool exclude_from_all,
   bool optional, cmListFileBacktrace backtrace)
-  : cmInstallGenerator(dest, configurations, component, message,
+  : cmInstallGenerator("", configurations, component, message,
                        exclude_from_all, false, std::move(backtrace))
   , TargetName(std::move(targetName))
-  , FileSet(fileSet)
+  , FileSetName(std::move(fileSetName))
   , FilePermissions(std::move(file_permissions))
+  , FileSetDestinations(std::move(dests))
   , Optional(optional)
 {
   this->ActionsPerConfig = true;
@@ -44,6 +53,39 @@ bool cmInstallFileSetGenerator::Compute(cmLocalGenerator* lg)
       lg->GetGlobalGenerator()->FindGeneratorTarget(this->TargetName);
   }
 
+  auto const& target = *this->Target->Target;
+  this->FileSet = target.GetFileSet(this->FileSetName);
+
+  if (!this->FileSet) {
+    // No file set of the given name was ever provided for this target, nothing
+    // for this generator to do
+    return true;
+  }
+
+  cmList interfaceFileSetEntries{ target.GetSafeProperty(
+    cmTarget::GetInterfaceFileSetsPropertyName(this->FileSet->GetType())) };
+
+  if (std::find(interfaceFileSetEntries.begin(), interfaceFileSetEntries.end(),
+                this->FileSetName) != interfaceFileSetEntries.end()) {
+    if (this->FileSet->GetType() == "HEADERS"_s) {
+      this->Destination = this->FileSetDestinations.Headers;
+    } else {
+      this->Destination = this->FileSetDestinations.CXXModules;
+    }
+  } else {
+    // File set of the given name was provided but it's private, so give up
+    this->FileSet = nullptr;
+    return true;
+  }
+
+  if (this->Destination.empty()) {
+    lg->IssueMessage(MessageType::FATAL_ERROR,
+                     cmStrCat("File set \"", this->FileSetName,
+                              "\" installed by target \"", this->TargetName,
+                              "\" has no destination."));
+    return false;
+  }
+
   return true;
 }
 
@@ -57,6 +99,11 @@ std::string cmInstallFileSetGenerator::GetDestination(
 void cmInstallFileSetGenerator::GenerateScriptForConfig(
   std::ostream& os, std::string const& config, Indent indent)
 {
+
+  if (!this->FileSet) {
+    return;
+  }
+
   for (auto const& dirEntry : this->CalculateFilesPerDir(config)) {
     std::string destSub;
     if (!dirEntry.first.empty()) {

Source/cmInstallFileSetGenerator.h

@@ -14,11 +14,17 @@ class cmFileSet;
 class cmListFileBacktrace;
 class cmLocalGenerator;
 
+struct cmFileSetDestinations
+{
+  std::string Headers;
+  std::string CXXModules;
+};
+
 class cmInstallFileSetGenerator : public cmInstallGenerator
 {
 public:
-  cmInstallFileSetGenerator(std::string targetName, cmFileSet* fileSet,
-                            std::string const& dest,
+  cmInstallFileSetGenerator(std::string targetName, std::string fileSetName,
+                            cmFileSetDestinations dests,
                             std::string file_permissions,
                             std::vector<std::string> const& configurations,
                             std::string const& component, MessageLevel message,
@@ -31,7 +37,8 @@ public:
   std::string GetDestination(std::string const& config) const;
   std::string GetDestination() const { return this->Destination; }
   bool GetOptional() const { return this->Optional; }
-  cmFileSet* GetFileSet() const { return this->FileSet; }
+  std::string GetFileSetName() const { return this->FileSetName; }
+  cmFileSet const* GetFileSet() const { return this->FileSet; };
   cmGeneratorTarget* GetTarget() const { return this->Target; }
 
 protected:
@@ -41,8 +48,10 @@ protected:
 private:
   std::string TargetName;
   cmLocalGenerator* LocalGenerator;
-  cmFileSet* const FileSet;
+  cmFileSet const* FileSet;
+  std::string const FileSetName;
   std::string const FilePermissions;
+  cmFileSetDestinations FileSetDestinations;
   bool const Optional;
   cmGeneratorTarget* Target;
 

Source/cmInstallGenerator.h

@@ -93,7 +93,7 @@ protected:
                        TweakMethod const& tweak);
 
   // Information shared by most generator types.
-  std::string const Destination;
+  std::string Destination;
   std::string const Component;
   MessageLevel const Message;
   bool const ExcludeFromAll;

Source/cmTargetExport.h

@@ -7,7 +7,6 @@
 #include <map>
 #include <string>
 
-class cmFileSet;
 class cmGeneratorTarget;
 class cmInstallCxxModuleBmiGenerator;
 class cmInstallFileSetGenerator;
@@ -33,7 +32,7 @@ public:
   cmInstallTargetGenerator* FrameworkGenerator;
   cmInstallTargetGenerator* BundleGenerator;
   cmInstallFilesGenerator* HeaderGenerator;
-  std::map<cmFileSet*, cmInstallFileSetGenerator*> FileSetGenerators;
+  std::map<std::string, cmInstallFileSetGenerator*> FileSetGenerators;
   cmInstallCxxModuleBmiGenerator* CxxModuleBmiGenerator;
   ///@}
 

Tests/RunCMake/target_sources/FileSetInstallFirstExport.cmake

@@ -0,0 +1,22 @@
+enable_language(C)
+
+add_library(lib1 STATIC lib1.c)
+
+install(TARGETS lib1 EXPORT export FILE_SET HEADERS FILE_SET a FILE_SET b FILE_SET c DESTINATION include/dir FILE_SET d FILE_SET e FILE_SET f DESTINATION include/$<IF:$<CONFIG:Debug>,debug,release> FILE_SET g FILE_SET dir3 DESTINATION include/dir3)
+install(EXPORT export FILE export.cmake NAMESPACE install:: DESTINATION lib/cmake)
+export(EXPORT export FILE export.cmake NAMESPACE export::)
+
+target_sources(lib1
+  PUBLIC FILE_SET HEADERS BASE_DIRS ${CMAKE_CURRENT_SOURCE_DIR} FILES error.c
+  PRIVATE FILE_SET a TYPE HEADERS BASE_DIRS ${CMAKE_CURRENT_SOURCE_DIR} FILES h1.h
+  PUBLIC FILE_SET b TYPE HEADERS BASE_DIRS ${CMAKE_CURRENT_SOURCE_DIR} FILES h2.h
+  INTERFACE FILE_SET c TYPE HEADERS BASE_DIRS "$<1:${CMAKE_CURRENT_SOURCE_DIR}/dir>" FILES "$<1:dir/dir.h>"
+  INTERFACE FILE_SET d TYPE HEADERS BASE_DIRS FILES "${CMAKE_CURRENT_SOURCE_DIR}/$<IF:$<CONFIG:Debug>,debug,release>/empty.h"
+  INTERFACE FILE_SET e TYPE HEADERS BASE_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/$<IF:$<CONFIG:Debug>,debug,release>" FILES "${CMAKE_CURRENT_SOURCE_DIR}/$<IF:$<CONFIG:Debug>,debug,release>/empty2.h"
+  INTERFACE FILE_SET f TYPE HEADERS BASE_DIRS "${CMAKE_CURRENT_SOURCE_DIR}" FILES "${CMAKE_CURRENT_SOURCE_DIR}/empty3.h"
+  INTERFACE FILE_SET g TYPE HEADERS BASE_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/dir1" "${CMAKE_CURRENT_SOURCE_DIR}/dir2" FILES "${CMAKE_CURRENT_SOURCE_DIR}/dir1/file1.h" "${CMAKE_CURRENT_SOURCE_DIR}/dir2/file2.h"
+  INTERFACE FILE_SET dir3 TYPE HEADERS BASE_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/dir3" FILES dir3/dir3.h
+  )
+
+add_library(lib2 STATIC lib2.c)
+target_link_libraries(lib2 PRIVATE lib1)

Tests/RunCMake/target_sources/RunCMakeTest.cmake

@@ -61,6 +61,12 @@ run_cmake(FileSetFileNoExist)
 unset(RunCMake_TEST_OPTIONS)
 
 function(run_export_import name)
+  if(ARGV1)
+    set(importname ${ARGV1})
+  else()
+    set(importname ${name})
+  endif()
+
   if(RunCMake_GENERATOR_IS_MULTI_CONFIG)
     set(_config_options "-DCMAKE_CONFIGURATION_TYPES=Debug\\\\;Release")
   else()
@@ -81,14 +87,14 @@ function(run_export_import name)
   endif()
   unset(RunCMake_TEST_OPTIONS)
 
-  set(RunCMake_TEST_BINARY_DIR "${RunCMake_BINARY_DIR}/${name}Import-build")
+  set(RunCMake_TEST_BINARY_DIR "${RunCMake_BINARY_DIR}/${importname}Import-build")
   unset(RunCMake_TEST_OPTIONS)
   file(REMOVE_RECURSE "${RunCMake_TEST_BINARY_DIR}")
   file(MAKE_DIRECTORY "${RunCMake_TEST_BINARY_DIR}")
-  run_cmake(${name}Import)
-  run_cmake_command(${name}Import-build ${CMAKE_COMMAND} --build . --config Debug)
+  run_cmake(${importname}Import)
+  run_cmake_command(${importname}Import-build ${CMAKE_COMMAND} --build . --config Debug)
   if(RunCMake_GENERATOR_IS_MULTI_CONFIG)
-    run_cmake_command(${name}Import-build ${CMAKE_COMMAND} --build . --config Release)
+    run_cmake_command(${importname}Import-build ${CMAKE_COMMAND} --build . --config Release)
   endif()
 
   unset(RunCMake_TEST_BINARY_DIR)
@@ -96,4 +102,5 @@ function(run_export_import name)
 endfunction()
 
 run_export_import(FileSet)
+run_export_import(FileSetInstallFirst FileSet)
 run_export_import(FileSetAbsoluteInstallIncludeDir)