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

Fri Oct 8 09:31:35 PDT 2021

ldionne 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
----------------
mstorsjo wrote:
> 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.
I will change to `this is usually the correct way`. I want to suggest that people look no further if they don't need more than that. If they need something more complicated, then sure, they can use the "default" build in some way to achieve their goals.

================
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
----------------
mstorsjo wrote:
> 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.
Yes. In other words, they are marked as deprecated immediately (so that means they are deprecated when we release LLVM 14). And as soon as we branch LLVM 14 out, we are free to remove support for those builds on `main`. It's somewhat aggressive, but at the same time we have a clear and easy migration path for everybody, and supporting all the ways to build right now is pretty taxing.

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

Fixed, thanks.

> 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.)

Nope, that's deprecated too after this patch. That requires using the various hacks to tell libc++abi where libc++ is and stuff like that (`LIBCXXABI_LIBCXX_PATH`). That's exactly the thing we're trying to move away from. If you need to pass different flags to the projects, we can add a better way to do that. Allowing the runtimes to be built separately goes against exactly the push I'm making, which is to ensure that we can assume we have a unified CMake invocation for all the runtimes.

================
Comment at: libcxx/utils/ci/run-buildbot:468
 ;;
+legacy-project-build)
+    clean
----------------
mstorsjo wrote:
> 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).
IMO it is tied precisely to the deprecation, since I moved all the other jobs to the new runtimes build. This effectively promotes the new runtimes build to a first class citizen that we run CI on by default, and at the same time adds a new job that tests the old way to build, just to make sure we don't break it yet. Did you mis-read the change and now you agree, or do you still disagree this belongs here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111356