Move download-ci-llvm out of bootstrap.py
#95170
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
15 tasks
|
@rustbot label: -S-waiting-on-review +S-waiting-on-author |
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
bjorn3
reviewed
Mar 21, 2022
|
FYI curl exists on newer versions of Windows (circa 2018). So you maybe could try it first then fallback to powershell. |
|
@ChrisDenton I'd prefer to keep this as close to the python version as possible to start. We can always change it to do feature detection later once this code isn't changing so much. |
jyn514
commented
Mar 22, 2022
There was a problem hiding this comment.
Ok, this should be ready for review :) if anyone on NixOS or Windows (either mingw/WSL or MSVC) could test this out, that would be extremely helpful ❤️ I've already tested on Ubuntu and it worked fine - NixOS failed because of invalid SSL certs, but I don't know if that's because my install is cursed or it's actually a real issue for other people.
You can test this by:
- checking out the branch:
git fetch origin pull/95170/head && git checkout FETCH_HEAD - clearing your download cache:
rm -r build/cache build/$(rustc --version --verbose | grep host | cut -d' ' -f2)/ci-llvm. This won't delete your build cache, just libLLVM.so. - downloading using the new code:
x.py checkwith[llvm] download-ci-llvm = truein config.toml.
UPDATE: this has been tested on Windows and Mac, it could still use testing on NixOS though.
jyn514
changed the title
download-ci-llvm out of bootstrap.pydownload-ci-llvm out of bootstrap.py
This comment has been minimized.
This comment has been minimized.
|
macOS Monterey 12.3 due to macOS Monterey 12.3 release notes include:
by default, there is no see also #71818 (comment) |
|
On WSL2 Ubuntu 20.04.4 LTS (running on Windows 11 host, although that doesn't matter), for some reason I wonder where the difference is between the above macOS monterey 12.3 and WSL2 Ubuntu 20.04 as they should exhibit the same error as |
|
@Walther this doesn't yet remove the python requirement - it shouldn't be too hard to do, since afaik python is only used for building llvm and a few rustdoc tests, but it's not going to be in the first draft. Can you set |
|
On Windows 11 natively, in a PowerShell session, with rust installed with |
Yeah - mostly curious as to where the difference is coming from, as it appears something is working around the name/path issue on one platform but not the other. Again on the macOS Monterey 12.3 on my M1 mac mini (2020): Adding a section to [build]
python = "python3"fails with a new error: |
Oh uh good point, I should have suggested |
jyn514
commented
Mar 22, 2022
This comment has been minimized.
This comment has been minimized.
I unsuccessfully tried to test this on NixOS together with #95234. |
|
@ben0x539 thanks! I pushed a fix for the rename I think should work :) I tried it out on my own nixOS install and got an error I don't quite understand - does it look familiar? |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 92f5052b91c11c1d125e5d9196b00d7c9d5d4f2a with merge f601fa6c7ef581b3298fe167d6aea661b2154d63... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This attempts to keep the logic as close to the original python as possible. `probably_large` has been removed, since it was always `True`, and UTF-8 paths are no longer supported when patching files for NixOS. I can readd UTF-8 support if desired. Note that this required making `llvm_link_shared` computed on-demand, since we don't know whether it will be static or dynamic until we download LLVM from CI.
|
I forgot dynamic linking isn't supported on all platforms - it broke during |
|
Hm, previously we only assumed dynamic linking when downloading from CI -- I guess linux has dynamic and windows has static linking for those artifacts (we support CI-built LLVM on windows, right? I seem to recall PRs to that effect). In any case, I think this should be OK. @bors r+ |
|
📌 Commit 7885ade has been approved by |
bors
added
S-waiting-on-bors
We support it on Windows, yes, that was the same PR adding support for static linking: #80932 |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (ff18038): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
eddyb
reviewed
May 14, 2022
| let llvm_lib = llvm_root.join("lib"); | ||
| for entry in t!(fs::read_dir(&llvm_lib)) { | ||
| let lib = t!(entry).path(); | ||
| if lib.ends_with(".so") { |
There was a problem hiding this comment.
You cannot use this to check for an extension, only whole file names - the docs say "Only considers whole path components to match." - I have no idea how this ever passed NixOS, it just can't ever work AFAICT.
There was a problem hiding this comment.
Opened fix PR:
eddyb
reviewed
May 14, 2022
| entries | ||
| }; | ||
| patchelf.args(&[OsString::from("--set-rpath"), rpath_entries]); | ||
| if !fname.ends_with(".so") { |
There was a problem hiding this comment.
Same thing here, this cannot check for an extension (it's equivalent to fname.file_name() == ".so" basically).
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 15, 2022
…k-Simulacrum Fix `download-ci-llvm` NixOS patching for `.so`s. See rust-lang#95170 (comment) - in short, `Path::ends_with` doesn't do the same thing as `str::ends_with`, and can only be used to check for whole file names, not extensions. With this PR, I get the full suite of: ``` extracting /home/eddy/Projects/rust-A/build/cache/llvm-ebb80ec4e90f8622440f3e33562db0d6e6c66555-true/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz to /home/eddy/Projects/rust-A/build/x86_64-unknown-linux-gnu/ci-llvm info: you seem to be using Nix. Attempting to patch /home/eddy/Projects/rust-A/build/x86_64-unknown-linux-gnu/ci-llvm/bin/llvm-config /nix/store/r4bzq2xilvv8fmqjg626hzwi22ah3hf4-rust-stage0-dependencies info: you seem to be using Nix. Attempting to patch /home/eddy/Projects/rust-A/build/x86_64-unknown-linux-gnu/ci-llvm/bin/FileCheck info: you seem to be using Nix. Attempting to patch /home/eddy/Projects/rust-A/build/x86_64-unknown-linux-gnu/ci-llvm/lib/libLLVM-14-rust-1.62.0-nightly.so ``` (that `libLLVM-14-rust-1.62.0-nightly.so` at the end having been missing before) r? `@Mark-Simulacrum` cc `@jyn514`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is ready for review. It has been tested on Windows, Linux, and NixOS.
The second commit ports the changes from #95234 to Rust; I can remove it if desired.
Helps with #94829.
As a follow-up, this makes it possible to avoid downloading llvm until it's needed for building
rustc_llvm; it would be nice to do that, but it shouldn't go in the first draft. It might also be possible to avoid requiring python until tests run (currently there's a check insanity.rs), but I haven't looked too much into that.@rustbot label +A-rustbuild