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

Switch to using the v2 resolver in most workspaces #128722

Merged
merged 6 commits into from
Aug 19, 2024

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Aug 6, 2024

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" everywhere except library.

@rustbot
Copy link
Collaborator

rustbot commented Aug 6, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Aug 6, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 6, 2024

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

The run-make-support library was changed

cc @jieyouxu

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead.

cc @calebzulawski, @programmerjake

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 6, 2024

Split off of #128359 since the library was split off to its own Cargo.lock.

src/tools/tidy/src/deps.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@klensy
Copy link
Contributor

klensy commented Aug 7, 2024

Why updating any deps here? There exist separate CI task (#127140, which stuck a little). As i understand, portable-simd developed in it's own repo (https://github.com/rust-lang/portable-simd), so shouldn't be changed here (at least this way it will be simpler to sync it)?

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 7, 2024

Why updating any deps here? There exist separate CI task (#127140, which stuck a little). As i understand, portable-simd developed in it's own repo (https://github.com/rust-lang/portable-simd), so shouldn't be changed here (at least this way it will be simpler to sync it)?

For licensing - the WASM tools and the crates that depend on them all need to be the latest version. Otherwise we get duplicate versions with different licenses that confuses tidy, as mentioned in #128722 (comment). I can rebase after #127140 so the diff is smaller.

You are right about portable-simd, I will drop those changes.

@bors
Copy link
Contributor

bors commented Aug 7, 2024

☔ The latest upstream changes (presumably #128783) made this pull request unmergeable. Please resolve the merge conflicts.

@tgross35 tgross35 mentioned this pull request Aug 15, 2024
@Mark-Simulacrum
Copy link
Member

r=me with this rebased

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"` everywhere except library and
submodules.
With the new resolver, a few dependencies get brought in twice with
different licenses. For example, all dependencies from `wasm-tools`
gained Apache-2.0 and MIT options, and with the v2 resolver we were
using one version from before and one version from after this change.
This made tidy's license check difficult.

Update some minimum versions to remove duplicate dependencies and smooth
out license checking.
v2 resolves some dependencies differently, and we adjusted some
dependency versions. Run `cargo update` to make sure everything is in
sync.
Recent versions of wasm-tools are now Apache-2.0 or MIT or Apache-2.0
with the LLVM exception, rather than strictly Apache-2.0 with the LLVM
exception. The only component with the exception has moved to a new
dependency `wasi-preview1-component-adapter-provider`.
@tgross35
Copy link
Contributor Author

Thanks Mark.

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Aug 18, 2024

📌 Commit 32fbbaa has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@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 Aug 18, 2024
@klensy
Copy link
Contributor

klensy commented Aug 18, 2024

rollup=never?

@tgross35
Copy link
Contributor Author

Hopefully this shouldn't conflict with anything, maybe just

@bors rollup=iffy

@bors
Copy link
Contributor

bors commented Aug 19, 2024

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 45fbf41 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 19, 2024
@bors bors merged commit 45fbf41 into rust-lang:master Aug 19, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 19, 2024
@tgross35 tgross35 deleted the new-resolver-root branch August 19, 2024 12:16
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (45fbf41): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

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
Regressions ❌
(secondary)
1.0% [1.0%, 1.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 2.2%, secondary -0.3%)

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)
2.2% [2.2%, 2.2%] 1
Regressions ❌
(secondary)
1.4% [1.4%, 1.4%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.0% [-2.0%, -2.0%] 2
All ❌✅ (primary) 2.2% [2.2%, 2.2%] 1

Cycles

Results (primary -4.5%, secondary -2.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.

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

Binary size

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

Bootstrap: 750.46s -> 749.421s (-0.14%)
Artifact size: 339.15 MiB -> 336.74 MiB (-0.71%)

@dpaoliello
Copy link
Contributor

@rustbot label +relnotes

This change includes a new version of cc-rs which no longer supports Visual Studio 2013, which means that Rust will no longer use or consider the VS 2013 linker when building *-msvc targets.

@rustbot rustbot added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 19, 2024
@Mark-Simulacrum
Copy link
Member

@tgross35 can we back that cc change out? It shouldn't go through a PR like this - probably merits t-compiler FCP?

(I'm not sure if we document which linker versions we consider, but I'm not comfortable landing it in a PR like this).

@dpaoliello
Copy link
Contributor

Per INSTALL.md, VS 2017 is required:

rust/INSTALL.md

Line 199 in 636d7ff

MSVC builds of Rust additionally require an installation of Visual Studio 2017

Rustup's docs have also been updated, but it looks like it hasn't been deployed yet: rust-lang/rustup@4831701

That said, I don't think there's anything blocking older versions except for cc-rs support.

@Mark-Simulacrum
Copy link
Member

INSTALL.md is ... not really the right place for that to be documented, it should probably go on the platform support page (rendered https://doc.rust-lang.org/nightly/rustc/platform-support.html) and doesn't appear there currently. I don't have any objection to raising the limit, but we should do that intentionally.

tgross35 added a commit to tgross35/rust that referenced this pull request Aug 20, 2024
`cc` 1.0.106 removes support for Visual Studio 12. Pin to 1.0.105 so we
don't drop support yet.

Link: rust-lang#128722 (comment)
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 20, 2024
`cc` 1.0.106 removes support for Visual Studio 12. Pin to 1.0.105 so we
don't drop support yet.

Fixes: rust-lang#128722 (comment)
@tgross35 tgross35 mentioned this pull request Aug 20, 2024
@tgross35
Copy link
Contributor Author

I opened #129290 to pin to the version below that release, per rust-lang/cc-rs#1142. Bit surprising that this change was semver-compatible, but maybe that wasn't explicitly intentional.

@dpaoliello since you have the background here, maybe you could put up a PR upgrading to 1.1.x and changing the docs after my pinning PR merges? As a place for compiler FCP to get us unpinned.

@dpaoliello
Copy link
Contributor

changing the docs

I assume you mean create a platform support page for the -msvc targets?

Bit surprising that this change was semver-compatible, but maybe that wasn't explicitly intentional.

If I recall, we went ahead with the change since 1) VS 2013 is out of support and 2) cc-rs's MSRV was bumped past the version where the Rust's INSTALL.md indicated that VS2017 was required.

Seems like there should be some guidance for bumping and documenting min tool versions somewhere. I know we've been waiting on changing the raw-dylib feature since we need a newer binutils version, but I can't find any docs that indicate what the min required version is.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 24, 2024
Pin `cc` to 1.0.105

`cc` 1.0.106 removes support for Visual Studio 12. Pin to 1.0.105 so we don't drop support yet.

Fixes: rust-lang#128722 (comment)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 24, 2024
Pin `cc` to 1.0.105

`cc` 1.0.106 removes support for Visual Studio 12. Pin to 1.0.105 so we don't drop support yet.

Fixes: rust-lang#128722 (comment)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 25, 2024
Rollup merge of rust-lang#129290 - tgross35:pin-cc, r=Mark-Simulacrum

Pin `cc` to 1.0.105

`cc` 1.0.106 removes support for Visual Studio 12. Pin to 1.0.105 so we don't drop support yet.

Fixes: rust-lang#128722 (comment)
@Mark-Simulacrum
Copy link
Member

Removing relnotes since #129290 dropped the break.

@Mark-Simulacrum Mark-Simulacrum removed the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 25, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Aug 26, 2024
Pin `cc` to 1.0.105

`cc` 1.0.106 removes support for Visual Studio 12. Pin to 1.0.105 so we don't drop support yet.

Fixes: rust-lang/rust#128722 (comment)
@Veykril
Copy link
Member

Veykril commented Aug 28, 2024

This seems to have broken most (if not all) ./x.py test compiler/<insert crate name here> invocations, failing with

error: none of the selected packages contains these features: jemalloc, llvm, max_level_info, rustc_use_parallel_compiler

for compiler/rustc_abi for example

@tgross35
Copy link
Contributor Author

Maybe bootstrap needs to get adjusted to be more accurate with the feature flags it passes? Might be worth a new issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc CI-spurious-fail-msvc CI spurious failure: target env msvc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.