-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Do not copy libstd dynamic library to sysroot #131188
Conversation
rustbot has assigned @albertlarsan68. Use |
@@ -1,4 +1,5 @@ | |||
//@ run-pass | |||
//@ no-prefer-dynamic We move the binary around, so do not depend dynamically on libstd |
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.
Why is this changed needed? Was it linking to the compiler's std copy before? If so, why?
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.
The test was buggy. It compiled a Rust binary that dynamically linked to libstd.so. Then it moved the binary around, which should have broken it. However, the binary somehow linked to the compiler's std copy instead (!), so it worked by accident.
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.
Since UI tests are built with -Cprefer-dynamic
did they all (modulo no-prefer-dynamic
) dynamically link to the compiler's std?
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.
I don't think so, no other tests broke. It was probably just caused by the fact that this binary was moved around, lost its link to the target library and then somehow got dynamically linked to the compiler one instead.
EDIT: or, in theory, all tests could have linked to the compiler's std before, and now they link to target instead, that's a second explanation. But that would mean that all cross-compiled tests would be failing before, right?
Well, for CI one (x86_64-pc-windows-msvc) - yes, but my locally build copy for some reason links dynamically (messed up config.toml, probably?) Ah, so, this is stage dependent. |
57ead4a
to
f59c8ff
Compare
You can r=me if you want. |
As usually with these kinds of changes, it's hard to predict all the repercussions that this can have. Let's see if full CI passes, and if yes, if someone starts to complain. @bors r=onur-ozkan rollup=never |
☀️ Test successful - checks-actions |
Finished benchmarking commit (e561499): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 1.9%, secondary 0.5%)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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 773.78s -> 773.813s (0.00%) |
Since #122362, rustc links statically to libstd.[so|dll]. Which means that the libstd.[so|dll] file no longer has to be in the rustc sysroot. However, we are currently still shipping this file, in every new release of Rust, for no reason, it's just wasted bytes.
This PR removes the dynamic library file from the built sysroot.
However, it is not yet performed on Windows, because stage0 incremental tests start failing there (see description of the issue here).
This is an extended version of #128986.
CC @Zoxc