This is an archive of the discontinued LLVM Phabricator instance.

Install import libraries
ClosedPublic

Authored by jschueller on May 2 2019, 2:59 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jschueller created this revision.May 2 2019, 2:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2019, 2:59 AM
RKSimon edited reviewers, added: beanz, bogner, xiaobai; removed: RKSimon.May 2 2019, 3:10 AM

Adding some people better at cmake shenanigans

beanz accepted this revision.May 2 2019, 11:05 AM

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.

This revision is now accepted and ready to land.May 2 2019, 11:05 AM
xiaobai accepted this revision.May 2 2019, 11:59 AM

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

mstorsjo added a reviewer: rnk.May 7 2019, 3:33 AM
mstorsjo added a subscriber: rnk.

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?

@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 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.

This revision was automatically updated to reflect the committed changes.
vzakhari added inline comments.
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 added a subscriber: phosek.May 20 2019, 11:41 PM

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?

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.