### What does this PR try to resolve?
When looking to add shallow fetch for the git CLI, I found it hard to
tell what we test. This tries to clean up the tests and make it easier
to add new ones. My hope is to later generalize the test bodies so we
can more easily test any git backend in any combination of shallow or
deep fetches.
This is part of #13285.
### How should we test and review this PR?
### Additional information
### What does this PR try to resolve?
As #10358 said, `When a build.rs script emits
cargo:rerun-if-env-changed, it is not re-run when the value of the
specified variable is changed via the env configuration.`
Fixes#10358
### How should we test and review this PR?
Add test bofore fixing to reflect the issue, the next commit fixed it.
### Additional information
The PR is a sucessor of https://github.com/rust-lang/cargo/pull/14058,
so the previous dicussion can be refer to it.
`check_repo_state` checks the entire git repo status.
This is usually fine if you have only a few packages in a workspace.
For huge monorepos, it may hit performance issues.
For example,
on awslabs/aws-sdk-rust@2cbd34d
the workspace has roughly 434 members to publish.
`git ls-files` reported us 204379 files in this Git repository.
That means git may need to check status of all files 434 times.
That would be `204379 * 434 = 88,700,486` checks!
Moreover, the current algorithm is finding the intersection of
`PathSource::list_files` and `git status`.
It is an `O(n^2)` check.
Let's assume files are evenly distributed into each package,
so roughly 470 files per package.
If we're unlucky to have some dirty files, say 100 files.
We will have to do `470 * 100 = 47,000` times of path comparisons.
Even worse, because we `git status` everything in the repo,
we'll have to it for all members,
even when those dirty files are not part of the current package in question.
So it becomes `470 * 100 * 434 = 20,398,000`!
Instead of comparing with the status of the entire repository,
this patch use the magic pathspec[1] to tell git only reports
paths that match a certain path prefix.
This wouldn't help the `O(n^2)` algorithm,
but at least it won't check dirty files outside the current package.
Also, we don't `git status` against entire git worktree/index anymore.
[1]: https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefpathspecapathspec
This PR contains the following updates:
| Package | Type | Update | Change |
|---|---|---|---|
| alpine | final | minor | `3.20` -> `3.21` |
---
### Configuration
📅 **Schedule**: Branch creation - "before 5am on the first day of the
month" (UTC), Automerge - At any time (no schedule defined).
🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.
♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.
🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.
---
- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box
---
This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/rust-lang/cargo).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS44NS4wIiwidXBkYXRlZEluVmVyIjoiMzkuODUuMCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciIsImxhYmVscyI6W119-->
### What does this PR try to resolve?
This adds a special case for checking source files are symlinks
and have been modified when under a VCS control
This is required because those paths may link to a file outside the
current package root, but still under the git workdir, affecting the
final packaged `.crate` file.
### How should we test and review this PR?
Pretty similar to #14966, as a part of #14967.
This may have potential performance issue. If a package contains
thousands of symlinks, Cargo will fire `git status` for each of them.
Not sure if we want to do anything proactively now.
The introduction of the `PathEntry` struct gives us more room for
storing file metadata to satisfiy use cases in the future. For
instances,
* Knowing a source file is a symlink and resolving it when packaging on
Windows
* #5664
* #14965
* Knowing more about a file's metadata (e.g. permission bits from Git)
* #4413
* #8006
* Provide richer information for `cargo package --list`, for example
JSON output mode
* #11666
* #13331
* #13953
metdata path fields may point to a dirty symlilnk and cause
duplicate report. This commit combines `dirty_metadata_paths`
and `dirty_symlinks` into one so is able to de-duplicate them.
This adds a special case for checking source files are symlinks
and have been modified when under a VCS control
This is required because those paths may link to a file outside the
current package root, but still under the git workdir, affecting the
final packaged `.crate` file.
This may have potential performance issue. If a package contains
thousands of symlinks, Cargo will fire `git status` for each of them.
This is helpful for VCS status check.
Paths emitted by PathSource are always under package root,
We lose the track of file type info of paths under symlink dirs,
so we need this extra bit of information.
### What does this PR try to resolve?
`cargo package` will warn users when git `core.symlinks` is `false`
and some symlinks were checked out as plain files during packaging.
Git config [`core.symlinks`] defaults to true when unset.
In git-for-windows (and git as well),
the config should be set to false explicitly when the repo was created,
if symlink support wasn't detected [^1].
We assume the config was always set at creation time and never changed.
So, if it is true, we don't bother users with any warning.
[^1]:
f1241afcc7/setup.c (L2394-L2403)
[`core.symlinks`]:
https://git-scm.com/docs/git-config#Documentation/git-config.txt-coresymlinks
### How should we test and review this PR?
CI passes.
This shares two commits 42dc4eff43ce480350979e07c8605e0f12983b79 and
c8c8223c265ba8e7ea4cd29f829583ce786834f6 with #14981.
I didn't commit to fix all symlink issues all at once.
This PR demonstrates how we could leverage metadata in `PathEntry`.
Maybe later we can really follow plain-text symlinks and resolve the
issue.
### Additional information
cc #5664
`cargo package` will warn users when git `core.symlinks` is `false`
and some symlinks were checked out as plain files during packaging.
Git config [`core.symlinks`] defaults to true when unset.
In git-for-windows (and git as well),
the config should be set to false explicitly when the repo was created,
if symlink support wasn't detected [^1].
We assume the config was always set at creation time and never changed.
So, if it is true, we don't bother users with any warning.
[^1]: <f1241afcc7/setup.c (L2394-L2403)>
[`core.symlinks`]: https://git-scm.com/docs/git-config#Documentation/git-config.txt-coresymlinks
This gives us more room to store file metadata.
For example,
* knowing a source file is a symlink and resolving it when packaging,
* providing a rich JSON output for `cargo package --list`
* enriching the `.cargo-vcs-info.json` with copied/symlinked file info
### What does this PR try to resolve?
This was found during some recent works around `cargo package`.
The purpose of it is showing the caller's line number when panicking.
### How should we test and review this PR?
Wait for CI. Nothing really change though.
### What does this PR try to resolve?
rust-lang/rust#122565 adds a new line to thread panic output.
To make the current test suites works on stable, beta, and nightly,
similar to rust-lang/cargo#14602,
this relaxes the assertion around that by globbing everything.
### What does this PR try to resolve?
This PR relaxes the `bad_crate_type` test to have it only match the
prefix of the unknown crate type error message emitted by rustc.
This is so that the cargo test isn't sensitive to (future) suggestions
for known crate types that rustc may emit to help the user.
### How should we test and review this PR?
This test should already be run as part of cargo CI. (This is definitely
run as part of rust-lang/rust CI, lol.)
### Additional information
rust-lang/rust side PR that's trying to add a suffix to the bad crate
type error message to list all valid `--crate-type` values:
https://github.com/rust-lang/rust/pull/134720.
Without relaxing this test, the rust-lang/rust side PR [fails
with](https://github.com/rust-lang/rust/pull/134720#issuecomment-2561993224):
<details>
<summary>rust-lang/rust CI fail message</summary>
(Ignore the missing colon after `unknown crate type`)
```
---- bad_config::bad_crate_type stdout ----
running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/cargo build -v`
thread 'bad_config::bad_crate_type' panicked at src/tools/cargo/tests/testsuite/bad_config.rs:434:10:
---- expected: tests/testsuite/bad_config.rs:424:27
++++ actual: stderr
1 1 | [ERROR] failed to run `rustc` to learn about crate-type bad_type information
2 2 |
3 3 | Caused by:
4 4 | process didn't exit successfully: `rustc - --crate-name ___ --print=file-names --crate-type bad_type` ([EXIT_STATUS]: 1)
5 5 | --- stderr
6 - [ERROR] unknown crate type: `bad_type`
Error: 6 + [ERROR] unknown crate type `bad_type`, expected one of: `bin`, `cdylib`, `dylib`, `lib`, `proc-macro`, `rlib`
7 7 |
```
</details>
Discussed at
https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/Reblessing.20a.20cargo.20test.
rust-lang/rust#122565 adds a new line to thread panic output.
To make the current test suites works on stable, beta, and nightly,
similar to rust-lang/cargo#14602,
this relaxes the assertion around that by globbing everything.
### What does this PR try to resolve?
Do nothing but move code around
* `cargo_package.rs` -> `cargo_package/mod.rs`
* Extract vcs info helpers to `cargo_package/vcs.rs`
* Extract verification helpers to `cargo_package/verify.rs`
Doing so because I realized how big `cargo_package.rs` has grown.
Like, the vcs helpers will continue growing with #14981 and potentially
other optimizations.
### How should we test and review this PR?
All tests pass.
### What does this PR try to resolve?
This adds a special case for `package.{readme,license-file}`
to Git VCS status check.
If they were specified with paths outside the current package root,
but still under git workdir, Cargo checks git status of those files
to determine if they were dirty.
We don't need to take care of other fields with path values because
* `PathSource` only list files under the package root.
Things like `target.path` works for `cargo build`, but won't be
included in `.crate` file from `cargo publish`.
* The only exceptions are `package.readme`/`package.license-file`.
Cargo would copy files over if they are outside package root.
### How should we test and review this PR?
While this doesn't fix ever case listed in #14967,
it at least fixes one of them.
This adds a special case for `package.{readme,license-file}`
to Git VCS status check.
If they were specified with paths outside the current package root,
but still under git workdir, Cargo checks git status of those files
to determine if they were dirty.
We don't need to take care of other fields with path values because
* `PathSource` only list files under the package root.
Things like `target.path` works for `cargo build`, but won't be
included in `.crate` file from `cargo publish`.
* The only exceptions are `package.readme`/`package.license-file`.
Cargo would copy files over if they are outside package root.
This is a test showing corner cases that dirty files outside
the package being packaging actually made the `.crate` file dirty.
However, `cargo package` and `.cargo_vcs_info.json` didn't capture it.
### What does this PR try to resolve?
So we don't need to `p.to_str().unwrap()`
and are able to pass different types for each argument
### How should we test and review this PR?
No response.
<!--
Thanks for submitting a pull request 🎉! Here are some tips for you:
* If this is your first contribution, read "Cargo Contribution Guide"
first:
https://doc.crates.io/contrib/
* Run `cargo fmt --all` to format your code changes.
* Small commits and pull requests are always preferable and easy to
review.
* If your idea is large and needs feedback from the community, read how:
https://doc.crates.io/contrib/process/#working-on-large-features
* Cargo takes care of compatibility. Read our design principles:
https://doc.crates.io/contrib/design.html
* When changing help text of cargo commands, follow the steps to
generate docs:
https://github.com/rust-lang/cargo/tree/master/src/doc#building-the-man-pages
* If your PR is not finished, set it as "draft" PR or add "WIP" in its
title.
* It's ok to use the CI resources to test your PR, but please don't
abuse them.
### What does this PR try to resolve?
Explain the motivation behind this change.
A clear overview along with an in-depth explanation are helpful.
You can use `Fixes #<issue number>` to associate this PR to an existing
issue.
### How should we test and review this PR?
Demonstrate how you test this change and guide reviewers through your
PR.
With a smooth review process, a pull request usually gets reviewed
quicker.
If you don't know how to write and run your tests, please read the
guide:
https://doc.crates.io/contrib/tests
### Additional information
Other information you want to mention in this PR, such as prior arts,
future extensions, an unresolved problem, or a TODO list.
-->
### What does this PR try to resolve?
Fixes#14154
~~Added some of the missing package metadata to the fingerprint so that
rebuilds are triggered correctly.~~
~~Also updated the test to prevent a future regression.~~
Moved the manifest metadata tracking to use dep info in favor of
fingerprint so that rebuilds are only triggered if the metadata is
actually used.
This change moves the manifest metadata track to dep-info files
with the goal of reduce unneeded rebuilds when metadata is changed as
well fixing issues where builds are not retrigged due to metadata
changes when they should (ie. #14154)
Personally I liked that the test was only dependent on what really matters, the lack of presence of a particular filename. Now the test would fail if Cargo one day adds more (generated) files to the package.
Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>
### What does this PR try to resolve?
Despite being interned `SourceId::Eq` is not a `ptr::eq`. Which in turn
is because `SourceId`s concept of identity is a complete mess. The code
relies on having to IDs that are `Eq` but do not have the same values
for their fields.
As one measure of this `SourceId` has an `impl Hash` which does
something different from `fn full_hash` and `fn stable_hash`. Separately
`SourceIdInner` has a different implementation. Similar levels of
complexity exist for `Eq`. Every one of these `impl`s was added due to a
real bug/issue we've had that needs to stay fixed. Not all of witch are
reproducible enough to have made it into our test suite.
I [have some
ideas](https://github.com/rust-lang/cargo/pull/14665#issuecomment-2412077472)
for how to reorganize the types so that this is easier to reason about
and faster. But given the history and the complexity I want to move
extremely carefully.
### How should we test and review this PR?
The test pass, and it's a one line change, but this still needs careful
review.
### Additional information
r? @ehuss I remember you and Alex working very hard to track down most
of these bugs.
