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: Set MSRV for internal packages #12381

Merged
merged 2 commits into from
Aug 24, 2023
Merged

fix: Set MSRV for internal packages #12381

merged 2 commits into from
Aug 24, 2023

Conversation

epage
Copy link
Contributor

@epage epage commented Jul 19, 2023

What does this PR try to resolve?

Correctly communicates the MSRV we support to our users.

For packages that are more generally mean to be used by other people, I've punted on for now (see ehuss' comment on this PR). We'll likely need to figure this out for #12432 though

Additional information

This is prep for a future change which will have us use a fixed rust version for the semver tests with a PR updating them.

@rustbot
Copy link
Collaborator

rustbot commented Jul 19, 2023

r? @ehuss

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

@rustbot rustbot added A-cfg-expr Area: Platform cfg expressions A-cli-help Area: built-in command-line help A-dependency-resolution Area: dependency resolution and the resolver A-environment-variables Area: environment variables A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-interacts-with-crates.io Area: interaction with registries A-registry-authentication Area: registry authentication and authorization (authn authz) A-semver Area: semver specifications, version matching, etc. A-testing-cargo-itself Area: cargo's tests S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 19, 2023
@epage epage force-pushed the msrv branch 2 times, most recently from b7e2473 to 4284fa2 Compare July 19, 2023 15:06
@bors
Copy link
Contributor

bors commented Jul 19, 2023

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

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is technically true that Cargo-the-library is only guaranteed to be compatible with one corresponding Rust version. And this also gets rid off some burdens we would have on MSRV. However, it is a bit too strict for every crate to stick to the latest stable. Someone may only depends on a tiny part of cargo-the-library but with this MSRV bump that couldn't anymore.

@weihanglo
Copy link
Member

This is prep for a future change which will have us use a fixed rust version for the semver tests with a PR updating them.

What will this look like? I didn't get the full picture maybe because of missing this?

@epage
Copy link
Contributor Author

epage commented Jul 20, 2023

Someone may only depends on a tiny part of cargo-the-library but with this MSRV bump that couldn't anymore.

Unless its behind a feature flag or relying on a behavior change, MSRV is all-or-nothing.

@epage
Copy link
Contributor Author

epage commented Jul 20, 2023

This is prep for a future change which will have us use a fixed rust version for the semver tests with a PR updating them.

What will this look like? I didn't get the full picture maybe because of missing this?

Either one could be done first. Basically, we'd have a similar regex-based PR update so cargo +stable run -p semver-check could be cargo +1.71.0 run -p semver-check

@bors
Copy link
Contributor

bors commented Jul 20, 2023

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

@weihanglo
Copy link
Member

Some crates like crates-io and home are pretty not directly coupled with cargo, feel like they shouldn't be set with the same MSRV.

(and perhaps cargo-util if we have a firmed definition of what utils are…)

@epage
Copy link
Contributor Author

epage commented Jul 21, 2023

Some crates like crates-io and home are pretty not directly coupled with cargo, feel like they shouldn't be set with the same MSRV.

The choices I'm aware of

  • Don't set MSRV and put it in users hands
    • Short term: users have to play around to find the version that works
    • Long term: with MSRV aware resolver, users will still have to play around to find the version that works
  • Set to latest
    • Might be too high in some cases
    • Easy to verify
  • Set different MSRVs
    • To ensure accuracy, we'd need to add MSRV check jobs
    • We'd need to hand-manage the list of which packages to verify with which MSRV or write a tool
  • Manage multiple workspaces, one of the core of cargo and one for more external stuff
    -This might be of more interest if we start taking responsibility for Rust APIs for cargo (e.g. build scripts, metadata)

Considering

  • home is primarily for cargo (nasty cargo fetch #12386) (and cargo-util looks similar)
  • So far, we've not been looking to take on the maintenance load of clients for our crates (making them more "buyer beware")
  • We want our process to scale with adding more packages, including CI

To me, it seems like its a choice to no MSRV or latest (until / unless we decide to have multiple workspaces) and setting it would be useful so I would lean towards that.

Thoughts?

@weihanglo weihanglo added the T-cargo Team: Cargo label Jul 21, 2023
@weihanglo
Copy link
Member

From the point of view of maintenance, I agree on all the points above. However, I believe this will frustrate some people, especially home crate users, though I doubt we will have any significant release they want in any future 😆. Besides, developers of crates depending on cargo or cargo-util should have known they live on the bleeding edge.

That is to say, I think the approach is pretty acceptable. And yes, if cargo-the-library ends up being more modularized, we can explore a more fine-grained MSRV control.

I'd like to do a quick poll before proceeding.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 21, 2023

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Jul 21, 2023
@weihanglo
Copy link
Member

BTW, version bump check is somehow broken. See #12347. You may need to manually check if it does need a bump for nightly.

(I'll try fixing that in the next few days)

],
depNameTemplate: 'latest-msrv',
packageNameTemplate: 'rust-lang/rust',
datasourceTemplate: 'github-releases',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just mind that 1.71.1 is not yet in GitHub Release.

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 24, 2023

📌 Commit 87b6e0e has been approved by weihanglo

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 24, 2023
@bors
Copy link
Contributor

bors commented Aug 24, 2023

⌛ Testing commit 87b6e0e with merge 8b39dc0...

bors added a commit that referenced this pull request Aug 24, 2023
fix: Set MSRV for internal packages

### What does this PR try to resolve?

Correctly communicates the MSRV we support to our users.

For packages that are more generally mean to be used by other people, I've punted on for now (see ehuss' comment on this PR).  We'll likely need to figure this out for #12432 though

### Additional information

This is prep for a future change which will have us use a fixed rust version for the semver tests with a PR updating them.
@bors
Copy link
Contributor

bors commented Aug 24, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 8b39dc0 to master...

@bors
Copy link
Contributor

bors commented Aug 24, 2023

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@weihanglo
Copy link
Member

@bors retry

@bors
Copy link
Contributor

bors commented Aug 24, 2023

⌛ Testing commit 87b6e0e with merge 2a159aa...

@bors
Copy link
Contributor

bors commented Aug 24, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 2a159aa to master...

@bors bors merged commit 2a159aa into rust-lang:master Aug 24, 2023
20 checks passed
@epage epage deleted the msrv branch August 24, 2023 12:30
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 26, 2023
Update cargo

13 commits in 2cc50bc0b63ad20da193e002ba11d391af0104b7..925280f028db3a322935e040719a0754703947cf
2023-08-22 22:43:08 +0000 to 2023-08-25 21:16:44 +0000
- string leek is stable (rust-lang/cargo#12559)
- refactor: Pull out cargo-add MSRV code for reuse (rust-lang/cargo#12553)
- Contrib: Add process for security responses. (rust-lang/cargo#12487)
- Support dependencies from registries for artifact dependencies, take 2 (rust-lang/cargo#12421)
- fix(toml): Improve parse errors (rust-lang/cargo#12556)
- Create dedicated unstable flag for asymmetric-token (rust-lang/cargo#12551)
- chore(deps): update latest msrv to v1.72.0 (rust-lang/cargo#12549)
- changelog: add link to CVE-2023-40030 (rust-lang/cargo#12550)
- refactor(install): Move value parsing to clap (rust-lang/cargo#12547)
- fix: Set MSRV for internal packages (rust-lang/cargo#12381)
- doc: fix two links to tracing docs (rust-lang/cargo#12537)
- use AND search when having multiple terms (rust-lang/cargo#12548)
- fix(log): Use a more compact relative-time format (rust-lang/cargo#12542)

r? ghost
@weihanglo
Copy link
Member

Just realized, with this pull request and #12395, it is inevitable that every time a Rust release comes out, cargo-util and crates-io will be forced to release a version due to the bump of package.rust-version, even when there is no actual code change involved.

Is that something we've considered? It feels like a bit unnecessary.

@ehuss ehuss added this to the 1.74.0 milestone Sep 30, 2023
bors added a commit that referenced this pull request Oct 8, 2023
Set and verify all MSRVs in CI

### What does this PR try to resolve?

Allow us to advertise an MSRV for all public packages, unblocking #12432.

Packages are broken down into two MSRV policies
- Internal packages: `rust-version=stable`
- Public packages: `rust-version=stable-2`

To support this
- RenovateBot will automatically update all MSRVs
- CI will verify all packages build according to their MSRV

Since this takes the "single workspace" approach, the downside is that common dependencies are subject to each package's MSRV.  We also can't rely on `-Zmsrv-policy` to help us generate a lockfile because it breaks down when trying to support multiple MSRVs in a workspace

### How should we test and review this PR?

Per commit

### Additional information

#12381 skipped setting some MSRVs because we weren't sure how to handle it.  For `cargo-credential`, we needed to do something and did one-off verification (#12623).  `cargo-hack` recently gained the ability to automatically select MSRVs for each package allowing us to scale this up to the entire workspace.

I don't know if we consciously chose an MSRV policy for `cargo-credential` but it looked like N-2 so that is what I stuck with and propagated out.
- Without an aggressive sliding MSRV, we discourage people from using newer features because they will feel like they need permission which means it needs to be justified
- Without an aggressive sliding MSRV, if the MSRV at one point in time works for someone, they tend to assume it will always work, leading to frustration at unmet expectations.

I switched the MSRV check to `cargo check` from `cargo test` because I didn't want to deal with the out-of-diskspace issues and `check` will catch 99% of MSRV issues.

Potential future improvements to `cargo-hack`
- Allow `--version-range ..stable` so we can verify all MSRVs that aren't stable which would bypass the diskspace issues and allow us to more easily use `cargo test` again
- Verify on a `cargo package` version of a crate (taiki-e/cargo-hack#216)
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jan 8, 2024
Pkgsrc changes:

 * Remove NetBSD-8 support (embedded LLVm requires newer C++
   than what is in -8; it's conceivable that this could still
   build with an external LLVM)
 * undo powerpc 9.0 file naming tweak, since we no longer support -8.
 * Remove patch to LLVM for powerpc now included by upstream.
 * Minor adjustments, checksum changes etc.


Upstream changes:

Version 1.74.1 (2023-12-07)
===========================

- [Resolved spurious STATUS_ACCESS_VIOLATIONs in LLVM]
  (rust-lang/rust#118464)
- [Clarify guarantees for std::mem::discriminant]
  (rust-lang/rust#118006)
- [Fix some subtyping-related regressions]
  (rust-lang/rust#116415)

Version 1.74.0 (2023-11-16)
==========================

Language
--------

- [Codify that `std::mem::Discriminant<T>` does not depend on any
  lifetimes in T]
  (rust-lang/rust#104299)
- [Replace `private_in_public` lint with `private_interfaces` and
  `private_bounds` per RFC 2145]
  (rust-lang/rust#113126)
  Read more in
  [RFC 2145](https://rust-lang.github.io/rfcs/2145-type-privacy.html).
- [Allow explicit `#[repr(Rust)]`]
  (rust-lang/rust#114201)
- [closure field capturing: don't depend on alignment of packed fields]
  (rust-lang/rust#115315)
- [Enable MIR-based drop-tracking for `async` blocks]
  (rust-lang/rust#107421)

Compiler
--------

- [stabilize combining +bundle and +whole-archive link modifiers]
  (rust-lang/rust#113301)
- [Stabilize `PATH` option for `--print KIND=PATH`]
  (rust-lang/rust#114183)
- [Enable ASAN/LSAN/TSAN for `*-apple-ios-macabi`]
  (rust-lang/rust#115644)
- [Promote loongarch64-unknown-none* to Tier 2]
  (rust-lang/rust#115368)
- [Add `i686-pc-windows-gnullvm` as a tier 3 target]
  (rust-lang/rust#115687)

Libraries
---------

- [Implement `From<OwnedFd/Handle>` for ChildStdin/out/err]
  (rust-lang/rust#98704)
- [Implement `From<{&,&mut} [T; N]>` for `Vec<T>` where `T: Clone`]
  (rust-lang/rust#111278)
- [impl Step for IP addresses]
  (rust-lang/rust#113748)
- [Implement `From<[T; N]>` for `Rc<[T]>` and `Arc<[T]>`]
  (rust-lang/rust#114041)
- [`impl TryFrom<char> for u16`]
  (rust-lang/rust#114065)
- [Stabilize `io_error_other` feature]
  (rust-lang/rust#115453)
- [Stabilize the `Saturating` type]
  (rust-lang/rust#115477)
- [Stabilize const_transmute_copy]
  (rust-lang/rust#115520)

Stabilized APIs
---------------

- [`core::num::Saturating`]
  (https://doc.rust-lang.org/stable/std/num/struct.Saturating.html)
- [`impl From<io::Stdout> for std::process::Stdio`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStdout%3E-for-Stdio)
- [`impl From<io::Stderr> for std::process::Stdio`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`impl From<OwnedHandle> for std::process::Child{Stdin, Stdout, Stderr}`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`impl From<OwnedFd> for std::process::Child{Stdin, Stdout, Stderr}`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`std::ffi::OsString::from_encoded_bytes_unchecked`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.from_encoded_bytes_unchecked)
- [`std::ffi::OsString::into_encoded_bytes`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.into_encoded_bytes)
- [`std::ffi::OsStr::from_encoded_bytes_unchecked`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.from_encoded_bytes_unchecked)
- [`std::ffi::OsStr::as_encoded_bytes`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.as_encoded_bytes)
- [`std::io::Error::other`]
  (https://doc.rust-lang.org/stable/std/io/struct.Error.html#method.other)
- [`impl TryFrom<char> for u16`]
  (https://doc.rust-lang.org/stable/std/primitive.u16.html#impl-TryFrom%3Cchar%3E-for-u16)
- [`impl<T: Clone, const N: usize> From<&[T; N]> for Vec<T>`]
  (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E)
- [`impl<T: Clone, const N: usize> From<&mut [T; N]> for Vec<T>`]
  (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26mut+%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E)
- [`impl<T, const N: usize> From<[T; N]> for Arc<[T]>`]
  (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#impl-From%3C%5BT;+N%5D%3E-for-Arc%3C%5BT%5D,+Global%3E)
- [`impl<T, const N: usize> From<[T; N]> for Rc<[T]>`]
  (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#impl-From%3C%5BT;+N%5D%3E-for-Rc%3C%5BT%5D,+Global%3E)

These APIs are now stable in const contexts:

- [`core::mem::transmute_copy`]
  (https://doc.rust-lang.org/beta/std/mem/fn.transmute_copy.html)
- [`str::is_ascii`]
  (https://doc.rust-lang.org/beta/std/primitive.str.html#method.is_ascii)
- [`[u8]::is_ascii`]
  (https://doc.rust-lang.org/beta/std/primitive.slice.html#method.is_ascii)

Cargo
-----

- [fix: Set MSRV for internal packages]
  (rust-lang/cargo#12381)
- [config: merge lists in precedence order]
  (rust-lang/cargo#12515)
- [fix(update): Clarify meaning of --aggressive as --recursive]
  (rust-lang/cargo#12544)
- [fix(update): Make `-p` more convenient by being positional]
  (rust-lang/cargo#12545)
- [feat(help): Add styling to help output ]
  (rust-lang/cargo#12578)
- [feat(pkgid): Allow incomplete versions when unambigious]
  (rust-lang/cargo#12614)
- [feat: stabilize credential-process and registry-auth]
  (rust-lang/cargo#12649)
- [feat(cli): Add '-n' to dry-run]
  (rust-lang/cargo#12660)
- [Add support for `target.'cfg(..)'.linker`]
  (rust-lang/cargo#12535)
- [Stabilize `--keep-going`]
  (rust-lang/cargo#12568)
- [feat: Stabilize lints]
  (rust-lang/cargo#12648)

Rustdoc
-------

- [Add warning block support in rustdoc]
  (rust-lang/rust#106561)
- [Accept additional user-defined syntax classes in fenced code blocks]
  (rust-lang/rust#110800)
- [rustdoc-search: add support for type parameters]
  (rust-lang/rust#112725)
- [rustdoc: show inner enum and struct in type definition for concrete type]
  (rust-lang/rust#114855)

Compatibility Notes
-------------------

- [Raise minimum supported Apple OS versions]
  (rust-lang/rust#104385)
- [make Cell::swap panic if the Cells partially overlap]
  (rust-lang/rust#114795)
- [Reject invalid crate names in `--extern`]
  (rust-lang/rust#116001)
- [Don't resolve generic impls that may be shadowed by dyn built-in impls]
  (rust-lang/rust#114941)

Internal Changes
----------------

These changes do not affect any public interfaces of Rust, but they represent
significant improvements to the performance or internals of rustc and related
tools.

None this cycle.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cfg-expr Area: Platform cfg expressions A-cli-help Area: built-in command-line help A-dependency-resolution Area: dependency resolution and the resolver A-environment-variables Area: environment variables A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-interacts-with-crates.io Area: interaction with registries A-registry-authentication Area: registry authentication and authorization (authn authz) A-semver Area: semver specifications, version matching, etc. A-testing-cargo-itself Area: cargo's tests disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants