[libcxx-commits] [PATCH] D111356: [runtimes] Use the new "runtimes" build by default and deprecate other builds

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/docs/BuildingLibcxx.rst:63
+single CMake invocation. This is the correct way to build the runtimes when putting together a
+toolchain, or when the system compiler is not adequate to build them (too old, unsupported, etc.).
+To do this, use the following CMake invocation, and in particular notice how we're now rooting the
----------------
FWIW, I'm not sure I entirely agree with the sentence implying that this is the only correct way to build the runtimes when putting together a toolchain: This approach doesn't work entirely when bootstrapping a toolchain entirely from scratch (when e.g. the cross C runtime isn't built yet).

In these cases, one still has to build the compiler first, then compiler-rt builtins, then externally build e.g. the cross C runtime, then proceed to build libc++ and the other runtimes. So when one has to do external steps inbetween, the standalone build (with e.g. `<monorepo>/runtimes`) decoupled from building the compiler, is the only practical way right now. See e.g. https://github.com/ClangBuiltLinux/llvm-distributors-conf-2021/issues/13#issuecomment-930911835 for more confirmation that this indeed isn't quite supported by the bootstrapping build process yet.


================
Comment at: libcxx/docs/ReleaseNotes.rst:64
+  maintain with a good level of support. Starting with this release, the runtimes support
+  exactly two ways of being built, which should catter to all use-cases. Furthermore,
+  these builds are as lightweight as possible and will work consistently even when targetting
----------------
Typo: cater


================
Comment at: libcxx/docs/ReleaseNotes.rst:70
+
+  All other ways to build are deprecated and will not be supported in the next release.
+  We understand that making these changes can be daunting. For that reason, here's a
----------------
So these will be marked as deprecated in the 14 release, and be unsupported by the time 15 is released? That sounds tolerable to me.


================
Comment at: libcxx/docs/ReleaseNotes.rst:91
+
+        $ cmake -S <monorepo>/runtimes -B build <LIBCXX-OPTIONS> <LIBCXXABI-OPTIONS>
----------------
This example is lacking the `LLVM_ENABLE_RUNTIMES=libcxx;libcxxabi` bit.

Also technically, it's still possible to build them separately in two invocations/build dirs by pointing them at runtimes and specifying one at a time. That's of course something that one would rather avoid unless really needed, but I could see the need if building in a config where one wants to set specific `CMAKE_CXX_FLAGS` differently for each of the runtime libraries. (I used to need to do this before, but I've managed to upstream fixes so this no longer is needed.)


================
Comment at: libcxx/utils/ci/run-buildbot:468
 ;;
+legacy-project-build)
+    clean
----------------
I'd kinda prefer to split out these CI changes to a separate commit, with separate explanations/arguments for why there, as this change isn't directly tied to the rest of the deprecation (it still tests essentially the same set of configurations, just slightly different labels on it).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111356