Don't leak sysroot crates through dependencies #149273
Conversation
Previously if a dependency of the current crate depended on a sysroot crate, then extern crate would in the current crate would pick the first loaded version of said sysroot crate even in case of an ambiguity. This is surprising and brittle. For -Ldependency= we already blocked this, but the fix didn't account for sysroot crates.
rustbot
added
S-waiting-on-review
|
r? @chenyukang rustbot has assigned @chenyukang. Use |
This comment has been minimized.
This comment has been minimized.
|
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
|
We end up with two copies of rustc_macros in the stage1 sysroot. I suspect cargo decided to built rustc_macros once as build dependency without optimizations and once as output artifact with optimizations because bootstrap passed |
rustbot
added
the
T-bootstrap
This comment has been minimized.
This comment has been minimized.
bjorn3
force-pushed
the
crate_locator_improvements
branch
from
8d439c2 to
f0a26f2
Compare
petrochenkov
added
S-waiting-on-author
|
Checking how important the @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Don't leak sysroot crates through dependencies
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (c77897e): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.3%, secondary -5.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.0%, secondary 3.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 470.06s -> 469.991s (-0.01%) |
rustbot
removed
the
S-waiting-on-perf
bjorn3
force-pushed
the
crate_locator_improvements
branch
from
ac0aabb to
e58a6df
Compare
|
That turns out to be much better than I expected. It is a small perf win rather than a perf hit as I was afraid of. I've now cleaned up @rustbot ready |
For direct dependencies it is a lot harder to not accidentally resolve an ambiguity through an indirect dependency edge and as it turns out this check was actually more expensive than the work it skipped. For indirect dependencies existing_match is still necessary due to an assert in load, but at the same time it also can't accidentally resolve an ambiguity given that we already know exactly which crate to look for based on the crate hash.
bjorn3
force-pushed
the
crate_locator_improvements
branch
from
15b716c to
7f1828c
Compare
|
I think I fixed the duplicate crate detection. Squashed all changes to detect duplicate crates into a single commit. @rustbot ready |
rustbot
added
S-waiting-on-review
|
@bors r+ |
bors
added
S-waiting-on-bors
…nkov Don't leak sysroot crates through dependencies Previously if a dependency of the current crate depended on a sysroot crate, then `extern crate` would in the current crate would pick the first loaded version of said sysroot crate even in case of an ambiguity. This is surprising and brittle. For `-Ldependency=` we already blocked this since #110229, but the fix didn't account for sysroot crates. Should fix #147966
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
The logs for github actions seem to be broken, so I can't see where this went wrong. @bors try jobs=dist-x86_64-msvc |
This comment has been minimized.
This comment has been minimized.
Don't leak sysroot crates through dependencies try-job: dist-x86_64-msvc
|
Guess it was a spurious failure on github's side. @bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 3f4dc1e (parent) -> 08de25c (this PR) Test differencesShow 7 test diffsStage 1
Stage 2
Additionally, 4 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 08de25c4ea16d7ecc3ceeb093d4f343a2be30df5 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (08de25c): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -4.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 0.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 478.161s -> 476.492s (-0.35%) |
7 participants
Previously if a dependency of the current crate depended on a sysroot crate, then
extern cratewould in the current crate would pick the first loaded version of said sysroot crate even in case of an ambiguity. This is surprising and brittle. For-Ldependency=we already blocked this since #110229, but the fix didn't account for sysroot crates.Should fix #147966