From 95210d027a03192532fcf2c71c98c79fd4cc0fd1 Mon Sep 17 00:00:00 2001 From: Brad King Date: Tue, 29 Jan 2019 11:54:46 -0500 Subject: 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 --- Source/cmInstallTargetGenerator.cxx | 31 ++++++++++++++++++++-- .../Framework/InstallBeforeFramework-stderr.txt | 7 +++++ .../Framework/InstallBeforeFramework.cmake | 5 ++++ Tests/RunCMake/Framework/RunCMakeTest.cmake | 2 ++ 4 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 Tests/RunCMake/Framework/InstallBeforeFramework-stderr.txt create mode 100644 Tests/RunCMake/Framework/InstallBeforeFramework.cmake diff --git a/Source/cmInstallTargetGenerator.cxx b/Source/cmInstallTargetGenerator.cxx index d1d4316..61e5979 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 0000000..a3a7c6c --- /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 0000000..3791dac --- /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 4fc83f8..e705a31 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) -- cgit v0.12