cargo icon indicating copy to clipboard operation
cargo copied to clipboard

fix: create ephemeral workspace for git source

Open hi-rustin opened this issue 3 months ago • 5 comments

What does this PR try to resolve?

close https://github.com/rust-lang/cargo/issues/13259

Root cause

The root cause of this problem is that cargo install creates a non-ephemeral workspace for git sources, that would try finding all member packages in that git repo, and add them as path dependencies.

However, when computing fingerprints to determine if a rebuild is needed, Cargo doesn't hash git revision into fingerprint directly. Instead, it checks if source file path has changed. For git source, it is an absolute path to ~/.cargo/git/checkouts//, so commit SHA is always there. For path dependency, it is relative to workspace root, so nothing change and Cargo doesn't rebuild even it is from a different git revision.

Sloution

If any time we create an ephemeral workspace, then we will update the git source every time and that would help us use the right code and rebuild the binary correctly.

There are some historical PRs related to this change:

  1. 4 years ago, we tried to all use ephemeral workspace in https://github.com/rust-lang/cargo/pull/5350. But it caused https://github.com/rust-lang/cargo/issues/5662 so we revert that change in https://github.com/rust-lang/cargo/pull/5685.
  2. https://github.com/rust-lang/cargo/pull/7768 In this PR, we try to search the root manifest by using Workspace::new, but it also caused the https://github.com/rust-lang/cargo/issues/13259.

So in my fix, I just rollback to use ephemera workspace but using find_root to achieve the same goal of searching the root manifest. You can find more in the test: git_install_reads_workspace_manifest

How should we test and review this PR?

  • [x] manually test https://github.com/rust-lang/cargo/pull/13689#issuecomment-2053473575
  • [x] unit test

Additional information

None

hi-rustin avatar Apr 02 '24 13:04 hi-rustin

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 avatar Apr 02 '24 13:04 rustbot

Debugging note:

  1. The failed case was caused by my change.
  2. The reason is that we don't check the package content in Workspace::ephemera function, but in Workspace::new we will try to load the package again. This way, it can bail out the correct error from the manifest.
  3. Perhaps we should explicitly check the manifest in the ephemera function or find a way to abort the process before make_ws_rustc_target.

hi-rustin avatar Apr 07 '24 02:04 hi-rustin

Tested locally:

  1. Add a new config
[build] 
target-dir = "/Volumes/t7/code/cargo/target/tmp1" 
  1. Install the video_in_waveform with new cargo build:
./target/debug/cargo install --git https://github.com/hi-rustin/cargo-information --rev 5fd64bb6afe37092ee1dd7c518f4fc4865175c22
   Compiling bytesize v1.3.0
   Compiling cargo-information v0.4.2 (https://github.com/hi-rustin/cargo-information?rev=5fd64bb6afe37092ee1dd7c518f4fc4865175c22#5fd64bb6)
    Finished `release` profile [optimized] target(s) in 1m 26s
  Installing /Users/hi-rustin/.cargo/bin/cargo-info
   Installed package `cargo-information v0.4.2 (https://github.com/hi-rustin/cargo-information?rev=5fd64bb6afe37092ee1dd7c518f4fc4865175c22#5fd64bb6)` (executable `cargo-info`)
  1. Calculate the hash:
md5 ~/.cargo/bin/cargo-info
MD5 (/Users/hi-rustin/.cargo/bin/cargo-info) = 3a7ccae20527098730fda436790ddf72
  1. Install it again with a different version:
./target/debug/cargo install --git https://github.com/hi-rustin/cargo-information --rev e8b3419bc033d3252ad19081f516946c94e0c02e
    Updating git repository `https://github.com/hi-rustin/cargo-information`
    Updating crates.io index
   Compiling cargo-information v0.4.2 (https://github.com/hi-rustin/cargo-information?rev=e8b3419bc033d3252ad19081f516946c94e0c02e#e8b3419b)
    Finished `release` profile [optimized] target(s) in 7.85s
   Replacing /Users/hi-rustin/.cargo/bin/cargo-info
    Replaced package `cargo-information v0.4.2 (https://github.com/hi-rustin/cargo-information?rev=5fd64bb6afe37092ee1dd7c518f4fc4865175c22#5fd64bb6)` with `cargo-information v0.4.2 (https://github.com/hi-rustin/cargo-information?rev=e8b3419bc033d3252ad19081f516946c94e0c02e#e8b3419b)` (executable `cargo-info`)
  1. Calculate the hash:
md5 ~/.cargo/bin/cargo-info
MD5 (/Users/hi-rustin/.cargo/bin/cargo-info) = abd82eed7d6f3f88cf7a5371bf75d469

hi-rustin avatar Apr 13 '24 04:04 hi-rustin

@weihanglo Could you please take a look? Thanks! 💚 💙 💜 💛 ❤️

hi-rustin avatar Apr 13 '24 04:04 hi-rustin

:umbrella: The latest upstream changes (presumably #13769) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Apr 18 '24 19:04 bors