[libcxx-commits] [PATCH] D89013: [libcxx] Support per-target __config_site in per-target runtime build

Tue Apr 13 13:36:23 PDT 2021

phosek added a comment.

@ldionne friendly ping.

In D89013#2686799 <https://reviews.llvm.org/D89013#2686799>, @ldionne wrote:

> I agree the design is clean from the "what we ship" perspective. I really like the idea of shipping the whole library in a generic spot and only having one file embody all the target specific configuration of the library. I think it's a very elegant way to handle the configuration of libc++, and it will have positive effects beyond your intended use case like giving us a place and a way to push platform-specific hacks to.
>
> Where this really stinks IMO is in the way we build the library. I don't want us to have a separate `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR` setting and a different mode for building libc++. This is at the heart of many discussions we've been having lately. Basically, the way I do things:
>
> 1. Build libc++ for one target using the most straightforward possible CMake settings
> 2. Then, post-process that by moving headers around, creating universal binaries, etc, all of it from a separate script.
>
> That's how I set up the libc++ build at Apple because I didn't want to add too many platform specific things in the CMake. Otherwise we'd have e.g. a mode for building for many architectures (`x86_64` and `arm64`) at once on Apple platforms and creating a universal dylib from that. That's not the way CMake is intended to be used unless I fundamentally missed something. Also note that it would be much better if CMake had proper support for multi-arch, but I'm not aware of anything like that.
>
> On the contrary, your approach (please correct me if I'm mis-speaking) is to make the CMake work out of the box exactly as you want it, even if that means adding complexity. This has the nice benefit that everything is done in a single place, however it forces us to go against the way CMake was designed in a few places, for example when it comes to building the library for several platforms.
>
> My only push back on this patch, and on several patches in the same vein, is about this fundamental difference in approach that we have.

I agree with you, I'm just not sure if what you're describing is possible, at least not yet. There is one additional requirement we have, which is that build layout matches the installation layout. This is what many of our tools and tests rely on. For example, when you build `clang++` and then use it, it's going to look for C++ headers in `../include/c++/v1` and without needing to run `ninja install`. This is where a lot of the complexity comes from and changing this assumption would be non-trivial (and it's not even clear to me that the alternative would be better from usability perspective).

If you want to simplify the libc++ build, we could consider moving the path setup (that is the `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR` branch) to the runtimes build, but we would still need expose all of the variables to parametrize where different build artifacts are placed. If we decide to go that way, I'd prefer to do it in a follow up change since it's technically orthogonal.

I'd also point out that we support a lot of different ways to do things which is where a lot of the complexity comes from, one example is having multiple different ways to build runtimes which you brought up in your RFC, but there are others. `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR` is actually an example of that, I'd really prefer if that was the default and the only supported way, but we would need to ensure that all vendors have switched to it, just as I'd like for everyone to use the runtimes build.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89013