[dolphin] make link with LLVM
ClosedPublic

Authored by rjvbb on Jul 29 2019, 9:22 AM.

Details

Summary

This patch fixes a link failure when building with the LLVM toolchain which does not discover the dependency on or pull in the private dolphin library when linking dolphin itself.

BUG: 410237

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rjvbb created this revision.Jul 29 2019, 9:22 AM
Restricted Application added a subscriber: kfm-devel. · View Herald TranscriptJul 29 2019, 9:22 AM
rjvbb requested review of this revision.Jul 29 2019, 9:22 AM

Can you try to build another kdeinit app (i.e. khelpcenter) to check if you get the same error?

src/CMakeLists.txt
304–306

Untested: have you tried to make dolphinprivate a PUBLIC dependency of kdeinit_dolphin?

rjvbb added a comment.Aug 14 2019, 7:38 AM
Can you try to build another kdeinit app (i.e. `khelpcenter`) to check if you get the same error?

No. But khelpcentre doesn't use a static library.

Untested: have you tried to make dolphinprivate a PUBLIC dependency of kdeinit_dolphin?

No, should I?

rjvbb added a comment.Aug 15 2019, 8:27 AM

Untested: have you tried to make dolphinprivate a PUBLIC dependency of kdeinit_dolphin?

Appears that this works too:

target_link_libraries(kdeinit_dolphin PUBLIC
    dolphinprivate
    PRIVATE
    dolphinstatic
    KF5::Crash
)

Untested: have you tried to make dolphinprivate a PUBLIC dependency of kdeinit_dolphin?

Appears that this works too:

target_link_libraries(kdeinit_dolphin PUBLIC
    dolphinprivate
    PRIVATE
    dolphinstatic
    KF5::Crash
)

Cool. I'd prefer this fix, so that we don't add another target_link_libraries call.

rjvbb updated this revision to Diff 64638.Aug 26 2019, 7:52 AM

Updated to use the "cool solution" ;)

rjvbb set the repository for this revision to R318 Dolphin.Aug 26 2019, 7:53 AM
elvisangelaccio accepted this revision.Sep 1 2019, 11:46 AM
This revision is now accepted and ready to land.Sep 1 2019, 11:46 AM
meven added a subscriber: meven.Tue, Nov 12, 1:54 PM

ping @rjvbb
Do you want someone else to commit this patch or do you want to do it yourself https://community.kde.org/Infrastructure/Phabricator ?

This revision was automatically updated to reflect the committed changes.
rjvbb added a comment.Tue, Nov 12, 4:50 PM

Apologies, I've been stretched way too thin since about the time this diff was accepted, I don't think I even noticed the fact.