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

Fri Oct 8 09:41:04 PDT 2021

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
----------------
ldionne wrote:
> 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.
Ok, that sounds good.

================
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
----------------
ldionne wrote:
> 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.
Ok, that's probably workable for me, hopefully it is for all other downstreams/vendors packaging it too.

I might request to postpone the removal ever so slightly, until 14.0.0 actually is released, since I try to test things across both latest tagged stable release, up until current git main branch - so after 14.x is branched, in the 1-2 months until the release is tagged, I build versions ranging from 13.0.0 up to early 15.x git main with the same setup. But I think the `<monorepo>/runtimes` setup might have worked well enough in the 13.0.0 release already (building worked, testing didn't, I think) that I could switch my build scripts already while keeping building on 13.0.0.

================
Comment at: libcxx/docs/ReleaseNotes.rst:91
+
+        $ cmake -S <monorepo>/runtimes -B build <LIBCXX-OPTIONS> <LIBCXXABI-OPTIONS>
----------------
ldionne wrote:
> 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.
Ok, right - yeah the interop between libcxxabi and libcxx is tricky and it makes sense to deprecate it.

================
Comment at: libcxx/utils/ci/run-buildbot:468
 ;;
+legacy-project-build)
+    clean
----------------
ldionne wrote:
> 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?
It took a little while to wrap my head around what this CI restructuring did exactly, so my first reaction was to split it out - but on second thought I don't mind keeping it coupled with the actual deprecation/docs change.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111356