Use sha2 to calculate SHA256
crypto-hash seems to be unmaintained (last real commit on Feb 27, 2020) and forces duplicate crates due to outdated dependencies.
Minimal change to replace crypto-hash by sha2.
This was already attempted in https://github.com/rust-lang/cargo/pull/4545 (back in 2017).
Previously it actually was very hot and needed an entire CPU for itself
due to constant querying of progress information.
Now it's slowed down by consistently sleeping for a short amount of time.
This time should be short enough to not let the progress bar hold up the
overall progress of the fetch operation, hence the 10ms sleep time,
reducing the worst-case hold-up to 10ms.
The `git2` implementation can leverage that `git2` provides a consistent
view on the objects to be index, so it looks like 33% of the time is spent
receiving objects, and the rest of the time is used resolving them.
In `gitoxide`, there are two distinct phases and these are exposed by the way
we obtain progress for two separate phases. We have to do some math to renormalize
those to a single, continuous progress by mapping the values for 'amount of objects'
to the first half and second half of the progress bar respectively.
This has the advantage of having the first phase (receiving objects) end at 50%
and the second phase (resolving deltas) at 100%.
chore: Update base64
This removes one of cargo's duplicate dependencies as found by #11761.
`base64` is a bit of a controversial crate right now. It is going through large API changes, making it not as ergonomic for basic cases, which has ticked off a number of people. I kept it for now because its elsewhere in our dependency tree.
Byron already updated `prodash` to use the latest `parking_lot`
Remaining duplicates
- `hex` is blocked on `crypto-hash` which seems to no longer be maintained
- `hashbrown` is blocked on `indexmap` (updated in master) and `imara-diff`
- `humantime`, `env_logger`, `hermit-abi` are present from the optional `pretty_env_logger` dependency (why are we using optional deps? #6348)
- `windows-sys` is held back by `schannel`, `tempfile`, and `mio`
chore(ci): Enforce cargo-deny in CI
With #11448, we are pulling in a wide and deep dependency tree which makes it harder for us to track what we are pulling in over time.
I've been trying out [`cargo-deny`](https://github.com/EmbarkStudios/cargo-deny) on my projects and wanted to explore how useful it might be for cargo. atm I only have it configured to fail for unexpected licenses. We can also use its warnings to hunt down and remove duplicated dependencies to speed up our builds.
I did also enable advisories. We ignore the failure in a way to not block PRs or even show up as failure in PRs as PR authors are not responsible for dealing with these (unless its a new dep) and it can be intimidating as a contributor to see a failure and have no idea how to resolve it (as authors generally assume CI is green and failures are there fault)
I did not go too much further into what all `cargo-deny` can do; there might be more we can leverage.
gitoxide integration: fetch
This PR is the first step towards resolving #1171.
In order to get there, we integrate `gitoxide` into `cargo` in such a way that one can control its usage in nightly via `-Zgitoxide` or `Zgitoxide=<feature>[,featureN]`.
Planned features are:
* **fetch** - all fetches are done with `gitxide` (this PR)
* **shallow_index** - the crates index will be a shallow clone (_planned_)
* **shallow_deps** - git dependencies will be a shallow clone (_planned_)
* **checkout** - plain checkouts with `gitoxide` (_planned_)
The above list is a prediction and might change as we understand the requirements better.
### Testing and Transitioning
By default, everything stays as is. However, relevant tests can be re-runwith `gitoxide` using
```
RUSTFLAGS='--cfg always_test_gitoxide' cargo test git
```
There are about 200 tests with 'git' in their name and I plan to enable them one by one. That way the costs for CI stay managable (my first measurement with one test was 2min 30s), while allowing to take one step at a time.
Custom tests shall be added once we realize that more coverage is needed.
That way we should be able to maintain running `git2` and `gitoxide` side by side until we are willing to switch over to `gitoxide` entirely on stable cargo. Then turning on `git2` might be a feature toggle for a while until we finally remove it from the codebase.
_Please see the above paragraph as invitation for discussion, it's merely a basis to explore from and improve upon._
### Tasks
* [x] add feature toggle
* [x] setup test system with one currently successful test
* [x] implement fetch with `gitoxide` (MVP)
* [x] fetch progress
* [x] detect spurious errors
* [x] enable as many git tests as possible (and ignore what's not possible)
* [x] fix all git-related test failures (except for 1: built-in upload-pack, skipped for now)
* [x] validate that all HTTP handle options that come from `cargo` specific values are passed to `gitoxide`
* [x] a test to validate `git2` code can handle crates-index clones created with `gitoxide` and vice-versa
* [x] remove patches that enabled `gitoxide` enabled testing - it's not used anymore
* [x] ~~remove all TODOs and use crates-index version of `git-repository`~~ The remaining 2 TODO's are more like questions for the reviewer.
* [x] run all tests with gitoxide on the fastest platform as another parallel task
* [x] switch to released version
* [x] [Tasks from first review round](https://github.com/rust-lang/cargo/pull/11448#issuecomment-1374421517)
* [x] create a new `gitoxide` release and refer to the latest version from crates.io (instead of git-dependency)
* [x] [address 2nd review round comments](https://github.com/rust-lang/cargo/pull/11448#issuecomment-1426326996)
### Postponed Tasks
I suggest to go breadth-first and implement the most valuable features first, and then aim for a broad replacement of `git2`. What's left is details and improved compatibility with the `git2` implementation that will be required once `gitoxide` should become the default implementation on stable to complete the transition.
* **built-in support for serving the `file` protocol** (i.e. without using `git`). Simple cases like `clone` can probably be supported quickly, `fetch` needs more work though due to negotiation.
* SSH name fallbacks via a native (probably ~~libssh~~ (avoid LGPL) `libssh2` based) transport. Look at [this issue](https://github.com/rust-lang/cargo/issues/2399) for some history.
* additional tasks from [this tracking issue](https://github.com/Byron/gitoxide/issues/450#issue-1290858067)
### Proposed Workflow
I am now using [stacked git](https://stacked-git.github.io) to keep commits meaningful during development. This will also mean that before reviews I will force-push a lot as changes will be bucketed into their respective commits.
Once review officially begins I will stop force-pushing and create small commits to address review comments. That way it should be easier to understand how things change over time.
Those review-comments can certainly be squashed into one commit before merging.
_Please let me know if this is feasible or if there are other ways of working you prefer._
### Development notes
* unrelated: [this line](9827412fee/src/cargo/ops/registry.rs (L620)) refers to an issue that has since been resolved in `curl`.
* Additional tasks related to a correct fetch implementation are collected in this [tracking issue](https://github.com/Byron/gitoxide/issues/450). **These affect how well the HTTP transport can be configured, needs work**
* _authentication_ [is quite complex](37cad5bd7f/src/cargo/sources/git/utils.rs (L490)) and centred around making SSH connections work. This feature is currently the weakest in `gitoxide` as it simply uses `ssh` (the program) and calls it a day. No authentication flows are supported there yet and the goal would be to match `git` there at least (which it might already do by just calling `ssh`). Needs investigation. Once en-par with `git` I think `cargo` can restart the whole fetch operation to try different user names like before.
- the built-in `ssh`-program based transport can now understand permission-denied errors, but the capability isn't used after all since a builtin ssh transport is required.
* It would be possible to implement `git::Progress` and just ignore most of the calls, but that's known to be too slow as the implementation assumes a `Progress::inc()` call is as fast as an atomic increment and makes no attempt to reduce its calls to it.
* learning about [a way to get custom traits in `thiserror`](https://github.com/dtolnay/thiserror/issues/212) could make spurious error checks nicer and less error prone during maintenance. It's not a problem though.
* I am using `RUSTFLAGS=--cfg` to influence the entire build and unit-tests as environment variables didn't get through to the binary built and run for tests.
### Questions
* The way `gitoxide` is configured the user has the opportunity to override these values using more specific git options, for example using url specific http settings. This looks like a feature to me, but if it's not `gitoxide` needs to provide a way to disable applying these overrides. Please let me know what's desired here - my preference is to allow overrides.
* `gitoxide` currently opens repositories similar to how `git` does which respects git specific environment variables. This might be a deviation from how it was before and can be turned off. My preference is to see it as a feature.
### Prerequisite PRs
* https://github.com/rust-lang/cargo/pull/11602
This allows to use `gitoxide` for all fetch operations, boosting performance
for fetching the `crates.io` index by a factor of 2.2x, while being consistent
on all platforms.
For trying it, nightly builds of `cargo` can specify `-Zgitoxide=fetch`.
It's also possible to set the `__CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2=1` environment
variable (value matters), which is when `-Zgitoxide=none` can be used
to use `git2` instead.
Limitations
-----------
Note that what follows are current shortcomings that will be addressed in future PRs.
- it's likely that authentication around the `ssh` protocol will work differently in practice
as it uses the `ssh` program.
- clones from `file://` based crates indices will need the `git` binary to serve the locatl repository.
- the progress bar shown when fetching doesn't work like the orgiinal, but should already feel 'faster'.
patch can conflict on not activated packages
### What does this PR try to resolve?
In the resolver there is a data structure called a `conflicting_activations`, which records all the reasons a "better" version was not picked for the package being resolved. Normally, these are packages that are currently active that for one reason or another block one of the other versions. Several optimizations assumed that this was always true, even going so far as to use `.expect`. This was a mistake because when there's a `patch` involved there can be conflicts on a version that is not selected. So the correct behavior is to fall back to skip the optimizations and try all versions when a `conflicting_activations` are not active.
### How should we test and review this PR?
This adds two automated tests based on the reproductions in #7463 and #11336. So the test suite now covers this case. It can also be tested by reconstructing the repositories originally reported in those issues.
### Additional information
It could be that in the long term the correct fix is to figure out how to support patch without having a conflicting activation that is not activated. But that would be a much bigger change. And for now this assumption is only used in optimizations, so this gets people unstuck.
fix(toml): Provide a way to show unused manifest keys for dependencies
Dependencies have not been able to show unused manifest keys for some time, this problem partially resulted in #11329.
This problem is caused by having an `enum` when deserializing. To get around this you can use:
```rust
#[serde(flatten)]
other: BTreeMap<String, toml::Value>,
```
This collects any unused keys into `other` that can later be used to show warnings. This idea was suggested in a thread I cannot find but is mentioned in [serde#941](https://github.com/serde-rs/serde/issues/941).
Improve error for missing crate in --offline mode for sparse index
This changes sparse registries to instead return `NotFound` when a non-cached crate is requested in `--offline` mode.
The resolver can then suggest removing the `--offline` flag if resolution fails, which is a more helpful error than the one currently issued: `attempting to make an HTTP request, but --offline was specified`.
With this change, the behavior matches what is already done for git-based registries.
Closes#11276
This changes sparse registries to instead return not found when a non-cached crate is requested in offline mode.
The resolver can then suggest removing the --offline flag if resolution
fails.
feat(resolver): `-Zdirect-minimal-versions`
This is an alternative to `-Zminimal-versions` as discussed in #5657.
Problems with `-Zminimal-versions` includes
- Requires the root most dependencies to verify it and we then percolate that up the stack. This requires a massive level of cooperation to accomplish and so far there have been mixed results with it to the point that cargo's unstable
documentation discourages its use.
- Users expect `cargo check -Zminimal-versions` to force resolving to minimal but it doesn't as the default maximal resolve is compatible and requires `cargo update -Zminimal-versions`
- Different compatible versions might be selected, breaking interop between crates, changing feature unification, and breaking `-sys` crates without bad `links`
`-Zdirect-minimal-versions` instead only applies this rule to your
direct dependencies, allowing anyone in the stack to immediately adopt
it, independent of everyone else.
Special notes
- Living up to the name and the existing design, this ignores yanked
crates. This makes sense for `^1.1` version requirements but might
look weird for `^1.1.1` version requirements as it could select
`1.1.2`.
- This will error if an indirect dependency requires a newer version.
Your version requirement will need to capture what you use **and** all
of you dependencies. An alternative design would have tried to merge
the result of minimum versions for direct dependencies and maximum
versions for indirect dependencies. This would have been complex and
led to weird corner cases, making it harder to predict. I also suspect
the value gained would be relatively low as you can't verify that
version requirement in any other way.
- This also means discrepancies between `dependencies` and `dev-dependencies` are errors
- The error could be improved to call out that this was from minimal
versions but I felt getting this out now and starting to collect
feedback was more important.
One advantage of this approach over `-Zminimal-versions` is that it removes most of the problems that [cargo-minimal-versions](https://github.com/taiki-e/cargo-minimal-versions) tried to workaround.
As for the implementation, this might not be the most elegant solution but it works and we can always iterate and improve on it in the future.
- We keep the state as a `bool` throughout but compensate for that by explicitly creating a variable to abstract away constants
- The name changes depending on the context, from `direct_minimal_version` when dealing with the unstable flag to `first_minimal_version` when the concept of "direct" is lost to `first_version` when we split off the ordering concept into a separate variable
- Packages that respect `direct_minimal_versions` are determined by whether they are the top-level `summaries` that get past into `resolve`
### What does this PR try to resolve?
The primary use case is verifying version requirements to avoid depending on something newer than might be available in a dependent
For this to help the MSRV use case, the crate author must directly depend on all indirect dependencies where their latest release has too new of an MSRV but at least they can do so with the `^` operator, rather than `<` and breaking the ecosystem.
### How should we test and review this PR?
The first two commits add tests using `-Zminimal-versions`. The commit that adds `-Zdirect-minimal-versions` updates the tests, highlighting the differences in behavior.
### Additional information
Potential areas of conversation for stablization
- Flag name
- Handling of yanked (pick first non-yanked, pick yanked, error)
- Quality of error message
- Should the package have a "memory" of this flag being set by writing it to the lockfile?
Potential future work
- Stablize this
- Remove `-Zminimal-versions`
- Update `cargo publish`s `--verify` step to use this.
- The challenge is this won't be using the packaged `Cargo.lock` which probably should also be verified.
feat: Use test name for dir when running tests
Tests for Cargo currently put their generated data in `target/tmp/cit/t{}`. This can make it very hard to debug tests if multiple are falling. Having the tests be named would make this much easier when debugging.
[This comment](eff8d693b9/crates/cargo-test-support/src/paths.rs (L54-L64)) explains why cargo used a number instead of the test name for the unique identifier. I found a way to get the test name to be the unique identifier. I did this by having `cargo-test-macro` get the name of the test when the macro is looking for where the function body starts. It passes the name of the test and the file that the test comes from into the boilerplate it was already adding.
It uses the file and the test name as the directory to place the tests generated data data into, i.e, `target/tmp/cit/testsuite/workspaces/workspace_in_git`.
Note: I also found `t{}` to be frustrating when trying to get the size of a test. There is no good way to get the size of a test within `target/tmp/cit`, without running the tests one at a time and checking the size like so:
```python
for test in tests:
subprocess.run(["cargo", "test", "-p", "cargo", test, "--", "--exact"])
test_size = subprocess.run(["du", "-sk", "target/tmp/cit/t0"], stdout=subprocess.PIPE, text=True)
size_dict[test] = int(test_size.stdout.split()[0])
```
It made it very hard to try and find which tests were causing most of the size of the `target` dir.
Fix help string for "--charset" option of "cargo tree"
The help text for the `cargo tree` subcommand currently is:
```console
> cargo --version
cargo 1.67.1 (8ecd4f20a 2023-01-10)
> cargo tree --help
Display a tree visualization of a dependency graph
Usage: cargo tree [OPTIONS]
Options:
-q, --quiet Do not print cargo log messages
--manifest-path <PATH> Path to Cargo.toml
-p, --package [<SPEC>] Package to be used as the root of the tree
-v, --verbose... Use verbose output (-vv very verbose/build.rs output)
--workspace Display the tree for all packages in the workspace
--exclude <SPEC> Exclude specific workspace members
--color <WHEN> Coloring: auto, always, never
--frozen Require Cargo.lock and cache are up to date
-F, --features <FEATURES> Space or comma separated list of features to activate
--locked Require Cargo.lock is up to date
--all-features Activate all available features
--offline Run without accessing the network
--config <KEY=VALUE> Override a configuration value
--no-default-features Do not activate the `default` feature
--target <TRIPLE> Filter dependencies matching the given target-triple (default host platform). Pass `all` to include all targets.
-Z <FLAG> Unstable (nightly-only) flags to Cargo, see 'cargo -Z help' for details
-e, --edges <KINDS> The kinds of dependencies to display (features, normal, build, dev, all, no-normal, no-build, no-dev, no-proc-macro)
-i, --invert [<SPEC>] Invert the tree direction and focus on the given package
--prune <SPEC> Prune the given package from the display of the dependency tree
--depth <DEPTH> Maximum display depth of the dependency tree
--prefix <PREFIX> Change the prefix (indentation) of how each entry is displayed [default: indent] [possible values: depth, indent, none]
--no-dedupe Do not de-duplicate (repeats all shared dependencies)
-d, --duplicates Show only dependencies which come in multiple versions (implies -i)
--charset <CHARSET> Character set to use in output: utf8, ascii [default: utf8] [possible values: utf8, ascii]
-f, --format <FORMAT> Format string used for printing dependencies [default: {p}]
-h, --help Print help information
```
Note that for the `--charset` option, the information about available encodings is duplicated. This PR removes the duplication.
Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml
The URL associated with a `SourceId` for a sparse registry includes the `sparse+` prefix in the URL to differentiate from git (non-sparse) registries.
`SourceId::from_url` was not including the `sparse+` prefix of sparse registry URLs on construction, which caused roundtrips of `as_url` and `from_url` to fail by losing the prefix.
Fixes#11751 by:
* Including the prefix in the URL
* Adding a test that verifies round-trip behavior for sparse `SourceId`s
* Modifying `CanonicalUrl` so it no longer considers `sparse+` and non-`sparse+` URLs to be equivalent
Fix warning with tempfile
A recent release of the tempfile crate added a `#[must_use]` attribute to the `into_path` method. This triggers a warning when building.
This also adds a test to verify that the intermediate artifacts persist in the temp directory.
