Skip to content

Conversation

linyihai
Copy link
Contributor

@linyihai linyihai commented Aug 6, 2024

What does this PR try to resolve?

Fixes #14354

This approach chose to not generate a .cargo_vcs_info.json for bare commit git repo.

How should we test and review this PR?

Compare the test changes before and after the two commits

Additional information

@rustbot
Copy link
Collaborator

rustbot commented Aug 6, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
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 Command-package S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2024
@linyihai linyihai force-pushed the issue-14354 branch 2 times, most recently from e33f540 to c8f2734 Compare August 6, 2024 09:27
@linyihai
Copy link
Contributor Author

linyihai commented Aug 6, 2024

It hits the API limit

#2 [internal] load metadata for docker.io/library/alpine:3.20
#2 ERROR: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/alpine/manifests/sha256:eddacbc7e24bf8799a4ed3cdcfa50d4b88a323695ad80f317b6629883b2c2a78: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

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.

Thank you!

@weihanglo weihanglo added the T-cargo Team: Cargo label Aug 6, 2024
@weihanglo
Copy link
Member

weihanglo commented Aug 6, 2024

@rfcbot fcp merge

This insta-stabilizes an edge case of .cargo_vcs_info.json:

When a Git repo has no commits, the file content would be

{
  "git": {
    # there is no "sha1" field because `HEAD` is missing
    "dirty": true
  },
  "path_in_vcs": "..."
}

Thinking backward from #13695, the requirement is to have commit hash as well as the path_in_vcs field for other tools if they want to post-process further. The current approach this PR takes, generating vcs info file even with no sha field, is still useful for preserving path_in_vcs.

@rfcbot
Copy link

rfcbot commented Aug 6, 2024

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 Aug 6, 2024
@epage
Copy link
Contributor

epage commented Aug 6, 2024

@weihanglo there are several solutions for this

  • Don't include the file
  • Do whatever non-git locations do
  • not have the git table

and likely more.

Is there a motivation for the proposed solution? I didn't see it here or in the issue.

@weihanglo
Copy link
Member

I am sorry that I just did a really quick FCP without thinking hard on it.

I don't have a good answer. Thinking backward from #13695, the requirement is to have commit hash as well as the path_in_vcs field for other tools if they want to post-process further. The current approach this PR takes, generating vcs info file even with no sha field, is still useful for preserving path_in_vcs.

However, I personally doubt it is useful in real life, since that happens on in an edge case that no commit exists. I am fine with whatever approach we choose. We just need to fix the regression.

@linyihai
Copy link
Contributor Author

linyihai commented Aug 7, 2024

@weihanglo Thanks you rapidly response.

Sorry for that, I had change my mind. I'm not sure if the absence of this field will cause problems in other ways, so it's better to not include/generate the .cargo_vcs_info.json in this case.

Of course, this is only a reasonable change that I currently feel, which should be very rare for this scenario, and it may be better to maintain the consistency with the previous

git: git(p, src_files, &repo, &opts)?,
path_in_vcs,
}));
let git = git(p, src_files, &repo, &opts)?;
Copy link
Member

Choose a reason for hiding this comment

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

Could fn git() return a None when nothing needed to be recorded? So that we don't need skip_serializing_if for sha1 field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with you and updated it

@weihanglo
Copy link
Member

Agree that not generating vcs info file is also a reasonable approach to take. It stays with the old behavior (not generating when --allow-dirty presents) without blocking cargo package.

@weihanglo
Copy link
Member

Let's discuss different approaches in the issue then.

@weihanglo
Copy link
Member

@rustbot fcp cancel

until we settle down a solution.

@weihanglo
Copy link
Member

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented Aug 8, 2024

@weihanglo proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Aug 8, 2024
@weihanglo
Copy link
Member

Thanks for the contribution. This should be the least controversial way to fix this. See the discussion in #14354.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 8, 2024

📌 Commit 37cda2d 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 8, 2024
@weihanglo weihanglo added the beta-nominated Nominated to backport to the beta branch. label Aug 8, 2024
@bors
Copy link
Contributor

bors commented Aug 8, 2024

⌛ Testing commit 37cda2d with merge b66cad8...

@bors
Copy link
Contributor

bors commented Aug 8, 2024

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

@bors bors merged commit b66cad8 into rust-lang:master Aug 8, 2024
weihanglo pushed a commit to weihanglo/cargo that referenced this pull request Aug 9, 2024
Fix: `cargo package` failed on bare commit git repo.

### What does this PR try to resolve?
Fixes rust-lang#14354

This approach chose to not generate a `.cargo_vcs_info.json` for bare commit git repo.

### How should we test and review this PR?
Compare the test changes before and after the two commits

### Additional information
@linyihai linyihai deleted the issue-14354 branch August 9, 2024 01:04
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2024
Update cargo

7 commits in 0d8d22f83b066503f6b2b755925197e959e58b4f..2f738d617c6ead388f899802dd1a7fd66858a691
2024-08-08 12:54:24 +0000 to 2024-08-13 10:57:52 +0000
- chore: downgrade to openssl v1.1.1 (again) (rust-lang/cargo#14391)
- feat(trim-paths): rustdoc supports trim-paths for diagnostics (rust-lang/cargo#14389)
- Use longhand gitoxide path-spec patterns (rust-lang/cargo#14380)
- feat: Add `info` cargo subcommand (rust-lang/cargo#14141)
- CI: Switch macos aarch64 to nightly (rust-lang/cargo#14382)
- Use context instead of with_context (rust-lang/cargo#14377)
- Fix: `cargo package` failed on bare commit git repo. (rust-lang/cargo#14359)

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2024
Update cargo

7 commits in 0d8d22f83b066503f6b2b755925197e959e58b4f..2f738d617c6ead388f899802dd1a7fd66858a691
2024-08-08 12:54:24 +0000 to 2024-08-13 10:57:52 +0000
- chore: downgrade to openssl v1.1.1 (again) (rust-lang/cargo#14391)
- feat(trim-paths): rustdoc supports trim-paths for diagnostics (rust-lang/cargo#14389)
- Use longhand gitoxide path-spec patterns (rust-lang/cargo#14380)
- feat: Add `info` cargo subcommand (rust-lang/cargo#14141)
- CI: Switch macos aarch64 to nightly (rust-lang/cargo#14382)
- Use context instead of with_context (rust-lang/cargo#14377)
- Fix: `cargo package` failed on bare commit git repo. (rust-lang/cargo#14359)

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2024
Update cargo

7 commits in 0d8d22f83b066503f6b2b755925197e959e58b4f..2f738d617c6ead388f899802dd1a7fd66858a691
2024-08-08 12:54:24 +0000 to 2024-08-13 10:57:52 +0000
- chore: downgrade to openssl v1.1.1 (again) (rust-lang/cargo#14391)
- feat(trim-paths): rustdoc supports trim-paths for diagnostics (rust-lang/cargo#14389)
- Use longhand gitoxide path-spec patterns (rust-lang/cargo#14380)
- feat: Add `info` cargo subcommand (rust-lang/cargo#14141)
- CI: Switch macos aarch64 to nightly (rust-lang/cargo#14382)
- Use context instead of with_context (rust-lang/cargo#14377)
- Fix: `cargo package` failed on bare commit git repo. (rust-lang/cargo#14359)

r? ghost
@rustbot rustbot added this to the 1.82.0 milestone Aug 15, 2024
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Aug 29, 2024
Update cargo

7 commits in 0d8d22f83b066503f6b2b755925197e959e58b4f..2f738d617c6ead388f899802dd1a7fd66858a691
2024-08-08 12:54:24 +0000 to 2024-08-13 10:57:52 +0000
- chore: downgrade to openssl v1.1.1 (again) (rust-lang/cargo#14391)
- feat(trim-paths): rustdoc supports trim-paths for diagnostics (rust-lang/cargo#14389)
- Use longhand gitoxide path-spec patterns (rust-lang/cargo#14380)
- feat: Add `info` cargo subcommand (rust-lang/cargo#14141)
- CI: Switch macos aarch64 to nightly (rust-lang/cargo#14382)
- Use context instead of with_context (rust-lang/cargo#14377)
- Fix: `cargo package` failed on bare commit git repo. (rust-lang/cargo#14359)

r? ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-nominated Nominated to backport to the beta branch. Command-package S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to cargo package --allow-dirty for repo with no commit
6 participants