Page MenuHomePhabricator

Use object library if cmake supports it
ClosedPublic

Authored by tatyana-krasnukha on Wed, Jun 19, 5:18 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

labath added a subscriber: labath.Wed, Jun 19, 5:28 AM

why not just change lldb-mi-utils into a conventional (STATIC) library, at which point it can just be linked in using regular target_link_libraries, which has worked since forever?

It will cause some time overhead for linking a library. Linking is already formidable for the project and I'd avoid it when possible.

hintonda added inline comments.Wed, Jun 19, 7:27 AM
tools/lldb/unittests/tools/lldb-mi/utils/CMakeLists.txt
16 ↗(On Diff #205556)

I'm assuming from your comment, that this is a performance issue. If so, could you add a comment and also update the commit message so people can appreciate the need for the added complexity?

Btw, just tested with 3.4.3, and your change made cmake happy. Thanks!

hintonda accepted this revision.Wed, Jun 19, 7:28 AM

Otherwise, LGTM, thanks!

This revision is now accepted and ready to land.Wed, Jun 19, 7:28 AM

I just reviewed all the release notes, and while 3.9 relaxed the usage of TARGET_OBJECTS, cmake doesn't explicitly mention allowing them in target_link_libraries until release 3.15.

I don't have any of those versions, but it looks like 3.15 is probably the version you need to use instead of 3.9. Please see: https://cmake.org/cmake/help/latest/release/3.15.html

tatyana-krasnukha retitled this revision from Add a worlaround for unsupported cmake feature to Use object library if cmake supports it.

As I figured out, cmake allows to use $<TARGET_OBJECTS:...> anywhere since version 3.9 (commit).

After the version 3.12 (commit) object libraries may be used as a right-hand side of target_link_libraries.

Version 3.15 allows to use $<TARGET_OBJECTS:...> expression for other types of libraries, but this change doesn't affect the patch.

Finally updated versions - target_sources supports using $<TARGET_OBJECTS:...> since CMake 3.5.0.

hintonda accepted this revision.Thu, Jun 20, 7:32 AM

Finally updated versions - target_sources supports using $<TARGET_OBJECTS:...> since CMake 3.5.0.

Great, thanks for narrowing that down,

Still LGTM!

Closed by commit rL363933: [unittests] Use object library if cmake supports it (authored by tkrasnukha, committed by ). · Explain WhyThu, Jun 20, 8:03 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jun 20, 8:03 AM

I really don't think this is the right solution.

$<TARGET_OBJECTS:lldb-mi-utils> can be passed in as source files in the unit test target, which should work just fine on CMake 3.4.x.

@beanz you're absolutely right, thank you.

This revision is now accepted and ready to land.Fri, Jun 21, 4:40 AM
Closed by commit rL364035: [unittests] Simplify CMakeLists with object library (authored by tkrasnukha, committed by ). · Explain WhyFri, Jun 21, 4:43 AM
This revision was automatically updated to reflect the committed changes.