Switch to using the v2 resolver in the workspace #128359
Closed
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
rustbot
added
A-testsuite
|
Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead. |
tgross35
force-pushed
the
use-new-resolver
branch
2 times, most recently
from
4bfb06f
to
43a47fb
Compare
tgross35
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
tgross35
force-pushed
the
use-new-resolver
branch
from
3b035a4
to
7c70f3d
Compare
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
This comment has been minimized.
This comment has been minimized.
|
I haven't figured out yet which targets are apparently incompatible (or if there's a specific problem with |
|
I found out what makes
If I hack around this by forcing plain std to be built with The solutions I can think of are:
edit: For the record, and in case anyone is interested, how I got here:
|
|
Thanks for figuring that out! Brought it up on Zulip https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Avoid.20std.20feature.20conflicts.20with.20V2.20resolver |
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #128529) made this pull request unmergeable. Please resolve the merge conflicts. |
bjorn3
reviewed
Aug 2, 2024
| @@ -67,6 +67,7 @@ impl Step for Std { | |||
| target, | |||
| self.override_build_kind.unwrap_or(builder.kind), | |||
| ); | |||
| cargo.arg("--features=std/panic-unwind"); | |||
There was a problem hiding this comment.
I think the root cause why this is necessary is that the sysroot crate enables the panic_unwind feature of std, rather than the panic-unwind feature. The panic_unwind feature exists because the panic-unwind feature enables the panic_unwind dependency using panic-unwind = ["panic_unwind"] rather than panic-unwind = ["dep:panic_unwind"].
There was a problem hiding this comment.
Try this diff:
diff --git a/library/std/Cargo.toml b/library/std/Cargo.toml
index fe601855cc1..a025a4ea04e 100644
--- a/library/std/Cargo.toml
+++ b/library/std/Cargo.toml
@@ -90,8 +90,8 @@ backtrace = [
'miniz_oxide/rustc-dep-of-std',
]
-panic-unwind = ["panic_unwind"]
-profiler = ["profiler_builtins"]
+panic-unwind = ["dep:panic_unwind"]
+profiler = ["dep:profiler_builtins"]
compiler-builtins-c = ["alloc/compiler-builtins-c"]
compiler-builtins-mem = ["alloc/compiler-builtins-mem"]
compiler-builtins-no-asm = ["alloc/compiler-builtins-no-asm"]
diff --git a/library/sysroot/Cargo.toml b/library/sysroot/Cargo.toml
index 7165c3e48af..b05f61fedcd 100644
--- a/library/sysroot/Cargo.toml
+++ b/library/sysroot/Cargo.toml
@@ -20,7 +20,7 @@ compiler-builtins-no-f16-f128 = ["std/compiler-builtins-no-f16-f128"]
compiler-builtins-mangled-names = ["std/compiler-builtins-mangled-names"]
llvm-libunwind = ["std/llvm-libunwind"]
system-llvm-libunwind = ["std/system-llvm-libunwind"]
-panic-unwind = ["std/panic_unwind"]
+panic-unwind = ["std/panic-unwind"]
panic_immediate_abort = ["std/panic_immediate_abort"]
optimize_for_size = ["std/optimize_for_size"]
profiler = ["std/profiler"](the changes to the profiler feature are not strictly necessary, but I figured removing another unintentional implicit feature can't hurt)
There was a problem hiding this comment.
Thanks, added at 7446e7f
There was a problem hiding this comment.
It may work without the cargo.arg("--features=std/panic-unwind"); now.
Pinning the resolver to v1 was done in 5abff37 ("Explicit set workspace.resolver ...") in order to suppress warnings. Since there is no specific reason not to use the new resolver and since it fixes issues, change to `resolver = "2"`. Fixes: rust-lang#128358
With the v2 resolver, this has the intended behavior of building compiler_builtins. Unfortunately, this exposes the fact that the current stage0 compiler cannot build the `f16` and `f128` implementation of compiler_builtins. Unconditionally disable `f16` and `f128` for now so we can update the resolver.
`std/panic-unwind` gets enabled when building the sysroot, but not when checking `std`. This was not a problem with the v1 resolver because it would unify features and just always enable `panic-unwind`. With the v2 resolver, however, this causes `std` to get built twice with different sets of features. This then causes an "multiple candidates for `rmeta` dependency `std` found" error. Always enable the `panic-unwind` feature to avoid this conflict.
tgross35
force-pushed
the
use-new-resolver
branch
from
79ca646
to
7446e7f
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The tidy error looks like zerocopy 0.6.6 is only BSD-2, 0.7.34 is BSD-2 or Apache-2 or MIT. This dependency seems to come from I think we will need a release after cryptocorrosion/cryptocorrosion#77 to resolve this, or temporarily allow BSD-2. |
|
Still the duplicate lang item failure on |
|
I believe the duplicate lang item error is caused by the presence of |
|
Does changing to the v2 resolver "just work" with the separate workspaces? I think that sounds like a nicer solution. |
|
The patch from #128359 (comment) is still necessary with #128534. And something like |
|
☔ The latest upstream changes (presumably #128534) made this pull request unmergeable. Please resolve the merge conflicts. |
This was referenced Aug 6, 2024
|
Splitting this into #128722 and #128724 to update separately after the lockfile split. This will no longer be needed as a fix for f128 symbols since I'm just removing reliance on cargo features rust-lang/compiler-builtins#652 / #128691. |
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.
Pinning the resolver to v1 was done in 5abff37 ("Explicit set workspace.resolver ...") in order to suppress warnings. Since there is no specific reason not to use the new resolver and since it fixes issues, change to
resolver = "2".Fixes #128358
r? @ehuss