Skip to content
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

Fix thin archive reading #107360

Merged
merged 3 commits into from
Jan 28, 2023
Merged

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jan 27, 2023

This includes a revert of #105221 to restore fat archive reading with LlvmArchiveBuilder.

Should fix #107162, #107334 and google/shaderc-rs#133

@bjorn3 bjorn3 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 27, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 27, 2023
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 27, 2023

@bors try

to provide a toolchain that can be used for testing if this indeed fixed it. This will build a linux toolchain, if necessary I can do a windows toolchain after that.

@bors
Copy link
Contributor

bors commented Jan 27, 2023

⌛ Trying commit de363d5 with merge 87b60fa8c61c1fc5243bb1bce530da8f3f694d4d...

@bors
Copy link
Contributor

bors commented Jan 27, 2023

☀️ Try build successful - checks-actions
Build commit: 87b60fa8c61c1fc5243bb1bce530da8f3f694d4d (87b60fa8c61c1fc5243bb1bce530da8f3f694d4d)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jan 27, 2023
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 27, 2023

This indeed fixes reading thin archives. At least on Linux:

$ rustup-toolchain-install-master 87b60fa8c61c1fc5243bb1bce530da8f3f694d4d
[...]
$ echo | rustc - --emit obj --crate-type lib # create dummy object file
$ ar rsT libthin.a rust_out.o # create thin archive
$ echo | rustc +stable -l static=thin - --crate-type staticlib -L .
error: failed to add native library ./libthin.a: Unsupported archive identifier

error: aborting due to previous error

$ echo | rustc +87b60fa8c61c1fc5243bb1bce530da8f3f694d4d -l static=thin - --crate-type staticlib -L .
$ echo $?
0

Going to do a windows try build too as most reports are coming from windows.

@bors try

@bors
Copy link
Contributor

bors commented Jan 27, 2023

⌛ Trying commit 3020ac1d605aabca87a39d218cdbc67c910f4f22 with merge e65225bb3f98a0001676c0cc139656e7209590c2...

@bjorn3 bjorn3 removed A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jan 27, 2023
@bors
Copy link
Contributor

bors commented Jan 27, 2023

☀️ Try build successful - checks-actions
Build commit: e65225bb3f98a0001676c0cc139656e7209590c2 (e65225bb3f98a0001676c0cc139656e7209590c2)

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 27, 2023

@jdm, @sagudev, @jpalus and @kanerogers can you try if this fixes the regression for you? You can use rustup-toolchain-install-master 87b60fa8c61c1fc5243bb1bce530da8f3f694d4d -c cargo to install the linux try build and rustup-toolchain-install-master e65225bb3f98a0001676c0cc139656e7209590c2 -c cargo to install the windows try build if you have rustup-toolchain-install-master installed from crates.io.

Edit: after that you also need to set it as default toolchain using rustup default <commit>.
Edit2: fixed rustup-toolchain-install-master command

@bjorn3 bjorn3 force-pushed the fix_thin_archive_reading branch from 3020ac1 to de363d5 Compare January 27, 2023 18:00
sagudev added a commit to sagudev/mozjs that referenced this pull request Jan 27, 2023
@sagudev
Copy link
Contributor

sagudev commented Jan 27, 2023

Can confirm it is working for mozjs on windows (apparently we use thin only for windows builds).
Here is nightly build for control test (it is expected to fail).
Here is build using e65225bb3f98a0001676c0cc139656e7209590c2 toolchain.

@jpalus
Copy link

jpalus commented Jan 27, 2023

@bjorn3 confirmed this PR (specifically de363d5) fixes #107334. Thanks.

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 27, 2023

Thanks for testing!

@bjorn3 bjorn3 added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Jan 27, 2023
@kanerogers
Copy link

Thanks so much for the quick turnaround @bjorn3!

I can confirm that e65225bb3f98a0001676c0cc139656e7209590c2 fixes google/shaderc-rs#133 on Windows. 👍🏻

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 28, 2023
@bors
Copy link
Contributor

bors commented Jan 28, 2023

⌛ Testing commit de363d5 with merge 2527416...

@bors
Copy link
Contributor

bors commented Jan 28, 2023

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 2527416 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 28, 2023
@bors bors merged commit 2527416 into rust-lang:master Jan 28, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 28, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2527416): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.3%, 0.6%] 13
Regressions ❌
(secondary)
0.5% [0.4%, 0.5%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [0.3%, 0.6%] 13

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.4% [1.4%, 1.4%] 1
Regressions ❌
(secondary)
2.7% [2.7%, 2.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.4% [1.4%, 1.4%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

@pnkfelix
Copy link
Member

pnkfelix commented Feb 2, 2023

Discussed in today's T-compiler triage meeting.

@rustbot label: beta-accepted stable-accepted

beta backport is PR #107609

stable backport is PR #107611

(I still would like to try to make regression tests, but that need not block the backports.)

@rustbot rustbot added beta-accepted Accepted for backporting to the compiler in the beta channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels Feb 2, 2023
@cuviper cuviper mentioned this pull request Feb 6, 2023
@cuviper cuviper removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Feb 6, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2023
…crum

Release 1.67.1

- Revert back to LlvmArchiveBuilder on all platforms rust-lang#107360
- 🚨 fix unsoundness in bootstrap cache code rust-lang#105624
- Mark uninlined_format_args as pedantic rust-lang/rust-clippy#10265
- Revert "switch to the macos-12-xl builder" rust-lang#107574

r? `@Mark-Simulacrum`
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 8, 2023
@cuviper cuviper modified the milestones: 1.69.0, 1.67.1 Feb 8, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2023
…n-archive-reading-for-1-68-beta, r=cuviper

Backport reverts to fix thin archive reading for 1 68 beta

This is a backport of PR rust-lang#107360 aimed at beta.

cc rust-lang#107162, rust-lang#107334 and google/shaderc-rs#133
wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Feb 12, 2023
Pkgsrc changes:
 * checksums only.

Upstream changes:

Version 1.67.1 (2023-02-09)
===========================

- [Fix interoperability with thin archives.]
  (rust-lang/rust#107360)
- [Fix an internal error in the compiler build process.]
  (rust-lang/rust#105624)
- [Downgrade `clippy::uninlined_format_args` to pedantic.]
  (rust-lang/rust-clippy#10265)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. stable-accepted Accepted for backporting to the compiler in the stable channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows builds fail to link C++ static library