diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a099edc44..b876f7910 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1700,8 +1700,7 @@ jobs: # the matching matrix key, which the suites treat as unset. SOCKET_PATCH_NPM_E2E_REQUIRED: ${{ matrix.npm_required || '' }} # `pnpm_required` does the same for the pnpm-family suites - # (corepack pnpm / pnpm on PATH). Set on the Linux rows only; the - # Windows and macOS rows have not been checked against it yet. + # (corepack pnpm / pnpm on PATH), on every OS that runs them. SOCKET_PATCH_PNPM_E2E_REQUIRED: ${{ matrix.pnpm_required || '' }} SOCKET_PATCH_CARGO_E2E_REQUIRED: ${{ matrix.suite == 'e2e_safety_cargo_build' && '1' || '' }} SOCKET_PATCH_UV_E2E_REQUIRED: ${{ matrix.uv != '' && '1' || '' }} @@ -1942,11 +1941,13 @@ jobs: include: - os: windows-latest suite: e2e_safety_pnpm + pnpm_required: '1' - os: windows-latest suite: e2e_redirect_npm_build npm_required: '1' - os: windows-latest suite: e2e_redirect_rush_sim + pnpm_required: '1' - os: windows-latest suite: e2e_redirect_bun_build e2e_vendor_bun_build mode_migration_bun bun: '1.4.2' @@ -1959,7 +1960,7 @@ jobs: - {os: windows-latest, suite: e2e_safety_vlt, vlt: '1.2.0', test_filter: --include-ignored vlt_pinned_matrix, vlt_store_linker: 'hardlink'} - {os: windows-latest, suite: e2e_vlt, vlt: '1.2.0', test_filter: --include-ignored vlt_pinned_matrix} - {os: windows-latest, suite: e2e_vlt, vlt: '1.0.0-rc.14', test_filter: --include-ignored vlt_pinned_matrix} - - {os: windows-latest, suite: e2e_redirect_pnpm_build, test_filter: '--ignored --skip pnpm_pinned_matrix'} + - {os: windows-latest, suite: e2e_redirect_pnpm_build, test_filter: '--ignored --skip pnpm_pinned_matrix', pnpm_required: '1'} - {os: windows-latest, suite: e2e_vendor_jvm_build, jvm_tool: maven, maven: '3.9.16', test_filter: '--ignored maven_reactor'} - {os: windows-latest, suite: e2e_vendor_jvm_build, jvm_tool: gradle, gradle: '8.14.3', java: '17', test_filter: '--ignored gradle_multi_project'} runs-on: ${{ vars.DISABLE_DEPOT_RUNNERS != 'true' && (matrix.os == 'ubuntu-latest' && 'depot-ubuntu-24.04-4' || matrix.os == 'ubuntu-22.04' && 'depot-ubuntu-22.04-4') || matrix.os }} @@ -1979,11 +1980,13 @@ jobs: include: - os: macos-latest suite: e2e_safety_pnpm + pnpm_required: '1' - os: macos-latest suite: e2e_redirect_npm_build npm_required: '1' - os: macos-latest suite: e2e_redirect_rush_sim + pnpm_required: '1' - os: macos-latest suite: e2e_redirect_bun_build bun: '1.4.2' @@ -2001,7 +2004,7 @@ jobs: - {os: macos-latest, suite: mode_migration_vlt, vlt: '1.2.0', test_filter: --include-ignored vlt_pinned_matrix} - {os: macos-latest, suite: e2e_safety_vlt, vlt: '1.2.0', test_filter: --include-ignored vlt_pinned_matrix, vlt_store_linker: 'hardlink'} - {os: macos-latest, suite: e2e_vlt, vlt: '1.2.0', test_filter: --include-ignored vlt_pinned_matrix} - - {os: macos-latest, suite: e2e_redirect_pnpm_build, test_filter: '--ignored --skip pnpm_pinned_matrix'} + - {os: macos-latest, suite: e2e_redirect_pnpm_build, test_filter: '--ignored --skip pnpm_pinned_matrix', pnpm_required: '1'} - {os: macos-latest, suite: e2e_redirect_uv_build, uv: '0.12.17'} - {os: macos-latest, suite: e2e_vendor_pypi_build, uv: '0.12.17', test_filter: --include-ignored} - {os: macos-latest, suite: e2e_vex_build, test_filter: 'poetry:: --ignored', poetry: '2.4.3'} diff --git a/crates/socket-patch-cli/tests/common/hermetic.rs b/crates/socket-patch-cli/tests/common/hermetic.rs index 8c7859040..7cab6e7c1 100644 --- a/crates/socket-patch-cli/tests/common/hermetic.rs +++ b/crates/socket-patch-cli/tests/common/hermetic.rs @@ -88,6 +88,19 @@ pub fn binary_command() -> Command { command(Path::new(env!("CARGO_BIN_EXE_socket-patch"))) } +/// A `Command` for the package-manager tool `name` on `PATH`, resolved with +/// the CLI's `resolve_tool` (every `PATHEXT` extension on Windows). npm and Node install `pnpm`, `corepack` and `npm` as `.cmd` +/// shims there, and a bare `Command::new("corepack")` only finds +/// `corepack.exe`, so the pnpm suites' probes reported the tool missing and +/// every Windows test skipped. A tool that isn't on `PATH` keeps the bare +/// name, so spawning it still fails the way the probes expect. +pub fn tool_command(name: &str) -> Command { + Command::new( + socket_patch_core::utils::process::resolve_tool(name) + .map_or_else(|| name.into(), std::path::PathBuf::into_os_string), + ) +} + /// Remove every ambient `SOCKET_*` var from `cmd`, except the telemetry /// opt-outs (an opted-out developer stays opted out), `SOCKET_NO_CONFIG` /// and `SOCKET_NO_UPDATE_CHECK`. [`command`] runs this; package-manager diff --git a/crates/socket-patch-cli/tests/common/mod.rs b/crates/socket-patch-cli/tests/common/mod.rs index 3eeb95327..05fabad6c 100644 --- a/crates/socket-patch-cli/tests/common/mod.rs +++ b/crates/socket-patch-cli/tests/common/mod.rs @@ -279,7 +279,7 @@ pub fn vlt_run(cwd: &Path, vlt_js: &Path, args: &[&str], extra_env: &[(&str, &st } fn run_toolchain(cwd: &Path, exe: &str, args: &[&str], extra_env: &[(&str, &str)]) { - let mut cmd = Command::new(exe); + let mut cmd = hermetic::tool_command(exe); cmd.args(args).current_dir(cwd); // Sandbox the caches first so `extra_env` can still override any // individual one (`Command` env ops are last-write-wins per name). diff --git a/crates/socket-patch-cli/tests/e2e_redirect_pnpm_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_pnpm_build.rs index c6cd5f28a..4bbba48f1 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_pnpm_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_pnpm_build.rs @@ -124,7 +124,7 @@ fn pnpm_command(pm: &str) -> Command { if let Some(bin) = std::env::var_os("SOCKET_PATCH_PNPM_E2E_BIN") { Command::new(bin) } else { - let mut cmd = Command::new("corepack"); + let mut cmd = hermetic::tool_command("corepack"); cmd.arg(pm); cmd } diff --git a/crates/socket-patch-cli/tests/e2e_redirect_rush_sim.rs b/crates/socket-patch-cli/tests/e2e_redirect_rush_sim.rs index cf614c0f3..bd812e2f4 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_rush_sim.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_rush_sim.rs @@ -29,7 +29,7 @@ mod common; use common::binary; use std::path::Path; -use std::process::{Command, Output, Stdio}; +use std::process::{Output, Stdio}; use sha2::{Digest, Sha512}; use wiremock::matchers::{method, path, path_regex}; @@ -82,7 +82,7 @@ fn has_corepack_pm(pm: &str) -> bool { }; // Isolated too: this probe is what actually downloads the package manager // the first time, and corepack stores it under `COREPACK_HOME`. - let mut cmd = Command::new("corepack"); + let mut cmd = hermetic::tool_command("corepack"); cmd.args([pm, "--version"]) .current_dir(probe.path()) .env("COREPACK_ENABLE_DOWNLOAD_PROMPT", "0"); @@ -95,7 +95,7 @@ fn has_corepack_pm(pm: &str) -> bool { } fn has_command(cmd: &str) -> bool { - let mut probe = Command::new(cmd); + let mut probe = hermetic::tool_command(cmd); probe.arg("--version"); cache_env::isolate(&mut probe); probe @@ -107,7 +107,7 @@ fn has_command(cmd: &str) -> bool { } fn corepack(cwd: &Path, pm: &str, args: &[&str], extra_env: &[(&str, &str)]) -> Output { - let mut cmd = Command::new("corepack"); + let mut cmd = hermetic::tool_command("corepack"); cmd.arg(pm).args(args).current_dir(cwd); hermetic::scrub_socket_vars(&mut cmd); hermetic::scrub_extra(&mut cmd, &[hermetic::Extra::Venv]); @@ -652,7 +652,7 @@ async fn rush_hosted_real_rush_update_install() { let rush = |args: &[&str]| -> Output { let mut full = vec!["x", "-y", rush_pkg.as_str()]; full.extend_from_slice(args); - let mut cmd = Command::new("npm"); + let mut cmd = hermetic::tool_command("npm"); cmd.args(&full).current_dir(root); hermetic::scrub_socket_vars(&mut cmd); hermetic::scrub_extra(&mut cmd, &[hermetic::Extra::Venv]); diff --git a/crates/socket-patch-cli/tests/e2e_safety_pnpm.rs b/crates/socket-patch-cli/tests/e2e_safety_pnpm.rs index 3884bf944..2a6e48941 100644 --- a/crates/socket-patch-cli/tests/e2e_safety_pnpm.rs +++ b/crates/socket-patch-cli/tests/e2e_safety_pnpm.rs @@ -28,7 +28,7 @@ use std::path::{Path, PathBuf}; mod common; use common::{ - assert_run_ok, git_sha256, git_sha256_file, has_command, pnpm_run, write_blob, + assert_run_ok, git_sha256, git_sha256_file, hermetic, pnpm_run, write_blob, write_minimal_manifest, write_package_json, PatchEntry, }; @@ -45,6 +45,22 @@ fn pnpm_required() -> bool { std::env::var_os("SOCKET_PATCH_PNPM_E2E_REQUIRED").is_some_and(|v| !v.is_empty()) } +/// `pnpm --version` succeeds. Probed through [`hermetic::tool_command`]: on +/// Windows pnpm is the `pnpm.cmd` shim `npm install -g` writes, which +/// `common::has_command`'s bare `Command::new` never finds. The exit status +/// counts too, since a resolved `.cmd` spawns through `cmd.exe` even when +/// the shim itself is broken. +fn has_pnpm() -> bool { + let mut probe = hermetic::tool_command("pnpm"); + probe.arg("--version"); + common::cache_env::isolate(&mut probe); + probe + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .is_ok_and(|s| s.success()) +} + /// Print the SKIP line for a missing pnpm, or panic under [`pnpm_required`]. fn skip_missing_pnpm() { assert!( @@ -249,7 +265,7 @@ fn file_identity(path: &Path) -> (u64, u64) { #[test] #[ignore] fn pnpm_install_produces_symlinked_layout() { - if !has_command("pnpm") { + if !has_pnpm() { skip_missing_pnpm(); return; } @@ -313,7 +329,7 @@ fn pnpm_install_produces_symlinked_layout() { #[test] #[ignore] fn apply_in_a_does_not_mutate_b_or_store() { - if !has_command("pnpm") { + if !has_pnpm() { skip_missing_pnpm(); return; } @@ -417,7 +433,7 @@ fn apply_in_a_does_not_mutate_b_or_store() { #[test] #[ignore] fn pnpm_install_in_b_does_not_revert_a() { - if !has_command("pnpm") { + if !has_pnpm() { skip_missing_pnpm(); return; } @@ -532,7 +548,7 @@ fn pnpm_install_in_b_does_not_revert_a() { #[test] #[ignore] fn apply_in_pnpm_project_emits_layout_note() { - if !has_command("pnpm") { + if !has_pnpm() { skip_missing_pnpm(); return; } diff --git a/scripts/tests/test_ci_e2e_tiers.py b/scripts/tests/test_ci_e2e_tiers.py index 81886440b..6ea29bad9 100644 --- a/scripts/tests/test_ci_e2e_tiers.py +++ b/scripts/tests/test_ci_e2e_tiers.py @@ -273,8 +273,8 @@ def test_compat_workflow_builds_its_own_binaries(self): class PnpmRequiredGate(unittest.TestCase): """The pnpm-family suites soft-skip when pnpm is missing, and libtest - counts a skip as `ok`, so their Linux rows must set the fail-instead-of- - skip gate (#1054).""" + counts a skip as `ok`, so every row that runs them, on every OS, must set + the fail-instead-of-skip gate (#1054).""" suites = {"e2e_redirect_pnpm_build", "e2e_safety_pnpm", "e2e_redirect_rush_sim"} jobs = ("e2e", "e2e-full", "e2e-extended", "e2e-windows", "e2e-macos") @@ -283,20 +283,42 @@ def test_run_step_reads_the_row_key(self): run = dict(rows_mod.steps(JOBS["e2e"]))["Run e2e tests"] self.assertIn("SOCKET_PATCH_PNPM_E2E_REQUIRED: ${{ matrix.pnpm_required || '' }}", run) - def test_every_linux_pnpm_row_sets_the_gate(self): - gated = set() + def test_every_pnpm_row_sets_the_gate(self): + gated = {} for job in self.jobs: for row in rows(job): - if self.suites & set(row["suite"].split()) and row["os"].startswith("ubuntu"): + if self.suites & set(row["suite"].split()): with self.subTest(job=job, row=row): self.assertEqual(row.get("pnpm_required"), "1") - gated |= self.suites & set(row["suite"].split()) + for suite in self.suites & set(row["suite"].split()): + gated.setdefault(suite, set()).add(row["os"].split("-")[0]) elif "pnpm_required" in row: self.fail(f"{job} row outside the checked set sets pnpm_required: {row}") - self.assertEqual(gated, self.suites) + self.assertEqual(gated, {s: {"ubuntu", "windows", "macos"} for s in self.suites}) self.assertIn("e2e_redirect_pnpm_build", {r["suite"] for r in rows("e2e") if r.get("pnpm_required")}, "the PR tier must keep its gated pnpm row") + def test_suites_resolve_windows_shims(self): + # A bare `Command::new("pnpm")` / `Command::new("corepack")` can't + # spawn the `.cmd` shims npm and Node install on Windows, so every + # Windows test skipped (passed in 0.02 s) until the gate made that a + # failure. `hermetic::tool_command` resolves them through PATHEXT. + for suite in sorted(self.suites): + with self.subTest(suite=suite): + src = (TESTS / f"{suite}.rs").read_text(encoding="utf-8") + self.assertNotRegex(src, r'Command::new\("(pnpm|corepack|npm)(\.cmd)?"\)') + if "fn has_command(" in src: + # A suite-local probe must resolve shims itself... + local = src[src.index("fn has_command("):] + self.assertIn("hermetic::tool_command(cmd)", local[:local.index("\n}\n")]) + else: + # ...and `common::has_command` does not. + self.assertNotRegex(src, r'has_command\("(pnpm|corepack|npm)"\)') + # `pnpm_run` goes through the shared runner. + common = (TESTS / "common" / "mod.rs").read_text(encoding="utf-8") + runner = common[common.index("fn run_toolchain("):] + self.assertIn("hermetic::tool_command(exe)", runner[:runner.index("\n}\n")]) + def test_every_gated_suite_honours_the_gate(self): for suite in sorted(self.suites): with self.subTest(suite=suite):