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

bryant via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 18 21:48:09 PST 2017


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 )
----------------
bryant wrote:
> 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.
> 
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.



================
Comment at: lib/Analysis/CMakeLists.txt:95
+    Object
+
+)
----------------
mgorny wrote:
> I would normally say you left a stray empty line here but I don't expect you to fix all those files ;-).
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D28855





More information about the llvm-commits mailing list