Install import libraries

[jschueller]

Summary

Simplify the cmake logic to install both runtime and import libraries (treated as ARCHIVE), as the later are needed to link against llvm.

[RKSimon]

Adding some people better at cmake shenanigans

[beanz]

Yep. This looks like the right solution. Un-used install destinations are effectively ignored by CMake, so there is no reason to selectively choose only one.

[xiaobai]

Looks like the right thing to do to me. As beanz said, selectively choosing is unneeded work.

[mstorsjo]

For reference, this fixes PR41704.

@rnk Does this look ok to you from the MSVC perspective as well?

@jschueller Do you need someone to commit this for you?

[jschueller]

@mstorsjo Yes, if someone could commit this it would be great

For MSVC, this will also install the missing .lib files, but I wont test it.

[mstorsjo]

In D61425#1493179, @jschueller wrote:

@mstorsjo Yes, if someone could commit this it would be great

Ok, I can commit it tonight/tomorrow.

FWIW, are you also running into the issue that unittests/Passes/TestPlugin fails to link when building in this configuration? (I did see your separate patch about Shlwapi.)

For MSVC, this will also install the missing .lib files, but I wont test it.

Yes, and installing them there also makes sense, as DLLs without import libs are pretty pointless. But I'll leave some time for @rnk to comment in any case.

[vzakhari]

llvm/trunk/cmake/modules/AddLLVM.cmake

688 ↗

(On Diff #198593)

Hi Julien,

With this change you effectively define COMPONENT only for the RUNTIME target. The LIBRARY and ARCHIVE targets are now added to the Unspecified component.

CMake install documentation is not very robust about this:
install(TARGETS targets... [EXPORT <export-name>]

[
 [ARCHIVE|LIBRARY|RUNTIME|...]
 [DESTINATION <dir>]

...

[COMPONENT <component>]

...

] [...]

This basically means that COMPONENT option has to be set after each of LIBRARY, ARCHIVE, RUNTIME options.

CMake component based install and CPack cannot work properly after this commit. Would you please fix it?

Thanks,
Slava

[phosek]

I noticed that libLTO.dylib is no longer getting installed as part of toolchain even though it's still listed in LLVM_DISTRIBUTION_COMPONENTS and I bisected it to this change. I agree with @vzakhari, with this change LIBRARY and ARCHIVE are no longer being considered as components which breaks existing use cases of LLVM_DISTRIBUTION_COMPONENTS. Can we please revert this change?

[phosek]

In D61425#1509575, @phosek wrote:

I noticed that libLTO.dylib is no longer getting installed as part of toolchain even though it's still listed in LLVM_DISTRIBUTION_COMPONENTS and I bisected it to this change. I agree with @vzakhari, with this change LIBRARY and ARCHIVE are no longer being considered as components which breaks existing use cases of LLVM_DISTRIBUTION_COMPONENTS. Can we please revert this change?

D62176 is a fix.

llvm/cmake/modules/AddLLVM.cmake

     if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY OR ${name} STREQUAL "LTO" OR
         ${name} STREQUAL "Remarks" OR
         (LLVM_LINK_LLVM_DYLIB AND ${name} STREQUAL "LLVM"))
-      set(install_dir lib${LLVM_LIBDIR_SUFFIX})
-      if(ARG_MODULE OR ARG_SHARED OR BUILD_SHARED_LIBS)
-        if(WIN32 OR CYGWIN OR MINGW)
-          set(install_type RUNTIME)
-          set(install_dir bin)
-        else()
-          set(install_type LIBRARY)
-        endif()
-      else()
-        set(install_type ARCHIVE)
-      endif()
-
-      if (ARG_MODULE)
-        set(install_type LIBRARY)
-      endif()
 
       set(export_to_llvmexports)
       if(${name} IN_LIST LLVM_DISTRIBUTION_COMPONENTS OR

 
       install(TARGETS ${name}
               ${export_to_llvmexports}
-              ${install_type} DESTINATION ${install_dir}
+              LIBRARY DESTINATION lib${LLVM_LIBDIR_SUFFIX}
+              ARCHIVE DESTINATION lib${LLVM_LIBDIR_SUFFIX}
+              RUNTIME DESTINATION bin
               COMPONENT ${name})
 
       if (NOT LLVM_ENABLE_IDE)