[PATCH] D28855: [CMake] Copy per-component `required_libraries` into `LINK_COMPONENTS`. NFC.

[bryant via Phabricator via llvm-commits]

bryant added inline comments.


================
Comment at: cmake/modules/LLVM-Config.cmake:204
-          message(FATAL_ERROR "Library `${c}' not found in list of llvm libraries.")
-        endif()
       else( lib_idx LESS 0 )
----------------
mgorny wrote:
> 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.
> 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.



Repository:
  rL LLVM

https://reviews.llvm.org/D28855