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

chore: remove version field from publish=false packages #13254

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

In 1.75 cargo allows versionless manifest.
We should keep up and dogfood ourselves.

How should we test and review this PR?

Additional information

@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2024

r? @epage

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

@rustbot rustbot added A-cli-help Area: built-in command-line help A-dependency-resolution Area: dependency resolution and the resolver A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. 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 Jan 5, 2024
@weihanglo weihanglo changed the title chore: remove version field from unpublished packages chore: remove version field from publish=false packages Jan 5, 2024
@weihanglo weihanglo force-pushed the versionless branch 2 times, most recently from 63b0489 to bfc9880 Compare January 5, 2024 21:34
@epage
Copy link
Contributor

epage commented Jan 5, 2024

@bors r+

This reminds me that I need to update cargo-release

@bors
Copy link
Collaborator

bors commented Jan 5, 2024

📌 Commit bfc9880 has been approved by epage

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 Jan 5, 2024
@bors
Copy link
Collaborator

bors commented Jan 5, 2024

⌛ Testing commit bfc9880 with merge 0e4ea9b...

bors added a commit that referenced this pull request Jan 5, 2024
chore: remove `version` field from `publish=false` packages
@weihanglo
Copy link
Member Author

https://github.com/rust-lang/cargo/actions/runs/7426946854/job/20211683866?pr=13254

I guess the renovatebot MSRV is broken since #12654?

@bors
Copy link
Collaborator

bors commented Jan 5, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 5, 2024
epage added a commit to epage/cargo that referenced this pull request Jan 6, 2024
Example: clap-rs/clap#4824

I'm hoping this will make it easier to see what is going on with
problems with RenovateBot, like our MSRVs not updating atm (rust-lang#13254).
bors added a commit that referenced this pull request Jan 6, 2024
chore: Add dependency dashboard

Example: clap-rs/clap#4824

I'm hoping this will make it easier to see what is going on with problems with RenovateBot, like our MSRVs not updating atm (#13254).
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.
@weihanglo
Copy link
Member Author

Should be fine after #13265. Let give it a try.

@bors r=epage

@bors
Copy link
Collaborator

bors commented Jan 10, 2024

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

@bors
Copy link
Collaborator

bors commented Jan 10, 2024

📌 Commit bfc9880 has been approved by epage

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jan 10, 2024

⌛ Testing commit bfc9880 with merge c027791...

@bors
Copy link
Collaborator

bors commented Jan 10, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 10, 2024
@epage
Copy link
Contributor

epage commented Jan 10, 2024

Oh right, we are verifying the MSRV locally which means all packages need to be able to be loaded.

We could verify this if cargo hack could package a workspace and then run tests on that.

@weihanglo weihanglo marked this pull request as draft January 10, 2024 21:50
@taiki-e
Copy link
Member

taiki-e commented Jan 17, 2024

if cargo hack could package a workspace and then run tests on that.

As far as I know, you can avoid this error by just passing the existing --no-private flag (instead of --ignore-private flag).

--no-private

Perform without publish = false crates. This is similar to --ignore-private, but is more powerful because this also prevents private crates from affecting lockfile and metadata.

(At least I confirmed this for a project that MSRV is 1.60. openrr/mesh-loader@4a30845)

@taiki-e
Copy link
Member

taiki-e commented Jan 17, 2024

CI failure is due to --no-private not handling workspace members with globs, well. I will work on a fix...

info: removing private crates from /Users/taiki/projects/forks/rust-lang/cargo/Cargo.toml
error: failed to compare /Users/taiki/projects/forks/rust-lang/cargo/crates/cargo-test-macro/Cargo.toml and /Users/taiki/projects/forks/rust-lang/cargo/crates/*/Cargo.toml: No such file or directory (os error 2)

@taiki-e
Copy link
Member

taiki-e commented Jan 17, 2024

CI failure is due to --no-private not handling workspace members with globs, well. I will work on a fix...

I have created a rough fix locally and it appears to be working well.

log
$ git log --graph --decorate --oneline | head -2
* d7d657528 (HEAD -> versionless) chore(ci): cargo-hack `--no-private`
* bfc9880b3 chore: remove `version` field from `publish=false` packages

$ cargo hack check --rust-version --workspace --no-private --no-dev-deps
info: --no-dev-deps and --no-private modify real `Cargo.toml` while cargo-hack is running and restores it when finished
info: skipped running on private package `cargo-test-macro`
info: skipped running on private package `cargo-test-support`
info: skipped running on private package `mdman`
info: skipped running on private package `resolver-tests`
info: skipped running on private package `semver-check`
info: skipped running on private package `xtask-build-man`
info: skipped running on private package `xtask-bump-check`
info: skipped running on private package `xtask-stale-label`
info: skipped running on private package `benchsuite`
info: skipped running on private package `capture`
info: running `rustup run 1.70 cargo check` on cargo-platform (1/12)
warning: /Users/taiki/projects/forks/rust-lang/cargo/crates/cargo-platform/Cargo.toml: unused manifest key: lints
<omitted 12 warnings about "unused manifest key: lints">
    Finished dev [unoptimized + debuginfo] target(s) in 0.23s

info: running `rustup run 1.70 cargo check` on home (2/12)
warning: /Users/taiki/projects/forks/rust-lang/cargo/credential/cargo-credential-libsecret/Cargo.toml: unused manifest key: lints
<omitted 12 warnings about "unused manifest key: lints">
    Finished dev [unoptimized + debuginfo] target(s) in 0.06s

info: running `rustup run 1.70 cargo check` on cargo-credential (3/12)
<omitted 12 warnings about "unused manifest key: lints">
warning: /Users/taiki/projects/forks/rust-lang/cargo/credential/cargo-credential-wincred/Cargo.toml: unused manifest key: lints
    Finished dev [unoptimized + debuginfo] target(s) in 0.06s

info: running `rustup run 1.70 cargo check` on rustfix (4/12)
warning: /Users/taiki/projects/forks/rust-lang/cargo/credential/cargo-credential-1password/Cargo.toml: unused manifest key: lints
<omitted 12 warnings about "unused manifest key: lints">
    Finished dev [unoptimized + debuginfo] target(s) in 0.07s

info: running `rustup run 1.70 cargo check` on cargo-credential-1password (5/12)
warning: /Users/taiki/projects/forks/rust-lang/cargo/credential/cargo-credential/Cargo.toml: unused manifest key: lints
<omitted 12 warnings about "unused manifest key: lints">
    Finished dev [unoptimized + debuginfo] target(s) in 0.07s

info: running `rustup run 1.73 cargo check` on cargo-util (6/12)
warning: /Users/taiki/projects/forks/rust-lang/cargo/credential/cargo-credential-wincred/Cargo.toml: unused manifest key `lints` (may be supported in a future version)
<omitted 12 warnings about "unused manifest key: lints">
    Finished dev [unoptimized + debuginfo] target(s) in 0.08s

info: running `rustup run 1.73 cargo check` on crates-io (7/12)
warning: /Users/taiki/projects/forks/rust-lang/cargo/crates/cargo-platform/Cargo.toml: unused manifest key `lints` (may be supported in a future version)
<omitted 12 warnings about "unused manifest key: lints">
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s

info: running `rustup run 1.73 cargo check` on cargo-util-schemas (8/12)
warning: /Users/taiki/projects/forks/rust-lang/cargo/credential/cargo-credential-macos-keychain/Cargo.toml: unused manifest key `lints` (may be supported in a future version)
<omitted 12 warnings about "unused manifest key: lints">
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s

info: running `rustup run 1.73 cargo check` on cargo (9/12)
<omitted 12 warnings about "unused manifest key: lints">
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s

info: running `rustup run 1.73 cargo check` on cargo-credential-libsecret (10/12)
warning: /Users/taiki/projects/forks/rust-lang/cargo/crates/home/Cargo.toml: unused manifest key `lints` (may be supported in a future version)
<omitted 12 warnings about "unused manifest key: lints">
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s

info: running `rustup run 1.73 cargo check` on cargo-credential-macos-keychain (11/12)
warning: /Users/taiki/projects/forks/rust-lang/cargo/crates/crates-io/Cargo.toml: unused manifest key `lints` (may be supported in a future version)
<omitted 12 warnings about "unused manifest key: lints">
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s

info: running `rustup run 1.73 cargo check` on cargo-credential-wincred (12/12)
warning: /Users/taiki/projects/forks/rust-lang/cargo/credential/cargo-credential-macos-keychain/Cargo.toml: unused manifest key `lints` (may be supported in a future version)
<omitted 12 warnings about "unused manifest key: lints">
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s

One thing to note is that the use of the --all-targets flag (that builds tests/examples) needed to be removed and the --no-dev-deps flag is needed. This is because the dev-dependencies use versionless manifests that cannot be read by the old compiler.

That said, we are not running tests with the old compiler, so I don't see a problem if the test suite cannot be built on the old compiler.

@bors
Copy link
Collaborator

bors commented Jan 19, 2024

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

@epage
Copy link
Contributor

epage commented Jan 20, 2024

As far as I know, you can avoid this error by just passing the existing --no-private flag (instead of --ignore-private flag).

This fixes this immediate problem but not others. For example, if rust-lang/rfcs#3537 gets merged, we are then blocked on setting our N-0 rust-versions to auto until its supported on all of our MSRVs.

@taiki-e
Copy link
Member

taiki-e commented Jan 24, 2024

CI failure is due to --no-private not handling workspace members with globs, well. I will work on a fix...

I have created a rough fix locally and it appears to be working well.

This fix has been released in cargo-hack 0.6.16.

In 1.75 cargo allows versionless manifest.
We should keep up and dogfood ourselves.
--no-private

Perform without publish = false crates. This is similar to
--ignore-private, but is more powerful because this also prevents
private crates from affecting lockfile and metadata.
.github/workflows/main.yml Outdated Show resolved Hide resolved
weihanglo and others added 2 commits January 24, 2024 21:18
One thing to note is that the use of the --all-targets flag (that builds
tests/examples) needed to be removed and the --no-dev-deps flag is needed.
This is because the dev-dependencies use versionless manifests that cannot
be read by the old compiler.

That said, we are not running tests with the old compiler, so I don't see
a problem if the test suite cannot be built on the old compiler.

Co-authored-by: Taiki Endo <te316e89@gmail.com>
@weihanglo weihanglo marked this pull request as ready for review January 25, 2024 02:43
@weihanglo
Copy link
Member Author

As far as I know, you can avoid this error by just passing the existing --no-private flag (instead of --ignore-private flag).

This fixes this immediate problem but not others. For example, if rust-lang/rfcs#3537 gets merged, we are then blocked on setting our N-0 rust-versions to auto until its supported on all of our MSRVs.

@epage do you think this is a blocker?

@epage
Copy link
Contributor

epage commented Jan 25, 2024

That was more of a "this isn't a panacea" to the larger MSRV conversation.

For me, I'm uncomfortable with it hacking up the source as it tests and I would prefer checking test targets but not enough to say "no" if you want to move forward with this knowing that there are those reservations.

@weihanglo
Copy link
Member Author

Got it. I think this is a good example for relevant MSRV conversation. This doesn't block anything so I'll just leave here, also for reminding us there are rooms for a better workflow :)

@weihanglo weihanglo marked this pull request as draft January 25, 2024 03:28
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.
@bors
Copy link
Collaborator

bors commented Mar 3, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli-help Area: built-in command-line help A-dependency-resolution Area: dependency resolution and the resolver A-documenting-cargo-itself Area: Cargo's documentation A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants