-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[Runtimes] Rework and remove default runtimes build #158156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary: Currently the runtimes builds works by creating separate CMake projects that build the respetive runtime. Right now we have a separate handling for the 'default' target and manually specific runtimes via the `-DLLVM_RUNTIME_TARGETS` option. This patch changes the behavior to put all runtimes through the `LLVM_RUNTIME_TARGETS` pipeline. The old `"default"` argument is now a shorthand for `LLVM_DEFAULT_TARGET_TRIPLE` and corresponds to a sane default. In practical terms, this means the old `runtimes-bins` directory will now be `runtimes-x86_64-unknown-linux-gnu-bins` for the majority of users. We will not have `check-<name>-<triple>` targets, but I have made a top-level target that invokes all of the enabled targets check lines to keep this backward compatible. There's likely some edge cases I missed here, but it seems to work in the typical case for me. We'll see what CI thinks of this.
1f6ca21
to
2082d82
Compare
Oh, that's |
Yes, ideally this change doesn't affect anyone that isn't depending on the name of directories in the build tree. Fundamentally I'm hoping to make our runtimes infrastructure more predictable by removing this weird disconnect we have between the I added the |
set(install-${runtime_name}-${name}-stripped install-${runtime_name}-stripped) | ||
list(APPEND ${name}_extra_targets ${runtime_name}-${name} install-${runtime_name}-${name} install-${runtime_name}-${name}-stripped) | ||
if(LLVM_INCLUDE_TESTS) | ||
set(check-${runtime_name}-${name} check-${runtime_name} ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] unrelated change
Ping, I'm hoping this change is NFC in practice. Also I forgot to mention that this solves #98897. |
Unfortunately it's not, this change would break builds on Apple platforms because they don't use target triple for building runtimes, that's the reason why the |
Interesting, could you elaborate on this? As far as I understand, each target has a default target triple. The main difference here is the directory naming and the fact that it passes |
Summary:
Currently the runtimes builds works by creating separate CMake projects
that build the respetive runtime. Right now we have a separate handling
for the 'default' target and manually specific runtimes via the
-DLLVM_RUNTIME_TARGETS
option.This patch changes the behavior to put all runtimes through the
LLVM_RUNTIME_TARGETS
pipeline. The old"default"
argument is now ashorthand for
LLVM_DEFAULT_TARGET_TRIPLE
and corresponds to a sanedefault.
In practical terms, this means the old
runtimes-bins
directory willnow be
runtimes-x86_64-unknown-linux-gnu-bins
for the majority ofusers. We will not have
check-<name>
targets, but I have madea top-level target that invokes all of the enabled targets check lines
to keep this backward compatible. So
ninja check-cxx
will still work.There's likely some edge cases I missed here, but it seems to work in
the typical case for me. We'll see what CI thinks of this.
Fixes: #98897