Generate the llvm-lit script unless invoked from llvm/runtimes.
Details
- Reviewers
phosek ldionne - Group Reviewers
Restricted Project Restricted Project - Commits
- rG565d45541f86: [libcxx] Add a CI configuration for standalone building in llvm-project/runtimes
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Ok, so this works for the standalone build, but breaks the full llvm/runtimes build. I’ll look into it a bit later then.
Fix the list separator for when inoking ExternalProject_Add. Surprisingly, the previous form worked with CMake 3.20.1 but not with 3.21.1.
runtimes/CMakeLists.txt | ||
---|---|---|
77–82 | Can we land this change as a separate commit? | |
92–102 | Rather than removing this altogether, could we either check if the compiler supports these flags or if the compiler is Clang? | |
92–102 | -nostdinc++ should be support by GCC. | |
114–117 | Can we land this change as a separate commit? |
runtimes/CMakeLists.txt | ||
---|---|---|
77–82 | Sure | |
92–102 | I don't remove it altogether, I move setting it to llvm/runtimes/CMakeLists.txt (where it's only used by the currently-built clang, so it's fine to use it unconditionally). But keeping them here with detection also sounds fine to me. FWIW they're pretty much duplicate and unnecessary right now though, as at least libunwind, libcxxabi and libcxx all try to detect and add these flags themselves. | |
114–117 | Sure, can do. |
Rebased after landing other changes that were split out.
The only refactoring left is moving setting LLVM_RUNTIMES_BUILD to llvm-project/runtimes, including llvm-lit if that isn't set, and setting a default value for LLVM_INCLUDE_TESTS.
@phosek - What do you think of the rest of the changes left here in this patch now - do they seem sensible? Should I split them out to one to three more individual changes for the runtimes build (moving setting LLVM_RUNTIMES_BUILD to llvm/runtimes, including llvm-lit when necessary, making sure LLVM_INCLUDE_TESTS is on by default)?
That is fine with me.
I want to standardize around exactly two types of supported builds:
- Using <root>/runtimes as a CMake root.
- Using <root>/llvm as a CMake root and mentioning LLVM_ENABLE_RUNTIMES=..... That is effectively bootstrapping Clang and building the runtimes with the just-built compiler.
In particular, I want to remove the ability to use <root>/llvm as a CMake root and say LLVM_ENABLE_PROJECTS=libcxx,libcxxabi,libunwind as a way of building, since that has a bunch of problems.
Are we on the same page? If so, we'll also want to improve on the terrible terminology we have today. (2) is currently called a "runtimes build", and (1) has no name. I would suggest we call (1) a "build of the runtimes", or just a "normal build", and that we rename (2) to a "bootstrapping build" or something like that.
That sounds like a good general direction. Distributors of llvm and the runtimes are going to need a decent sized transition period though.
For people using LLVM_ENABLE_PROJECTS, I would believe it should be straightforward for them to migrate. The bigger disruption is going to be when/if we stop allowing building the runtime projects entirely on their own by pointing cmake directly at e.g. libcxx. Judging from last weeks LLVM Distributors Conference, I would say that's the way a majority of distributors build it today, and transitioning that into e.g. 1. above will take some effort in the more complex cases. And some of them build this way with standalone source dirs without the surrounding monorepo, e.g. cc @tstellar.
The increased use of LLVM_RUNTIMES_BUILD concerns me a bit. That variable was always meant to be a short term hack to work around the issue with the compiler-rt build, see https://github.com/llvm/llvm-project/blob/47f79c6057764e0c83016269ae2359f8c5c8d135/runtimes/CMakeLists.txt#L134. I should have probably used a more ominous name like DONT_USE_THIS_COMPILER_RT_BUILD_NEEDS_FIXING to make it more clear.
runtimes/CMakeLists.txt | ||
---|---|---|
191–193 | Could avoid the llvm-lit setup in llvm/runtimes and always do it here unconditionally? |
+1 to everything said above.
Another thing I'd point out is that we may want to rethink the use of LLVM_ENABLE_RUNTIMES and <root>/llvm/runtimes. In the bootstrapping build, we want to build Clang and then use to build runtimes, but LLVM isn't aware of Clang, Clang depends on LLVM. To give you a concrete example, if you used -DLLVM_ENABLE_PROJECTS=llvm -DLLVM_ENABLE_RUNTIMES=all, it's not clear what should happen since there's no Clang being built.
I think that in the long run, it'd be better if the bootstrapping build had moved either into Clang or even to the very top-level of the monorepo (that is <root>/CMakeLists.txt) since in practice you'll also want a linker, and possibly Flang and other tools.
runtimes/CMakeLists.txt | ||
---|---|---|
191–193 | I'm not actually entirely sure where/how llvm-lit is set up in that case - I think it might be built as part of the main llvm build, and then just implicitly assumed to exist there. If I leave this out, I get this: ninja: Entering directory `/home/martin/code/llvm-project/build/new-standalone' [0/1] cd /home/martin/code/llvm-project/build/new-standalone/libcxx/test && /usr/bin/python3.8 /home/martin/code/llvm-project/build/new-standalone/bin/llvm-lit /home/martin/code/llvm-project/build/new-standalone/libcxx/test /usr/bin/python3.8: can't open file '/home/martin/code/llvm-project/build/new-standalone/bin/llvm-lit': [Errno 2] No such file or directory If we include the llvm-lit subdir when we're called from llvm/runtimes, the nested external cmake build would override/clobber files that the toplevel cmake build produces too. Not sure if it's practically benign or if it would give nasty surprises down the line at some point. Or should llvm/runtimes pass a more specific HAVE_LLVM_LIT=TRUE, or should we check whether the intended llvm-lit tool actually exists in the expected spot, and if not, include this? (Are there potential race conditions between the toplevel build and the cmake configuration for the runtimes then?) |
I'm glad we're all on the same page.
Agreed, we can give them like 2 LLVM releases (roughly 1 year).
The bigger disruption is going to be when/if we stop allowing building the runtime projects entirely on their own by pointing cmake directly at e.g. libcxx. Judging from last weeks LLVM Distributors Conference, I would say that's the way a majority of distributors build it today, and transitioning that into e.g. 1. above will take some effort in the more complex cases. And some of them build this way with standalone source dirs without the surrounding monorepo, e.g. cc @tstellar.
Indeed, and that sort of build (standalone builds) are something I've been trying to eradicate for a while. That type of build makes everything a lot more complicated for us. I think moving to a "normal build" using <root>/runtimes as the CMake root should be pretty easy -- they'll just end up with a single CMake invocation instead of two or more.
Yeah, I agree. How about this:
- cmake -S <root>/llvm -DLLVM_ENABLE_PROJECTS=libcxx doesn't work anymore.
- cmake -S <root> -DLLVM_ENABLE_PROJECTS=clang -DLLVM_BOOTSTRAP_RUNTIMES=libcxx performs a bootstrapping build (we can iterate on the naming, today this is -DLLVM_ENABLE_RUNTIMES=libcxx). If you don't add LLVM_ENABLE_PROJECTS=clang, the bootstrapping build can't work.
- cmake -S <root>/runtimes -DLLVM_ENABLE_RUNTIMES=libcxx performs a normal build of libc++ with whatever system compiler you have.
- All the other ways to build libc++ & al don't work anymore.
I think we can get there in the next year.
Agreed, that's what I had in mind. It'd enable lot of simplification and cleanup throughout the build.
This looks reasonable to me. Unless @phosek objects, I'd like to get this landed so I can start transitioning things to this build ASAP.
runtimes/CMakeLists.txt | ||
---|---|---|
191–193 | Having a more specific variable would be preferable. What I'm not sure about is where the lit depends comes from even in the bootstrapping build. We explicitly list other test dependencies here: https://github.com/llvm/llvm-project/blob/623f93ed1c99904a2b092c617bb392010e7a0184/llvm/runtimes/CMakeLists.txt#L461 but lit is not in that list. It's possible that this dependency comes from some other edge though. I want to make sure that this dependency is also expressed correctly, rather than it working thus far only by accident (which is a possibility). |
runtimes/CMakeLists.txt | ||
---|---|---|
191–193 | There's no build-time dependency for lit, the bin/llvm-lit script gets generated at configure time by configure_file(llvm-lit.in $out) - so whenever ninja is running, it should already exist. With that in mind, I guess it would be tolerable to try to peek here whether the supposed output llvm-lit executable script exists or not as well? The updated patch passes an explicit HAVE_LLVM_LIT though. |
Can we land this change as a separate commit?