Merge topic 'restore-install-late-framework'

f64099cf5e Merge branch 'backport-restore-install-late-framework'
95210d027a macOS: Restore compatibility for setting FRAMEWORK after install()
d9dd68cb60 macOS: Restore compatibility for setting FRAMEWORK after install()

Acked-by: Kitware Robot <kwrobot@kitware.com>
Acked-by: Kyle Edwards <kyle.edwards@kitware.com>
Merge-request: !2878

Source/cmInstallTargetGenerator.cxx

@@ -19,6 +19,7 @@
 #include "cmStateTypes.h"
 #include "cmSystemTools.h"
 #include "cmTarget.h"
+#include "cmake.h"
 
 cmInstallTargetGenerator::cmInstallTargetGenerator(
   std::string targetName, const char* dest, bool implib,
@@ -209,8 +210,34 @@ void cmInstallTargetGenerator::GenerateScriptForConfig(
       // An import library looks like a static library.
       type = cmInstallType_STATIC_LIBRARY;
     } else if (this->Target->IsFrameworkOnApple()) {
-      // There is a bug in cmInstallCommand if this fails.
-      assert(this->NamelinkMode == NamelinkModeNone);
+      // FIXME: In principle we should be able to
+      //   assert(this->NamelinkMode == NamelinkModeNone);
+      // but since the current install() command implementation checks
+      // the FRAMEWORK property immediately instead of delaying until
+      // generate time, it is possible for project code to set the
+      // property after calling install().  In such a case, the install()
+      // command will use the LIBRARY code path and create two install
+      // generators, one for the namelink component (NamelinkModeOnly)
+      // and one for the primary artifact component (NamelinkModeSkip).
+      // Historically this was not diagnosed and resulted in silent
+      // installation of a framework to the LIBRARY destination.
+      // Retain that behavior and warn about the case.
+      switch (this->NamelinkMode) {
+        case NamelinkModeNone:
+          // Normal case.
+          break;
+        case NamelinkModeOnly:
+          // Assume the NamelinkModeSkip instance will warn and install.
+          return;
+        case NamelinkModeSkip: {
+          std::string e = "Target '" + this->Target->GetName() +
+            "' was changed to a FRAMEWORK sometime after install().  "
+            "This may result in the wrong install DESTINATION.  "
+            "Set the FRAMEWORK property earlier.";
+          this->Target->GetGlobalGenerator()->GetCMakeInstance()->IssueMessage(
+            MessageType::AUTHOR_WARNING, e, this->GetBacktrace());
+        } break;
+      }
 
       // Install the whole framework directory.
       type = cmInstallType_DIRECTORY;

Tests/RunCMake/Framework/InstallBeforeFramework-stderr.txt

@@ -0,0 +1,7 @@
+^CMake Warning \(dev\) at InstallBeforeFramework.cmake:4 \(install\):
+  Target 'foo' was changed to a FRAMEWORK sometime after install\(\).  This may
+  result in the wrong install DESTINATION.  Set the FRAMEWORK property
+  earlier.
+Call Stack \(most recent call first\):
+  CMakeLists.txt:[0-9]+ \(include\)
+This warning is for project developers.  Use -Wno-dev to suppress it.

Tests/RunCMake/Framework/InstallBeforeFramework.cmake

@@ -0,0 +1,5 @@
+enable_language(C)
+
+add_library(foo SHARED foo.c)
+install(TARGETS foo LIBRARY DESTINATION lib)
+set_property(TARGET foo PROPERTY FRAMEWORK TRUE)

Tests/RunCMake/Framework/RunCMakeTest.cmake

@@ -1,5 +1,7 @@
 include(RunCMake)
 
+run_cmake(InstallBeforeFramework)
+
 function(framework_layout_test Name Toolchain Type)
   set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/${Toolchain}${Type}FrameworkLayout-build)
   set(RunCMake_TEST_NO_CLEAN 1)