[PATCH] D89177: [cmake] Add support for multiple distributions

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.


================
Comment at: clang/cmake/modules/AddClang.cmake:121
+          set(export_to_clangtargets EXPORT Clang${distribution}Targets)
+          set_property(GLOBAL PROPERTY CLANG${distribution}_HAS_EXPORTS True)
         endif()
----------------
phosek wrote:
> Should we make this uppercase and join it with `_` to match the existing conventions? For example, you'd have `ClangToolchainTargets` and `CLANG_TOOLCHAIN_HAS_EXPORTS`. With the change as is, we would end up with `CLANGToolchain_HAS_EXPORT` which is a bit unusual.
I'm playing a little trick with the lack of underscore. In case you're using the default unnamed distribution (as in you just specified `LLVM_DISTRIBUTION_COMPONENTS`), `${distribution}` will be empty, so it'll expand to just `CLANG_HAS_EXPORTS`, which matches the existing property. If you wanted to add the underscore, you'd need to special-case the unnamed distribution, which seemed like a bunch of effort for a property name that should never be accessed directly. With that being said though, since all the Add*.cmake usages of `get_llvm_distribution` follow a similar pattern, I was considering just creating a higher-level function. Lemme make that change and share what it would look like and then we can decide which way we wanna go.


================
Comment at: lld/cmake/modules/LLDConfig.cmake.in:13
 # Provide all our library targets to users.
-include("@LLD_CONFIG_EXPORTS_FILE@")
+ at lld_config_include_exports@
----------------
phosek wrote:
> We use upper-case variables for the `*_CONFIG_CODE` so using lower case here is a bit unfortunate, could we use upper case for these variables?
I used the lowercase for consistency with the existing `@llvm_config_include_buildtree_only_exports@` in the LLVM files, but I'm happy to change it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89177/new/

https://reviews.llvm.org/D89177