[libcxx-commits] [PATCH] D128568: [runtimes] adds llvm-libgcc to the list of runtimes to be sorted

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: runtimes/CMakeLists.txt:18-25
 set(LLVM_ALL_RUNTIMES "libc;libunwind;libcxxabi;libcxx;compiler-rt;openmp")
 set(LLVM_ENABLE_RUNTIMES "" CACHE STRING
   "Semicolon-separated list of runtimes to build (${LLVM_ALL_RUNTIMES}), or \"all\".")
 if(LLVM_ENABLE_RUNTIMES STREQUAL "all" )
   set(LLVM_ENABLE_RUNTIMES ${LLVM_ALL_RUNTIMES})
 endif()
 include(SortSubset)
----------------
ldionne wrote:
> I'd like to suggest this instead:
> 
> ```
> set(LLVM_DEFAULT_RUNTIMES "libc;libunwind;libcxxabi;libcxx;compiler-rt;openmp")
> set(LLVM_ALL_RUNTIMES "${LLVM_DEFAULT_RUNTIMES};llvm-libgcc")
> set(LLVM_ENABLE_RUNTIMES "" CACHE STRING
>   "Semicolon-separated list of runtimes to build (which must be in ${LLVM_ALL_RUNTIMES}), or \"all\", in which case the following set of default runtimes will be used: ${LLVM_DEFAULT_RUNTIMES}.")
> if(LLVM_ENABLE_RUNTIMES STREQUAL "all" )
>   set(LLVM_ENABLE_RUNTIMES ${LLVM_DEFAULT_RUNTIMES})
> endif()
> include(SortSubset)
> sort_subset("${LLVM_ALL_RUNTIMES}" "${LLVM_ENABLE_RUNTIMES}" LLVM_ENABLE_RUNTIMES)
> ```
> 
> This opens the door to adding other "alternative" runtimes that are not selected by default when one uses `all`. Note that `all` becomes a slight misnomer, but it seems like it already is anyway cause there exists some runtimes (such as `llvm-libgcc`) that are not selected by `all`.
Thanks for the improvement, this should future-proof it.

I'm okay with duping users into thinking that `all` still means everything, because it does from a user's perspective. llvm-libgcc is incompatible with compiler-rt and libunwind because it builds them both with strict opinions (and then does extra stuff). As a user, even I'm not supposed to install llvm-libgcc and expect it to work. That's something only toolchain release managers who own pretty much everything (that would be cjdb@ only when concerned with the ChromeOS toolchain). That's a really small group of people to cater for, and llvm-libgcc actually makes them go quite out of their way to say "yes, I know what I'm doing!" before it'll build for them.

I think the same should be true for anything else that shouldn't be covered by default, so I've left the `LLVM_ENABLE_RUNTIMES` description unchanged (but this should otherwise be the same as your suggestion).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128568