fix(command-vendor): strip_prefix panic in cp_sources method

[vksir]

What does this PR try to resolve?

When executing the cargo vendor command, there is a probability that mv pkg fails, causing strip_prefix to panic. This PR aims to fix this issue.

Fix: #15875

How to test and review this PR?

Modify the code to make the move pkg action inevitably fail. Then rebuild cargo and execute cargo vendor to test it.

[rustbot]

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

[weihanglo]

Do we need to comment out this to reproduce?

As documented, remove_dir_all will fail if the path is not a directory, so our fs::rename call should fail even without commenting this out I guess.

(And we also capture CARGO_LOG in the stderr assertion, so it should be fairly robust to ensure we went the code path)

[vksir]

use std::fs;

fn main() {
    fs::create_dir_all("./tmp").unwrap();
    fs::create_dir_all("./tmp/src").unwrap();
    fs::write("./tmp/src/src_file", "src_file").unwrap();

    // Will rename successfully
    fs::write("./tmp/dst", "blocking file").unwrap();
    fs::rename("./tmp/src", "./tmp/dst").unwrap();
}

I wrote a smallest test. fs::rename will make the src dir override the dst file. So I change the dst file to a dir in pre commit.

[weihanglo]

I wrote a smallest test. fs::rename will make the src dir override the dst file. So I change the dst file to a dir in pre commit.

Interesting 🤔. On Linux it failed: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=0b8b476b80e4bc4ac6839433e39fd840

[vksir]

Yes, I just thought of this as well and tested it. It failed in the Linux environment. (In the windows, it will rename successfully).
What ever, it seems that the unit test is running on the Linux docker. I would modify the code.

[weihanglo]

https://github.com/rust-lang/rust/blob/51f5892019f8fb07864647d46c4eb577d3b0719f/library/std/src/sys/fs/windows.rs?plain=1#L1272

Found it. The Windows implementation of fs::rename uses MOVEFILE_REPLACE_EXISTING. Probably that was the reason it succeeded?
Anyway, if we cannot make it passed on Windows, we can put this

#[cargo_test(ignore_windows = "annoying to create a repro on Windows; see rust-lang/cargo#16214")]

[weihanglo]

---- expected: tests/testsuite/vendor.rs:2117:27
++++ actual:   stderr
   1    1 | ...
   2    2 | [..]failed to `mv "[..]vendor[..].vendor-staging[..]log-0.3.5" "[..]vendor[..]log"`: [..]
   3    3 | ...
   4    4 | Caused by:
   5    5 |   failed to copy vendored sources for log v0.3.5
   6    6 | 
   7    7 | Caused by:
   8      -   failed to create directory `[..]log`
        8 +   failed to create directory `[ROOT]/foo/vendor/log/src`
   9    9 | 
  10   10 | Caused by:
  11      -   File exists[..]
       11 +   Not a directory (os error 20)
  12   12 | ...∅

Ouch! Then we have no way out for this.

maybe we should have a private environment variable unconditionally go through that code path? Like __CARGO_TEST_VENDOR_FALLBACK_CP_SOURCES"? So that we can actually verify the cp_sources is working correct. And with that we don't need to ignore Windows anymore.

View changes since this review

[vksir]

Okay! If adding environment variables is allowed, I'm glad to do so.

[weihanglo]

@vksir yeah go head. Just remind that we disallow std::env::var use throughout Cargo, so you may either put an allowed like this

cargo/src/cargo/core/source_id.rs

Lines 925 to 926 in df07b39

	// ALLOWED: For testing Cargo itself only.
	#[allow(clippy::disallowed_methods)]

or access env from gctx

cargo/src/cargo/ops/cargo_new.rs

Line 474 in df07b39

if gctx.get_env_os("__CARGO_TEST_INTERNAL_ERROR").is_some() {

[weihanglo]

Nice! Thanks for working with us!

View changes since this review

[vksir]

@weihanglo Also thank you once again for your patient guidance! :)