Page MenuHomePhabricator

[CMake] Copy per-component `required_libraries` into `LINK_COMPONENTS`. NFC.
Needs ReviewPublic

Authored by bryant on Jan 18 2017, 5:05 AM.

Details

Summary

In this patch, dependencies that are expressed in required_libraries
of LLVMBUild.txt are copied into LINK_COMPONENTS of each component's
add_llvm_library/-_target CMake call.

Notes:

  • This patch is NFC, except for a minor detail: Passing LINK_COMPONENTS causes llvm_map_components_to_libnames (in cmake/modules/LLVM-Config.cmake) to verify that each LINK_COMPONENTS item actually exists. It's possible for this check to spuriously fail since components aren't added in RPO.

    So an additional small change to LLVM-Config.cmake is needed to quell this error.
  • lib/LineEditor/CMakeLists.txt expresses its dependence on LLVMSupport through LINK_LIBS. The patch corrects this to use LINK_COMPONENTS instead.

Event Timeline

bryant created this revision.Jan 18 2017, 5:05 AM
bryant added a comment.EditedJan 18 2017, 5:07 AM

Note that this is the NFC part of D28086.

mgorny added inline comments.Jan 18 2017, 11:49 AM
cmake/modules/LLVM-Config.cmake
204 ↗(On Diff #84820)

To be honest, I'm not convinced about this. Do I understand correctly that this code means to ensure case-insensitive mapping of libraries to their correct names? If it's not possible to do that reliably, I'd rather not do that at all and instead require people to write out correct names.

For a start, do you think it'd be possible to record all hits of this branch and check them later on? I'm thinking of checking any possible mismatches at the end of CMake, so if the canonicalization can't work, developers get to fix their case anyway.

lib/Analysis/CMakeLists.txt
95 ↗(On Diff #84820)

I would normally say you left a stray empty line here but I don't expect you to fix all those files ;-).

bryant added inline comments.Jan 18 2017, 9:29 PM
cmake/modules/LLVM-Config.cmake
204 ↗(On Diff #84820)

To be honest, I'm not convinced about this. Do I understand correctly that
this code means to ensure case-insensitive mapping of libraries to their
correct names?

Yes. Effectively, this leverages the LLVM_AVAILABLE_LIBS oracle provided by
llvmbuild to figure out the correctly camel-cased LLVMxyz form.
(capitalized_libs is the LLVM_AVAILABLE_LIBS, capitalized.)

If it's not possible to do that reliably, I'd rather not do that at all and
instead require people to write out correct names.

I totally agree. In fact, under this patch, expanded_components is updated
regardless of whether LLVM${c, uppercased} is found in capitalized_libs
(which is the uppercased LLVM_AVAILABLE_LIBS), so this entire check can be
pruned away. See below for why.

For a start, do you think it'd be possible to record all hits of this branch
and check them later on? I'm thinking of checking any possible mismatches at
the end of CMake, so if the canonicalization can't work, developers get to fix
their case anyway.

We're on the same page. The corollary to this patch is D28856. In that patch,
you'll notice that I've added a function, validate_component_deps, that does
more or less what you've suggested. After having scanned all CMakeLists.txt in
lib/ -- which means that all possible components have been hit -- that
function verifies that { each component's dependency { is a valid component,
i.e., is a library we've hit before (which presupposes properly cased name) } }.

Please let me know whether or not that made sense.

bryant added inline comments.Jan 18 2017, 9:48 PM
cmake/modules/LLVM-Config.cmake
204 ↗(On Diff #84820)

One possible alternative to neutering the capitalized_libs check would be to add the components in RPO. This is sufficient because there aren't supposed to be cyclic dependencies.

But since CMake doesn't necessarily traverse lib/ in RPO, it would have to scan everything first before adding components. So the build init process would be slower.

lib/Analysis/CMakeLists.txt
95 ↗(On Diff #84820)

It was not as painless as I had expected to generate and verify these changes. (: But now that I have the diff, I think I could match and edit out the stray whitespace directly.

bryant updated this revision to Diff 85240.Jan 21 2017, 2:15 AM

Notes:

  • Fix is_llvm_target's logic. Previously, it checked a component name (X86Utils, LanaiInstPrinter, so on) against fully qualified libnames (LLVMX86Utils, LLVMLanaiInstPrinter), which is incorrect. This is exactly the fix in D28869 by Chris.
  • Further update is_llvm_target to is_omitted_target_lib. The former checks whether a component is part of a known target, whereas the latter checks if the component is part of an omitted target (which is the check that we want).
  • Further update is_llvm_target to is_omitted_target_lib. The former checks whether a component is part of a known target...

Just to clarify, this is incorrect because it checks for membership of any known target, regardless of whether or not the target has been selected for building.

bryant marked 2 inline comments as done.Jan 21 2017, 2:20 AM
bryant updated this revision to Diff 85246.Jan 21 2017, 11:49 AM
  • Add the missing target sub-components when expanding a target.
bryant accepted this revision.Feb 8 2017, 12:37 PM

lgtm

This revision is now accepted and ready to land.Feb 8 2017, 12:37 PM
bryant requested review of this revision.Feb 8 2017, 12:38 PM
bryant edited edge metadata.

Wrong window.

bryant resigned from this revision.Feb 8 2017, 12:38 PM
bryant updated this revision to Diff 210957.Jul 20 2019, 5:36 AM
bryant edited the summary of this revision. (Show Details)

Rebased onto latest git monorepo.

Dropped previous changes to LLVM-Config.cmake since D28869 has been checked
in.

Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2019, 5:36 AM
Herald added subscribers: wuzish, Jim, dang and 29 others. · View Herald Transcript
bryant updated this revision to Diff 210959.Jul 20 2019, 5:59 AM
  • {Scalar,ObjCARC} => {Scalar,ObjCARC}Opts.
E5ten added a subscriber: E5ten.Jul 20 2019, 6:08 AM
sbc100 added inline comments.Jul 20 2019, 7:54 AM
llvm/utils/llvm-build/llvmlinkcomps.py
1 ↗(On Diff #210959)

Start script with #!/usr/bin/env python

Add from __future__ import print_function?

Add a file-level docstring to say what this does? When is this script supposed to run and by whom?

bryant marked an inline comment as done.Jul 23 2019, 4:34 PM
bryant added inline comments.
llvm/utils/llvm-build/llvmlinkcomps.py
1 ↗(On Diff #210959)

This script was included into this differential only to illustrate how LLVM_LINK_COMPONENTS were mechanically added into CMakeLists , just in case there are correctness issues with the approach (I could spot any).

Do we really want it included with the final git check-in? My initial guess was not.

bryant marked an inline comment as done.Jul 23 2019, 4:35 PM
bryant added inline comments.
llvm/utils/llvm-build/llvmlinkcomps.py
1 ↗(On Diff #210959)

Correction: I could *not* spot any.

beanz added inline comments.Jul 26 2019, 10:53 AM
llvm/lib/Analysis/CMakeLists.txt
9

It would be better to use the LINK_COMPONENTS parameter to add_llvm_library rather than setting this. CMake's variable scoping behavior is non-intuitive, which can lead to bugs.

bryant updated this revision to Diff 212484.Jul 30 2019, 5:53 PM
  • Switched to LINK_COMPONENTS.
  • There are a bunch of places in llvm/tools that already set LLVM_LINK_COMPONENTS. These were left untouched.
  • lib/ToolDrivers/llvm-lib brought over from LLVM_LINK_COMPONENTS for consistency with other lib components.