fix(vendor)!: direct extraction for registry sources

[weihanglo]

What does this PR try to resolve?

PathSource::list_files has some heurstic rules for listing files. Those rules are mainly designed for cargo package.

Previously, cargo-vendor relies on those rules to understand what files to vendor. However, it shouldn't use those rules because:

• Package extracted from a .crate tarball isn't Git-controlled, some rules may apply differently.
• The extracted package already went through PathSource::list_files during packaging. It should be clean enough.
• Should keep crate sources from registry sources in a pristine state, which is exactly what vendoring is meant for.

Instead, we switch to direct extraction into the vendor directory
to ensure source code is the same as in the .crate tarball.

How should we test and review this PR?

There is already a test vendor::package_exclude for the fix of #9054,
though now I think it is not a good fix. The test change shows the correct behavior change.

I am not sure if we want more tests.

Also, there are some caveats with this fix:

• The overwrite protection in unpack_package assumes the unpack
  directory is always <pkg>-<version>.
  We don't want to remove this,
  but for cargo-vendor supports vendoring without version suffix.
  For that case, we need a temporary staging area,
  and move the unpacked source then.
• The heurstic in PathSource::list_files did something "good" in
  general cases, like excluding hidden directories. That means
  common directories like .github or .config won't be vendored.
  After this, those get included. This is another round of churns.
  We might want to get other cargo-vendor changes along with this
  in one single release.

Additional information

• Fixes cargo vendor vs cargo package inconsistency #9054
• Fixes Inconsistency with publishing/vendoring listing of files #9555
• Fixes cargo vendor does not include license-file if not in package list #9575
• Fixes cargo vendor doesn't generate .cargo_vcs_info.json #11000
• Fixes cargo vendor misses some files #14034
• Fixes cargo vendor loses some nonempty directories #15080
• Fixes cargo vendor should vendor crates in a pristine state #15090

This also opens a door for

• cargo vendor can't handle duplicates. #10310
• Normalization of Cargo.toml can break crates with deps. that read Cargo.toml #13191

Since we are changing vendored source (again), we might want to remove the .rej/.orig special rules in cargo-vendor, as well as look into the new source-dedup vendor dir layout.

Summary Notes

• benchmark-result by weihanglo

Generated by triagebot, see help for how to add more

[rustbot]

r? @ehuss

rustbot has assigned @ehuss.
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

[epage]

Except this doesn't guarantee things are pristine until we vendor the contents of the .crate. I worry this might cause us to vendor things like target/ directories.

[weihanglo]

Except this doesn't guarantee things are pristine until we vendor the contents of the .crate. I worry this might cause us to vendor things like target/ directories.

The chance of this happening is low because cargo-vendor holds a MutateExclusive cache lock during the entire call. Also basically everything is the same except using walkdir instead of PathSource::list_files. We can add a filter to walkdir to exclude target/ directory, if that helps.

This patch aims to be as simple as possible. The “direct extraction” is the correct fix but require more architectural changes.

[epage]

The chance of this happening is low because cargo-vendor holds a MutateExclusive cache lock during the entire call. Also basically everything is the same except using walkdir instead of PathSource::list_files. We can add a filter to walkdir to exclude target/ directory, if that helps.

And that we delete the extracted .crate so we get a fresh extraction.

So looking over this again, this does not fix #15090. This PR only changes which files we list, it doesn't change how we copy them which is what is causing the problem in #15090.

The “direct extraction” is the correct fix but require more architectural changes.

Is it all that big of an architectural change? Can we get an instance of the registry source and and call an extract method directly on it?

[weihanglo]

So looking over this again, this does not fix #15090. This PR only changes which files we list, it doesn't change how we copy them which is what is causing the problem in #15090.

How does it not fix #15090? cargo-vendor doesn't change any content of a registry package, at least cp_sources() doesn't. It does skip some files and create a .cargo-checksum.json though.

Is it all that big of an architectural change? Can we get an instance of the registry source and and call an extract method directly on it?

Maybe not really an architectural change, but we'll need to have an abstraction over local/remote/http registry (which is RegistryData/RegistrySource). Ensure .crates be prepared. And decouple unpack_package and get_pkg from the actual download logic. And then we still need to walk the extraced content to compute checksum. Not architectural but also not small.

[weihanglo]

Anyway, I got direct extraction working. Doesn't look too terrible than I thought!

[weihanglo]

@rustbot note benchmark-result

Also, the benchmark result shows that it is ~60% faster when vendoring cargo and its dependencies. On Apple M1 Pro with 500GiB APFS SSD

Benchmark 1: cargo-15514 vendor
  Time (mean ± σ):     12.331 s ±  0.221 s    [User: 2.705 s, System: 7.211 s]
  Range (min … max):   12.083 s … 12.753 s    10 runs

Benchmark 2: cargo-15514 vendor --versioned-dirs
  Time (mean ± σ):     12.737 s ±  0.242 s    [User: 2.704 s, System: 7.681 s]
  Range (min … max):   12.481 s … 13.240 s    10 runs

Benchmark 3: cargor-master vendor
  Time (mean ± σ):     19.639 s ±  0.475 s    [User: 2.947 s, System: 12.391 s]
  Range (min … max):   19.006 s … 20.481 s    10 runs

Benchmark 4: cargor-master vendor --versioned-dirs
  Time (mean ± σ):     19.574 s ±  0.558 s    [User: 2.948 s, System: 12.422 s]
  Range (min … max):   18.739 s … 20.489 s    10 runs

Summary
  cargofast vendor ran
    1.03 ± 0.03 times faster than cargo-15514 vendor --versioned-dirs
    1.59 ± 0.05 times faster than cargo-master vendor --versioned-dirs
    1.59 ± 0.05 times faster than cargo-master vendor

[ehuss]

Nice!

I have not gone over this with a fine-toothed comb, but the overall structure looks good to me.

I'm not going to click merge right now in case anyone else wants to look closer, or you had any additional concerns or changes you were thinking of. If there isn't more progress by sometime next week, then I think it would be fine to just merge.