macOS: Restore compatibility for setting FRAMEWORK after install()

The `FRAMEWORK` target property affects the way the `install()` command
treats the target and so should be set first.  Our implementation
assumed that this was always the case and led to an assertion failure.
Prior to CMake 3.12 this was visible only when using an explicit
`LIBRARY ... NAMELINK_ONLY` option, but commit 0212d7c762 (install: add
NAMELINK_COMPONENT argument, 2018-04-18, v3.12.0-rc1~139^2~3) made
it possible with a simple `LIBRARY DESTINATION`.

Fully supporting out-of-order specification will require non-trivial
refactoring to defer install generator creation to generate time.
For now simply restore the old behavior of installing the framework
to the library destination and warn about the case.

Fixes: #18848

4 files changed, 43 insertions, 2 deletions

diff --git a/Source/cmInstallTargetGenerator.cxx b/Source/cmInstallTargetGenerator.cxx
index d1d431653f..61e5979b73 100644
--- a/Source/cmInstallTargetGenerator.cxx
+++ b/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,
@@ -211,8 +212,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;
diff --git a/Tests/RunCMake/Framework/InstallBeforeFramework-stderr.txt b/Tests/RunCMake/Framework/InstallBeforeFramework-stderr.txt
new file mode 100644
index 0000000000..a3a7c6cbb6
--- /dev/null
+++ b/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.
diff --git a/Tests/RunCMake/Framework/InstallBeforeFramework.cmake b/Tests/RunCMake/Framework/InstallBeforeFramework.cmake
new file mode 100644
index 0000000000..3791dace79
--- /dev/null
+++ b/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)
diff --git a/Tests/RunCMake/Framework/RunCMakeTest.cmake b/Tests/RunCMake/Framework/RunCMakeTest.cmake
index 4fc83f801d..e705a31fa1 100644
--- a/Tests/RunCMake/Framework/RunCMakeTest.cmake
+++ b/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)