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

Set and verify all MSRVs in CI #12654

Merged
merged 4 commits into from
Oct 8, 2023
Merged

Set and verify all MSRVs in CI #12654

merged 4 commits into from
Oct 8, 2023

Conversation

epage
Copy link
Contributor

@epage epage commented Sep 11, 2023

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

@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2023

r? @weihanglo

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

@rustbot rustbot added A-cfg-expr Area: Platform cfg expressions A-environment-variables Area: environment variables A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-registry-authentication Area: registry authentication and authorization (authn authz) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2023
@epage epage force-pushed the msrv branch 3 times, most recently from 0de2b68 to a3699a7 Compare September 11, 2023 13:54
@@ -3,6 +3,7 @@ name = "cargo-credential-1password"
version = "0.4.0"
edition.workspace = true
license.workspace = true
rust-version = "1.70.0" # MSRV:3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From #12381 (comment)

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.

What we could do is swap workspace.rust-version so it is MSRV:3 and so these packages will be "untouched" despite getting MSRV bumps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weihanglo any thoughts on whether workspace.package.rust-version should track the MSRV for internal packages (MSRV:1) or user-facing packages (MSRV:3)?

Copy link
Member

Choose a reason for hiding this comment

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

It seems good starting from tracking MSRV:1, given it's more likely every user-facing package was an internal package from the beginning.

@epage epage force-pushed the msrv branch 4 times, most recently from 5b667f4 to e01878c Compare September 20, 2023 00:38
epage added a commit to epage/cargo that referenced this pull request Sep 20, 2023
1. Its extra churn to be forced to update parch releases
2. rust-lang#12654 adds `cargo hack` which doesn't handle MSRV patch versions
   well
bors added a commit that referenced this pull request Sep 20, 2023
chore(ci): Ignore patch version in MSRV

1. Its extra churn to be forced to update patch releases
2. #12654 adds `cargo hack` which doesn't handle MSRV patch versions well
@bors
Copy link
Contributor

bors commented Oct 1, 2023

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

@rustbot rustbot added A-credential-provider Area: credential provider for storing and retreiving credentials A-home Area: the `home` crate labels Oct 2, 2023
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.

Looks pretty good. Sorry for the long wait.

.github/renovate.json5 Outdated Show resolved Hide resolved
@@ -3,6 +3,7 @@ name = "cargo-platform"
version = "0.1.5"
edition.workspace = true
license.workspace = true
rust-version = "1.70.0" # MSRV:3
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems like a magic and is prone to remove? Do we have a way to avoid a remove even when maintainers aren't familiar with the msrv CI job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing I could think of is to add additional text after saying this is a renovatebot marker.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Or MSRV_DO_NOT_REMOVE:3 😆. Let's move on and deal with it later

bors added a commit that referenced this pull request Oct 3, 2023
Prep for automating MSRV management

### What does this PR try to resolve?

These are the simpler / less controversial parts of #12654 ensuring we get them in now, reducing the chance for conflicts and getting some of the benefits

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

CI

### Additional information
bors added a commit that referenced this pull request Oct 3, 2023
Prep for automating MSRV management

### What does this PR try to resolve?

These are the simpler / less controversial parts of #12654 ensuring we get them in now, reducing the chance for conflicts and getting some of the benefits

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

CI

### Additional information
@bors
Copy link
Contributor

bors commented Oct 4, 2023

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

@ehuss ehuss added this to the 1.75.0 milestone Oct 11, 2023
epage added a commit to epage/cargo that referenced this pull request Jan 8, 2024
Based on when we lost got an update, my best guess is rust-lang#12654 broke it.

PR rust-lang#12775 is the last MSRV we got.  That was merged before rust-lang#12654 and rust-lang#13106.
That makes rust-lang#12654 the most likely culprit.

Looking at the logs:
```
DEBUG: Matched 24 file(s) for manager regex: Cargo.toml, benches/benchsuite/Cargo.toml, benches/capture/Cargo.toml, crates/cargo-platform/Cargo.toml, crates/cargo-test-macro/Cargo.toml, crates/cargo-test-support/Cargo.toml, crates/cargo-test-support/containers/apache/bar/Cargo.toml, crates/cargo-test-support/containers/sshd/bar/Cargo.toml, crates/cargo-util-schemas/Cargo.toml, crates/cargo-util/Cargo.toml, crates/crates-io/Cargo.toml, crates/home/Cargo.toml, crates/mdman/Cargo.toml, crates/resolver-tests/Cargo.toml, crates/rustfix/Cargo.toml, crates/semver-check/Cargo.toml, crates/xtask-build-man/Cargo.toml, crates/xtask-bump-check/Cargo.toml, crates/xtask-stale-label/Cargo.toml, credential/cargo-credential-1password/Cargo.toml, credential/cargo-credential-libsecret/Cargo.toml, credential/cargo-credential-macos-keychain/Cargo.toml, credential/cargo-credential-wincred/Cargo.toml, credential/cargo-credential/Cargo.toml
...
DEBUG: manager extract durations (ms)
{
  "managers": {
    "dockerfile": 30,
    "github-actions": 38,
    "regex": 386,
    "cargo": 855
  }
}
DEBUG: Found cargo package files
DEBUG: Found dockerfile package files
DEBUG: Found github-actions package files
DEBUG: Found 25 package file(s)
```
Our regex managers have the files matched
but no regex manager packages are found.
I think this means that the name association failed or the regex within
the file content failed.

The differences between cargo and my other projects
- Use of `:`
- `depNameTemplate`
- Presence of `\b`

As a first attempt, I'm going to switch `\b` to `\\b` to be like the
other escaped regex values.
epage added a commit to epage/cargo that referenced this pull request Jan 8, 2024
With rust-lang#13254, we found MSRV updating is broken.
PR rust-lang#12775 is the last MSRV we got.
That was merged before rust-lang#12654 and rust-lang#13106.
That makes rust-lang#12654 the most likely culprit.

Looking at the logs:
```
DEBUG: Matched 24 file(s) for manager regex: Cargo.toml, benches/benchsuite/Cargo.toml, benches/capture/Cargo.toml, crates/cargo-platform/Cargo.toml, crates/cargo-test-macro/Cargo.toml, crates/cargo-test-support/Cargo.toml, crates/cargo-test-support/containers/apache/bar/Cargo.toml, crates/cargo-test-support/containers/sshd/bar/Cargo.toml, crates/cargo-util-schemas/Cargo.toml, crates/cargo-util/Cargo.toml, crates/crates-io/Cargo.toml, crates/home/Cargo.toml, crates/mdman/Cargo.toml, crates/resolver-tests/Cargo.toml, crates/rustfix/Cargo.toml, crates/semver-check/Cargo.toml, crates/xtask-build-man/Cargo.toml, crates/xtask-bump-check/Cargo.toml, crates/xtask-stale-label/Cargo.toml, credential/cargo-credential-1password/Cargo.toml, credential/cargo-credential-libsecret/Cargo.toml, credential/cargo-credential-macos-keychain/Cargo.toml, credential/cargo-credential-wincred/Cargo.toml, credential/cargo-credential/Cargo.toml
...
DEBUG: manager extract durations (ms)
{
  "managers": {
    "dockerfile": 30,
    "github-actions": 38,
    "regex": 386,
    "cargo": 855
  }
}
DEBUG: Found cargo package files
DEBUG: Found dockerfile package files
DEBUG: Found github-actions package files
DEBUG: Found 25 package file(s)
```
Our regex managers have the files matched
but no regex manager packages are found.
I think this means that the name association failed or the regex within
the file content failed.

The differences between cargo and my other projects
- Use of `:`
- `depNameTemplate`
- Presence of `\b`

As a first attempt, I'm going to switch `\b` to `\\b` to be like the
other escaped regex values.
bors added a commit that referenced this pull request Jan 8, 2024
chore(ci): Shot-in-the-dark fix for MSRV updating

With #13254, we found MSRV updating is broken.
PR #12775 is the last MSRV we got.
That was merged before #12654 and #13106.
That makes #12654 the most likely culprit.

Looking at the logs:
```
DEBUG: Matched 24 file(s) for manager regex: Cargo.toml, benches/benchsuite/Cargo.toml, benches/capture/Cargo.toml, crates/cargo-platform/Cargo.toml, crates/cargo-test-macro/Cargo.toml, crates/cargo-test-support/Cargo.toml, crates/cargo-test-support/containers/apache/bar/Cargo.toml, crates/cargo-test-support/containers/sshd/bar/Cargo.toml, crates/cargo-util-schemas/Cargo.toml, crates/cargo-util/Cargo.toml, crates/crates-io/Cargo.toml, crates/home/Cargo.toml, crates/mdman/Cargo.toml, crates/resolver-tests/Cargo.toml, crates/rustfix/Cargo.toml, crates/semver-check/Cargo.toml, crates/xtask-build-man/Cargo.toml, crates/xtask-bump-check/Cargo.toml, crates/xtask-stale-label/Cargo.toml, credential/cargo-credential-1password/Cargo.toml, credential/cargo-credential-libsecret/Cargo.toml, credential/cargo-credential-macos-keychain/Cargo.toml, credential/cargo-credential-wincred/Cargo.toml, credential/cargo-credential/Cargo.toml
...
DEBUG: manager extract durations (ms)
{
  "managers": {
    "dockerfile": 30,
    "github-actions": 38,
    "regex": 386,
    "cargo": 855
  }
}
DEBUG: Found cargo package files
DEBUG: Found dockerfile package files
DEBUG: Found github-actions package files
DEBUG: Found 25 package file(s)
```
Our regex managers have the files matched
but no regex manager packages are found.
I think this means that the name association failed or the regex within the file content failed.

The differences between cargo and my other projects
- Use of `:`
- `depNameTemplate`
- Presence of `\b`

As a first attempt, I'm going to switch `\b` to `\\b` to be like the other escaped regex values.
@djc djc mentioned this pull request Jan 9, 2024
Ghommie pushed a commit to tgstation/tgstation that referenced this pull request Feb 8, 2024
Updates tgs/precompile.sh hook to match what is deployed on campbell.

rust-lang/cargo#12654 has set a policy of setting this to be the latest
version-2, which kills any kind of signal this could have ever had.

cargo's subcrates like `home` are used in almost any complex rust
package, so this basically sets the tone for all packages and all crates
published after October 8th.

A min compiler version should be based on an actual need to use a
specific compiler version because of specific features that version has
or bugs that version doesn't have. This is signal. Setting to some
evergreen value as a matter of course is not signal, its noise.

I will not subject myself nor our downstreams to such nonsense.
SkyratBot pushed a commit to Skyrat-SS13/Skyrat-tg that referenced this pull request Feb 8, 2024
Updates tgs/precompile.sh hook to match what is deployed on campbell.

rust-lang/cargo#12654 has set a policy of setting this to be the latest
version-2, which kills any kind of signal this could have ever had.

cargo's subcrates like `home` are used in almost any complex rust
package, so this basically sets the tone for all packages and all crates
published after October 8th.

A min compiler version should be based on an actual need to use a
specific compiler version because of specific features that version has
or bugs that version doesn't have. This is signal. Setting to some
evergreen value as a matter of course is not signal, its noise.

I will not subject myself nor our downstreams to such nonsense.
NaakaKo pushed a commit to Bird-Lounge/Skyraptor-SS13 that referenced this pull request Feb 8, 2024
Updates tgs/precompile.sh hook to match what is deployed on campbell.

rust-lang/cargo#12654 has set a policy of setting this to be the latest
version-2, which kills any kind of signal this could have ever had.

cargo's subcrates like `home` are used in almost any complex rust
package, so this basically sets the tone for all packages and all crates
published after October 8th.

A min compiler version should be based on an actual need to use a
specific compiler version because of specific features that version has
or bugs that version doesn't have. This is signal. Setting to some
evergreen value as a matter of course is not signal, its noise.

I will not subject myself nor our downstreams to such nonsense.
NovaBot13 pushed a commit to NovaSector/NovaSector that referenced this pull request Feb 8, 2024
Updates tgs/precompile.sh hook to match what is deployed on campbell.

rust-lang/cargo#12654 has set a policy of setting this to be the latest
version-2, which kills any kind of signal this could have ever had.

cargo's subcrates like `home` are used in almost any complex rust
package, so this basically sets the tone for all packages and all crates
published after October 8th.

A min compiler version should be based on an actual need to use a
specific compiler version because of specific features that version has
or bugs that version doesn't have. This is signal. Setting to some
evergreen value as a matter of course is not signal, its noise.

I will not subject myself nor our downstreams to such nonsense.
TaleStationBot pushed a commit to TaleStation/TaleStation that referenced this pull request Feb 8, 2024
Updates tgs/precompile.sh hook to match what is deployed on campbell.

rust-lang/cargo#12654 has set a policy of setting this to be the latest
version-2, which kills any kind of signal this could have ever had.

cargo's subcrates like `home` are used in almost any complex rust
package, so this basically sets the tone for all packages and all crates
published after October 8th.

A min compiler version should be based on an actual need to use a
specific compiler version because of specific features that version has
or bugs that version doesn't have. This is signal. Setting to some
evergreen value as a matter of course is not signal, its noise.

I will not subject myself nor our downstreams to such nonsense.
SomeRandomOwl pushed a commit to NovaSector/NovaSector that referenced this pull request Feb 8, 2024
* Disable rust version checking in tgs precompile.sh hook (#81319)

Updates tgs/precompile.sh hook to match what is deployed on campbell.

rust-lang/cargo#12654 has set a policy of setting this to be the latest
version-2, which kills any kind of signal this could have ever had.

cargo's subcrates like `home` are used in almost any complex rust
package, so this basically sets the tone for all packages and all crates
published after October 8th.

A min compiler version should be based on an actual need to use a
specific compiler version because of specific features that version has
or bugs that version doesn't have. This is signal. Setting to some
evergreen value as a matter of course is not signal, its noise.

I will not subject myself nor our downstreams to such nonsense.

* Disable rust version checking in tgs precompile.sh hook

---------

Co-authored-by: Kyle Spier-Swenson <kyleshome@gmail.com>
Iajret pushed a commit to Fluffy-Frontier/FluffySTG that referenced this pull request Feb 8, 2024
* Disable rust version checking in tgs precompile.sh hook (#81319)

Updates tgs/precompile.sh hook to match what is deployed on campbell.

rust-lang/cargo#12654 has set a policy of setting this to be the latest
version-2, which kills any kind of signal this could have ever had.

cargo's subcrates like `home` are used in almost any complex rust
package, so this basically sets the tone for all packages and all crates
published after October 8th.

A min compiler version should be based on an actual need to use a
specific compiler version because of specific features that version has
or bugs that version doesn't have. This is signal. Setting to some
evergreen value as a matter of course is not signal, its noise.

I will not subject myself nor our downstreams to such nonsense.

* Disable rust version checking in tgs precompile.sh hook

---------

Co-authored-by: Kyle Spier-Swenson <kyleshome@gmail.com>
Useroth pushed a commit to Skyrat-SS13/Skyrat-tg that referenced this pull request Feb 8, 2024
)

* Disable rust version checking in tgs precompile.sh hook (#81319)

Updates tgs/precompile.sh hook to match what is deployed on campbell.

rust-lang/cargo#12654 has set a policy of setting this to be the latest
version-2, which kills any kind of signal this could have ever had.

cargo's subcrates like `home` are used in almost any complex rust
package, so this basically sets the tone for all packages and all crates
published after October 8th.

A min compiler version should be based on an actual need to use a
specific compiler version because of specific features that version has
or bugs that version doesn't have. This is signal. Setting to some
evergreen value as a matter of course is not signal, its noise.

I will not subject myself nor our downstreams to such nonsense.

* Disable rust version checking in tgs precompile.sh hook

---------

Co-authored-by: Kyle Spier-Swenson <kyleshome@gmail.com>
lessthnthree pushed a commit to effigy-se/effigy-se that referenced this pull request Feb 8, 2024
Updates tgs/precompile.sh hook to match what is deployed on campbell.

rust-lang/cargo#12654 has set a policy of setting this to be the latest
version-2, which kills any kind of signal this could have ever had.

cargo's subcrates like `home` are used in almost any complex rust
package, so this basically sets the tone for all packages and all crates
published after October 8th.

A min compiler version should be based on an actual need to use a
specific compiler version because of specific features that version has
or bugs that version doesn't have. This is signal. Setting to some
evergreen value as a matter of course is not signal, its noise.

I will not subject myself nor our downstreams to such nonsense.
Jolly-66 pushed a commit to TaleStation/TaleStation that referenced this pull request Feb 9, 2024
….sh hook (#9687)

Original PR: tgstation/tgstation#81319
-----
Updates tgs/precompile.sh hook to match what is deployed on campbell.

rust-lang/cargo#12654 has set a policy of setting this to be the latest
version-2, which kills any kind of signal this could have ever had.

cargo's subcrates like `home` are used in almost any complex rust
package, so this basically sets the tone for all packages and all crates
published after October 8th.

A min compiler version should be based on an actual need to use a
specific compiler version because of specific features that version has
or bugs that version doesn't have. This is signal. Setting to some
evergreen value as a matter of course is not signal, its noise.

I will not subject myself nor our downstreams to such nonsense.

---------

Co-authored-by: Kyle Spier-Swenson <kyleshome@gmail.com>
stupendoussuperpowers pushed a commit to stupendoussuperpowers/cargo that referenced this pull request Feb 28, 2024
With rust-lang#13254, we found MSRV updating is broken.
PR rust-lang#12775 is the last MSRV we got.
That was merged before rust-lang#12654 and rust-lang#13106.
That makes rust-lang#12654 the most likely culprit.

Looking at the logs:
```
DEBUG: Matched 24 file(s) for manager regex: Cargo.toml, benches/benchsuite/Cargo.toml, benches/capture/Cargo.toml, crates/cargo-platform/Cargo.toml, crates/cargo-test-macro/Cargo.toml, crates/cargo-test-support/Cargo.toml, crates/cargo-test-support/containers/apache/bar/Cargo.toml, crates/cargo-test-support/containers/sshd/bar/Cargo.toml, crates/cargo-util-schemas/Cargo.toml, crates/cargo-util/Cargo.toml, crates/crates-io/Cargo.toml, crates/home/Cargo.toml, crates/mdman/Cargo.toml, crates/resolver-tests/Cargo.toml, crates/rustfix/Cargo.toml, crates/semver-check/Cargo.toml, crates/xtask-build-man/Cargo.toml, crates/xtask-bump-check/Cargo.toml, crates/xtask-stale-label/Cargo.toml, credential/cargo-credential-1password/Cargo.toml, credential/cargo-credential-libsecret/Cargo.toml, credential/cargo-credential-macos-keychain/Cargo.toml, credential/cargo-credential-wincred/Cargo.toml, credential/cargo-credential/Cargo.toml
...
DEBUG: manager extract durations (ms)
{
  "managers": {
    "dockerfile": 30,
    "github-actions": 38,
    "regex": 386,
    "cargo": 855
  }
}
DEBUG: Found cargo package files
DEBUG: Found dockerfile package files
DEBUG: Found github-actions package files
DEBUG: Found 25 package file(s)
```
Our regex managers have the files matched
but no regex manager packages are found.
I think this means that the name association failed or the regex within
the file content failed.

The differences between cargo and my other projects
- Use of `:`
- `depNameTemplate`
- Presence of `\b`

As a first attempt, I'm going to switch `\b` to `\\b` to be like the
other escaped regex values.
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-credential-provider Area: credential provider for storing and retreiving credentials A-environment-variables Area: environment variables A-home Area: the `home` crate A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-registry-authentication Area: registry authentication and authorization (authn authz) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants