From 7e5ab6678476af44af7c237955cca83c95cc7eff Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 20 Feb 2024 12:32:23 -0800 Subject: [PATCH 1/6] Fix bug in GlobalCacheTracker::git_checkout_all This fixes a bug in the `GlobalCacheTracker::git_checkout_all` function which was using the wrong SQL column name. This function belongs to the set of of functions only used for testing, and this particular function was not yet used. --- src/cargo/core/global_cache_tracker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/core/global_cache_tracker.rs b/src/cargo/core/global_cache_tracker.rs index 814d4a5eff5..e9c4856a742 100644 --- a/src/cargo/core/global_cache_tracker.rs +++ b/src/cargo/core/global_cache_tracker.rs @@ -499,7 +499,7 @@ impl GlobalCacheTracker { let mut stmt = self.conn.prepare_cached( "SELECT git_db.name, git_checkout.name, git_checkout.size, git_checkout.timestamp FROM git_db, git_checkout - WHERE git_checkout.registry_id = git_db.id", + WHERE git_checkout.git_id = git_db.id", )?; let rows = stmt .query_map([], |row| { From f9ba89b0cd38f059904a2bb15964503acdc05e63 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 20 Feb 2024 12:33:32 -0800 Subject: [PATCH 2/6] Add Execs::args This adds the `Execs::args` method which forwards to `ProcessBuilder::args` to specify multiple command arguments at once. --- crates/cargo-test-support/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index e334041a044..a2a2e676bcc 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -764,6 +764,13 @@ impl Execs { self } + pub fn args>(&mut self, args: &[T]) -> &mut Self { + if let Some(ref mut p) = self.process_builder { + p.args(args); + } + self + } + pub fn cwd>(&mut self, path: T) -> &mut Self { if let Some(ref mut p) = self.process_builder { if let Some(cwd) = p.get_cwd() { From 8bb62310e511c4a3fa66c986063c3f90b443f740 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 20 Feb 2024 12:36:03 -0800 Subject: [PATCH 3/6] cargo_test CI macro: add requires_rustup_stable This adds the `requires_rustup_stable` option to the cargo_test macro to require `rustup` to exist, and for the `stable` toolchain to be installed. It is required that stable be installed in Cargo's CI. On a developer machine, the tests will be ignored if rustup is not installed. It will also be ignored on rust-lang/rust's CI since it does not have rustup. --- crates/cargo-test-macro/src/lib.rs | 34 ++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/crates/cargo-test-macro/src/lib.rs b/crates/cargo-test-macro/src/lib.rs index a94a239aa31..39f37f4e5ca 100644 --- a/crates/cargo-test-macro/src/lib.rs +++ b/crates/cargo-test-macro/src/lib.rs @@ -74,6 +74,12 @@ pub fn cargo_test(attr: TokenStream, item: TokenStream) -> TokenStream { requires_reason = true; set_ignore!(is_not_nightly, "requires nightly"); } + "requires_rustup_stable" => { + set_ignore!( + !has_rustup_stable(), + "rustup or stable toolchain not installed" + ); + } s if s.starts_with("requires_") => { let command = &s[9..]; set_ignore!(!has_command(command), "{command} not installed"); @@ -204,29 +210,27 @@ fn version() -> (u32, bool) { unsafe { VERSION } } -fn has_command(command: &str) -> bool { - let output = match Command::new(command).arg("--version").output() { +fn check_command(command_name: &str, args: &[&str]) -> bool { + let mut command = Command::new(command_name); + command.args(args); + let output = match command.output() { Ok(output) => output, Err(e) => { // * hg is not installed on GitHub macOS or certain constrained // environments like Docker. Consider installing it if Cargo // gains more hg support, but otherwise it isn't critical. // * lldb is not pre-installed on Ubuntu and Windows, so skip. - if is_ci() && !["hg", "lldb"].contains(&command) { - panic!( - "expected command `{}` to be somewhere in PATH: {}", - command, e - ); + if is_ci() && !matches!(command_name, "hg" | "lldb") { + panic!("expected command `{command_name}` to be somewhere in PATH: {e}",); } return false; } }; if !output.status.success() { panic!( - "expected command `{}` to be runnable, got error {}:\n\ + "expected command `{command_name}` to be runnable, got error {}:\n\ stderr:{}\n\ stdout:{}\n", - command, output.status, String::from_utf8_lossy(&output.stderr), String::from_utf8_lossy(&output.stdout) @@ -235,6 +239,18 @@ fn has_command(command: &str) -> bool { true } +fn has_command(command: &str) -> bool { + check_command(command, &["--version"]) +} + +fn has_rustup_stable() -> bool { + if option_env!("CARGO_TEST_DISABLE_NIGHTLY").is_some() { + // This cannot run on rust-lang/rust CI due to the lack of rustup. + return false; + } + check_command("cargo", &["+stable", "--version"]) +} + /// Whether or not this running in a Continuous Integration environment. fn is_ci() -> bool { // Consider using `tracked_env` instead of option_env! when it is stabilized. From 9f71231391f76894294fabdf20e91edeb480ae6c Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 20 Feb 2024 12:43:07 -0800 Subject: [PATCH 4/6] Add global_cache_tracker stability tests. This adds some tests to ensure that the database used in the global cache tracker stays compatible across versions. These tests work by using rustup to run both the current cargo and the stable cargo, and verifying that when switching versions, everything works as expected. --- .github/workflows/main.yml | 4 + tests/testsuite/global_cache_tracker.rs | 149 ++++++++++++++++++++++++ 2 files changed, 153 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index cd3460d0ba7..f8381b39359 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -156,6 +156,10 @@ jobs: - uses: actions/checkout@v4 - name: Dump Environment run: ci/dump-environment.sh + # Some tests require stable. Make sure it is set to the most recent stable + # so that we can predictably handle updates if necessary (and not randomly + # when GitHub updates its image). + - run: rustup update --no-self-update stable - run: rustup update --no-self-update ${{ matrix.rust }} && rustup default ${{ matrix.rust }} - run: rustup target add ${{ matrix.other }} - run: rustup component add rustc-dev llvm-tools-preview rust-docs diff --git a/tests/testsuite/global_cache_tracker.rs b/tests/testsuite/global_cache_tracker.rs index 9365e1e6856..f55e01fb6d8 100644 --- a/tests/testsuite/global_cache_tracker.rs +++ b/tests/testsuite/global_cache_tracker.rs @@ -1863,3 +1863,152 @@ fn clean_gc_quiet_is_quiet() { ) .run(); } + +#[cargo_test(requires_rustup_stable)] +fn compatible_with_older_cargo() { + // Ensures that db stays backwards compatible across versions. + + // T-4 months: Current version, build the database. + Package::new("old", "1.0.0").publish(); + Package::new("middle", "1.0.0").publish(); + Package::new("new", "1.0.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + old = "1.0" + middle = "1.0" + new = "1.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + // Populate the last-use data. + p.cargo("check -Zgc") + .masquerade_as_nightly_cargo(&["gc"]) + .env("__CARGO_TEST_LAST_USE_NOW", months_ago_unix(4)) + .run(); + assert_eq!( + get_registry_names("src"), + ["middle-1.0.0", "new-1.0.0", "old-1.0.0"] + ); + assert_eq!( + get_registry_names("cache"), + ["middle-1.0.0.crate", "new-1.0.0.crate", "old-1.0.0.crate"] + ); + + // T-2 months: Stable version, make sure it reads and deletes old src. + p.change_file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + new = "1.0" + middle = "1.0" + "#, + ); + p.process("cargo") + .args(&["+stable", "check", "-Zgc"]) + .masquerade_as_nightly_cargo(&["gc"]) + .env("__CARGO_TEST_LAST_USE_NOW", months_ago_unix(2)) + // Necessary since `process` removes rustup. + .env("PATH", std::env::var_os("PATH").unwrap()) + .run(); + assert_eq!(get_registry_names("src"), ["middle-1.0.0", "new-1.0.0"]); + assert_eq!( + get_registry_names("cache"), + ["middle-1.0.0.crate", "new-1.0.0.crate", "old-1.0.0.crate"] + ); + + // T-0 months: Current version, make sure it can read data from stable, + // deletes old crate and middle src. + p.change_file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + new = "1.0" + "#, + ); + p.cargo("check -Zgc") + .masquerade_as_nightly_cargo(&["gc"]) + .run(); + assert_eq!(get_registry_names("src"), ["new-1.0.0"]); + assert_eq!( + get_registry_names("cache"), + ["middle-1.0.0.crate", "new-1.0.0.crate"] + ); +} + +#[cargo_test(requires_rustup_stable)] +fn forward_compatible() { + // Checks that db created in an older version can be read in a newer version. + Package::new("bar", "1.0.0").publish(); + let git_project = git::new("from_git", |p| { + p.file("Cargo.toml", &basic_manifest("from_git", "1.0.0")) + .file("src/lib.rs", "") + }); + + let p = project() + .file( + "Cargo.toml", + &format!( + r#" + [package] + name = "foo" + + [dependencies] + bar = "1.0.0" + from_git = {{ git = '{}' }} + "#, + git_project.url() + ), + ) + .file("src/lib.rs", "") + .build(); + + p.process("cargo") + .args(&["+stable", "check", "-Zgc"]) + .masquerade_as_nightly_cargo(&["gc"]) + // Necessary since `process` removes rustup. + .env("PATH", std::env::var_os("PATH").unwrap()) + .run(); + + let config = GlobalContextBuilder::new().unstable_flag("gc").build(); + let lock = config + .acquire_package_cache_lock(CacheLockMode::MutateExclusive) + .unwrap(); + let tracker = GlobalCacheTracker::new(&config).unwrap(); + // Don't want to check the actual index name here, since although the + // names are semi-stable, they might change over long periods of time. + let indexes = tracker.registry_index_all().unwrap(); + assert_eq!(indexes.len(), 1); + let crates = tracker.registry_crate_all().unwrap(); + let names: Vec<_> = crates + .iter() + .map(|(krate, _timestamp)| krate.crate_filename) + .collect(); + assert_eq!(names, &["bar-1.0.0.crate"]); + let srcs = tracker.registry_src_all().unwrap(); + let names: Vec<_> = srcs + .iter() + .map(|(src, _timestamp)| src.package_dir) + .collect(); + assert_eq!(names, &["bar-1.0.0"]); + let dbs: Vec<_> = tracker.git_db_all().unwrap(); + assert_eq!(dbs.len(), 1); + let cos: Vec<_> = tracker.git_checkout_all().unwrap(); + assert_eq!(cos.len(), 1); + drop(lock); +} From ccaa118d27232d1eb3176da3e207146b394c92cf Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 20 Feb 2024 13:24:50 -0800 Subject: [PATCH 5/6] Add workaround for RUSTUP_WINDOWS_PATH_ADD_BIN. On Windows, rustup has an issue with recursive cargo invocations. This commit can be removed once https://github.com/rust-lang/rustup/issues/3036 is resolved. --- .github/workflows/main.yml | 3 ++- crates/cargo-test-macro/src/lib.rs | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index f8381b39359..249faea5c71 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -116,6 +116,8 @@ jobs: CARGO_PROFILE_TEST_DEBUG: 1 CARGO_INCREMENTAL: 0 CARGO_PUBLIC_NETWORK_TESTS: 1 + # Workaround for https://github.com/rust-lang/rustup/issues/3036 + RUSTUP_WINDOWS_PATH_ADD_BIN: 0 strategy: matrix: include: @@ -170,7 +172,6 @@ jobs: - name: Configure extra test environment run: echo CARGO_CONTAINER_TESTS=1 >> $GITHUB_ENV if: matrix.os == 'ubuntu-latest' - - run: cargo test -p cargo - name: Clear intermediate test output run: ci/clean-test-output.sh diff --git a/crates/cargo-test-macro/src/lib.rs b/crates/cargo-test-macro/src/lib.rs index 39f37f4e5ca..ef59733fcd3 100644 --- a/crates/cargo-test-macro/src/lib.rs +++ b/crates/cargo-test-macro/src/lib.rs @@ -248,6 +248,13 @@ fn has_rustup_stable() -> bool { // This cannot run on rust-lang/rust CI due to the lack of rustup. return false; } + if cfg!(windows) && !is_ci() && option_env!("RUSTUP_WINDOWS_PATH_ADD_BIN").is_none() { + // There is an issue with rustup that doesn't allow recursive cargo + // invocations. Disable this on developer machines if the environment + // variable is not enabled. This can be removed once + // https://github.com/rust-lang/rustup/issues/3036 is resolved. + return false; + } check_command("cargo", &["+stable", "--version"]) } From a82794ec4a205b58ad7cfd4382f50db6690ac218 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 20 Feb 2024 16:27:04 -0800 Subject: [PATCH 6/6] Fix path issues for running rustup wrapper on Windows. Cargo likes to modify PATH, which circumvents the ability to choose the correct "cargo" executable to run on Windows (because Windows uses PATH for both binary and shared library searching). --- crates/cargo-test-macro/src/lib.rs | 21 ++++++++++++++++----- tests/testsuite/global_cache_tracker.rs | 23 +++++++++++++++-------- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/crates/cargo-test-macro/src/lib.rs b/crates/cargo-test-macro/src/lib.rs index ef59733fcd3..f813b835a45 100644 --- a/crates/cargo-test-macro/src/lib.rs +++ b/crates/cargo-test-macro/src/lib.rs @@ -1,4 +1,5 @@ use proc_macro::*; +use std::path::Path; use std::process::Command; use std::sync::Once; @@ -210,8 +211,9 @@ fn version() -> (u32, bool) { unsafe { VERSION } } -fn check_command(command_name: &str, args: &[&str]) -> bool { - let mut command = Command::new(command_name); +fn check_command(command_path: &Path, args: &[&str]) -> bool { + let mut command = Command::new(command_path); + let command_name = command.get_program().to_str().unwrap().to_owned(); command.args(args); let output = match command.output() { Ok(output) => output, @@ -220,7 +222,7 @@ fn check_command(command_name: &str, args: &[&str]) -> bool { // environments like Docker. Consider installing it if Cargo // gains more hg support, but otherwise it isn't critical. // * lldb is not pre-installed on Ubuntu and Windows, so skip. - if is_ci() && !matches!(command_name, "hg" | "lldb") { + if is_ci() && !matches!(command_name.as_str(), "hg" | "lldb") { panic!("expected command `{command_name}` to be somewhere in PATH: {e}",); } return false; @@ -240,7 +242,7 @@ fn check_command(command_name: &str, args: &[&str]) -> bool { } fn has_command(command: &str) -> bool { - check_command(command, &["--version"]) + check_command(Path::new(command), &["--version"]) } fn has_rustup_stable() -> bool { @@ -255,7 +257,16 @@ fn has_rustup_stable() -> bool { // https://github.com/rust-lang/rustup/issues/3036 is resolved. return false; } - check_command("cargo", &["+stable", "--version"]) + // Cargo mucks with PATH on Windows, adding sysroot host libdir, which is + // "bin", which circumvents the rustup wrapper. Use the path directly from + // CARGO_HOME. + let home = match option_env!("CARGO_HOME") { + Some(home) => home, + None if is_ci() => panic!("expected to run under rustup"), + None => return false, + }; + let cargo = Path::new(home).join("bin/cargo"); + check_command(&cargo, &["+stable", "--version"]) } /// Whether or not this running in a Continuous Integration environment. diff --git a/tests/testsuite/global_cache_tracker.rs b/tests/testsuite/global_cache_tracker.rs index f55e01fb6d8..436013e4e3c 100644 --- a/tests/testsuite/global_cache_tracker.rs +++ b/tests/testsuite/global_cache_tracker.rs @@ -14,11 +14,12 @@ use cargo::GlobalContext; use cargo_test_support::paths::{self, CargoPathExt}; use cargo_test_support::registry::{Package, RegistryBuilder}; use cargo_test_support::{ - basic_manifest, cargo_process, execs, git, project, retry, sleep_ms, thread_wait_timeout, - Project, + basic_manifest, cargo_process, execs, git, process, project, retry, sleep_ms, + thread_wait_timeout, Execs, Project, }; use itertools::Itertools; use std::fmt::Write; +use std::path::Path; use std::path::PathBuf; use std::process::Stdio; use std::time::{Duration, SystemTime}; @@ -153,6 +154,14 @@ fn populate_cache( (cache_dir, src_dir) } +fn rustup_cargo() -> Execs { + // Get the path to the rustup cargo wrapper. This is necessary because + // cargo adds the "deps" directory into PATH on Windows, which points to + // the wrong cargo. + let rustup_cargo = Path::new(&std::env::var_os("CARGO_HOME").unwrap()).join("bin/cargo"); + execs().with_process_builder(process(rustup_cargo)) +} + #[cargo_test] fn auto_gc_gated() { // Requires -Zgc to both track last-use data and to run auto-gc. @@ -1915,12 +1924,11 @@ fn compatible_with_older_cargo() { middle = "1.0" "#, ); - p.process("cargo") + rustup_cargo() .args(&["+stable", "check", "-Zgc"]) + .cwd(p.root()) .masquerade_as_nightly_cargo(&["gc"]) .env("__CARGO_TEST_LAST_USE_NOW", months_ago_unix(2)) - // Necessary since `process` removes rustup. - .env("PATH", std::env::var_os("PATH").unwrap()) .run(); assert_eq!(get_registry_names("src"), ["middle-1.0.0", "new-1.0.0"]); assert_eq!( @@ -1978,11 +1986,10 @@ fn forward_compatible() { .file("src/lib.rs", "") .build(); - p.process("cargo") + rustup_cargo() .args(&["+stable", "check", "-Zgc"]) + .cwd(p.root()) .masquerade_as_nightly_cargo(&["gc"]) - // Necessary since `process` removes rustup. - .env("PATH", std::env::var_os("PATH").unwrap()) .run(); let config = GlobalContextBuilder::new().unstable_flag("gc").build();