diff --git a/AGENTS.md b/AGENTS.md index 973267c80..e671fd6c9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -48,6 +48,13 @@ Before writing code for a feature, bug fix, or behavior change: [Reporter / log events](./CODE_STYLE_GUIDE.md#reporter--log-events) in the style guide for the convention (channel mapping, threading `R: Reporter`, emit-site placement, recording-fake tests). +7. **Side-effecting code uses the dependency-injection seam.** Any + new code that touches the filesystem, environment variables, + network, time, or process state goes through a capability trait + on the `Host` provider, threaded as `Sys: ` through the + call site. See [Dependency injection for tests](./CODE_STYLE_GUIDE.md#dependency-injection-for-tests) + in the style guide for the eight principles, the worked example, + and the guidance on when a real fixture is simpler than a fake. If the upstream behavior is unclear or looks wrong, stop and ask the user rather than guessing. diff --git a/CODE_STYLE_GUIDE.md b/CODE_STYLE_GUIDE.md index 9d20b8f02..7fe890ee9 100644 --- a/CODE_STYLE_GUIDE.md +++ b/CODE_STYLE_GUIDE.md @@ -587,6 +587,127 @@ The external file itself sits in a directory named after the parent, using the s Do not flatten the tests into a sibling file such as `src/foo_tests.rs`, and do not skip the intermediate directory when the parent currently has no other submodules. This mirrors the flat file pattern (`module.rs` rather than `module/mod.rs`) described under [Module Organization](#module-organization). +### Dependency injection for tests + +Code that touches the outside world — filesystem, environment variables, network, time, process state — needs a seam tests can swap out. Pacquet's seams are *capability traits*: small static-method traits, one per primitive effect, all carried by a single zero-sized provider type. Production turbofishes the real provider; tests turbofish a unit struct declared inside the `#[test]` body. + +The current implementations live in [`crates/modules-yaml`](./crates/modules-yaml/src/lib.rs) (filesystem + clock), [`crates/cmd-shim`](./crates/cmd-shim/src/capabilities.rs) (filesystem, including exec-bit chmod), [`crates/config`](./crates/config/src/api.rs) (environment variables), and [`crates/reporter`](./crates/reporter/src/lib.rs) (`gethostname`). New side-effecting code added to the workspace should follow the same shape. + +#### Names + +- **Generic parameter: `Sys`.** The single type parameter that carries every capability bound a function consumes. Reads as "the system this function runs against." Never use a domain-specific name like `Fs` or `Env` for the generic — the generic spans every domain the function reaches into. +- **Production provider: `Host`.** The zero-sized struct that implements every capability trait by delegating to the real OS / `std` call. Unqualified, never `RealHost` or `DefaultHost`; the convention is that production is unqualified and fakes carry the qualifier. +- **Trait names: ``.** Filesystem traits carry an `Fs` prefix and mirror the matching `std::fs` name (`FsReadToString`, `FsCreateDirAll`, `FsWrite`). Other domains use the matching prefix (`Env*`, `GetDisk*`, `Http*`, …). A reader sees `Sys: FsCreateDirAll + EnvVar` and knows immediately which two domains the function reaches into. +- **Fake names describe the behaviour, not the domain.** A fake whose `read_to_string` returns garbage yaml is `BadYamlContent`, not `BadYamlFs`. The `Fs` suffix would label the domain — but fakes are providers, and by the principles below providers stay domain-neutral. + +#### Principles + +1. **One trait per capability.** Each side-effecting operation gets its own trait. Functions bind only what they consume, and test fakes implement only what gets called. Avoid lumped traits such as `FsApi { read, mkdir, write, ... }` that force `unreachable!()` to fill the unused methods. + +2. **One generic parameter satisfying multiple bounds.** When a function needs several capabilities, compose them as ``. Resist the multi-parameter form ``. The generic is a capability provider, not a capability domain, so it should never be named after one domain. + +3. **Capability methods take no `&self`. Use `static`s for stateful fakes.** Capability methods are associated functions. Production implementations are unit structs (`struct Host;`). Fakes are unit structs declared inside the test function, alongside any per-test scenario data held in `static`s such as `static CALLS: Mutex> = Mutex::new(Vec::new())` or `static SEQUENCE_INDEX: AtomicUsize = AtomicUsize::new(0)`. + + Declare each interior-mutable `static` inside the body of the `#[test]` function that owns it. Never at module scope, and never inside a helper that several tests call. The test runner invokes each `#[test]` function exactly once per process, so a `static` declared inside a single test body is only ever touched from one test thread, which keeps the fake stateless from the trait's perspective and free of data races. An interior-mutable `static` shared across tests is no longer per-test: multiple test threads can hit it in parallel and race. + + Immutable `static`s are exempt. Items such as `static FIXTURE_BYTES: &[u8] = b"...";` declared at module scope and shared across tests are fine — reads do not race. + +4. **Associated types instead of `&self` when a capability operates over a data type.** If a capability would otherwise force callers to pass an instance just to extract data, lift the data type to an associated type. The capability stays a static-method namespace, and the associated type lets one provider describe how to operate over a chosen data shape (see the disk-inspection sketch in principle 6's worked example). + +5. **Capability traits live on the implementor, not the data.** The capability provider `Host` implements the trait `FsReadToString`. The value type `Path` does not. This keeps the capability implementation swappable without changing call sites. + +6. **Provider names are domain-neutral; trait names are domain-scoped.** `Sys` (generic) and `Host` (production) carry every capability the workspace needs, regardless of how many domains they cover. Traits keep a domain prefix (`Fs*`, `Env*`, `GetDisk*`, …) so a reader can identify which domain a bound belongs to without chasing definitions. + +7. **Production callers turbofish the real implementation explicitly.** For example, `read_modules_manifest::(dir)`. Defaults on free-function type parameters are unstable on stable Rust, so an explicit turbofish is the price of zero-cost dependency injection. + +8. **Capabilities are primitives, not algorithms.** A capability trait names a leaf-level effect — typically a single `std` function or a single syscall. Its method signature and its name promise only what the underlying primitive promises. When a feature needs a stronger guarantee (atomic file replacement, transactional state machines, retried HTTP requests), implement it as a free function or higher-level type that *consumes* the primitive capabilities. Do not promote the algorithm into a capability trait of its own. The canonical failure mode is `FsWriteAtomic` backed by `std::fs::write`: the trait name lies to every caller, and the lie spreads through the public surface of the crate. The right shape is `FsWrite` (matching `std::fs::write`) plus a free function `atomic_write` once atomicity is actually needed. + +#### Worked example + +The filesystem and clock capabilities in [`crates/modules-yaml/src/lib.rs`](./crates/modules-yaml/src/lib.rs) are the canonical reference. The shape is: + +```rust +pub trait FsReadToString { fn read_to_string(path: &Path) -> io::Result; } +pub trait FsCreateDirAll { fn create_dir_all(path: &Path) -> io::Result<()>; } +pub trait FsWrite { fn write(path: &Path, contents: &[u8]) -> io::Result<()>; } +pub trait Clock { fn now() -> SystemTime; } + +pub struct Host; +impl FsReadToString for Host { /* delegates to std::fs::read_to_string */ } +impl FsCreateDirAll for Host { /* delegates to std::fs::create_dir_all */ } +impl FsWrite for Host { /* delegates to std::fs::write */ } +impl Clock for Host { /* delegates to SystemTime::now */ } +``` + +Public APIs bind the minimal capabilities: + +```rust +pub fn read_modules_manifest(modules_dir: &Path) -> Result, ReadModulesError> +where + Sys: FsReadToString + Clock, +{ /* ... */ } + +pub fn write_modules_manifest(modules_dir: &Path, manifest: Modules) -> Result<(), WriteModulesError> +where + Sys: FsCreateDirAll + FsWrite, +{ /* ... */ } +``` + +Test fakes implement only the bound the function under test consumes: + +```rust +#[test] +fn read_propagates_parse_error() { + struct BadYamlContent; + impl FsReadToString for BadYamlContent { + fn read_to_string(_: &Path) -> io::Result { + Ok("{ this is not valid yaml or json".to_string()) + } + } + impl Clock for BadYamlContent { + fn now() -> SystemTime { + unreachable!("clock must not be called when YAML parsing fails"); + } + } + + let err = read_modules_manifest::(Path::new("/dev/null/unused")) + .expect_err("expected error"); + assert!(matches!(err, ReadModulesError::ParseYaml { .. })); +} +``` + +The fake declares only `FsReadToString` and `Clock` because those are the bounds on `read_modules_manifest`. No `FsWrite` or `FsCreateDirAll` impls are needed. + +#### Combining capabilities of different domains + +A function reaching into multiple domains binds them all under the single `Sys` generic. When several traits share an associated type, an equality bound locks them to the same data type so the function operates on a single slice without re-introducing a lumped super-trait: + +```rust +fn warm_cache_for_local_disks( + disks: &[::Disk], + seed_url: &str, +) -> Result<(), CacheError> +where + Sys: GetDiskKind + + GetDiskName::Disk> + + GetMountPoint::Disk> + + HttpGet + + FsCreateDirAll + + FsWrite, +{ /* ... */ } +``` + +Tests provide their own fake `Sys` that implements every capability the function needs, sets `type Disk = FakeDisk;`, and defines `FakeDisk` inside the test function. No `&self`, no per-test instance plumbing, no second generic parameter per domain. + +#### When dependency injection is not needed + +Some tests should not use dependency injection because a real fixture is simpler: + +- **Real filesystem fixtures.** If the input can be produced cheaply by writing files under `tests/fixtures/` or a `tempdir()`, use that. See [`crates/modules-yaml/tests/real_fs.rs`](./crates/modules-yaml/tests/real_fs.rs) for the canonical pattern. +- **Pure logic.** Functions that take values and return values — `is_string_present`, `derive_hoisted_dependencies`, the bin-name validators — are tested with plain assertions on inputs and outputs. + +Dependency injection is for error paths that the real filesystem cannot reach portably (`PermissionDenied`, `ENOSPC`, mid-read truncation), for time-dependent branches (`prunedAt` defaulting), and for behaviour whose triggering conditions are awkward to set up on disk. + ### Cloning `Arc` and `Rc` Prefer using `Arc::clone` or `Rc::clone` to vague `.clone()` or `Clone::clone`. diff --git a/crates/cli/src/cli_args.rs b/crates/cli/src/cli_args.rs index 21305fc3f..8af3ab24c 100644 --- a/crates/cli/src/cli_args.rs +++ b/crates/cli/src/cli_args.rs @@ -9,7 +9,7 @@ use add::AddArgs; use clap::{Parser, Subcommand, ValueEnum}; use install::InstallArgs; use miette::{Context, IntoDiagnostic}; -use pacquet_config::{Config, RealApi}; +use pacquet_config::{Config, Host}; use pacquet_executor::execute_shell; use pacquet_package_manifest::PackageManifest; use pacquet_reporter::{NdjsonReporter, SilentReporter}; @@ -94,12 +94,12 @@ impl CliArgs { // builds its `localPrefix` from `cliOptions.dir`, not `cwd`) — // see [`loadNpmrcConfig`](https://github.com/pnpm/pnpm/blob/1819226b51/config/reader/src/loadNpmrcFiles.ts#L48-L50). // - // Production callers turbofish `RealApi` explicitly so the + // Production callers turbofish `Host` explicitly so the // dependency-injection plumbing is visible at the call site. // See [pnpm/pacquet#339](https://github.com/pnpm/pacquet/issues/339) // for the pattern and rationale. let config = || -> miette::Result<&'static mut Config> { - Config::current::( + Config::current::( || Ok::<_, std::convert::Infallible>(dir.clone()), home::home_dir, Default::default, diff --git a/crates/cli/tests/install.rs b/crates/cli/tests/install.rs index aa3d992bc..0d339a6d1 100644 --- a/crates/cli/tests/install.rs +++ b/crates/cli/tests/install.rs @@ -253,7 +253,7 @@ fn should_install_circular_dependencies() { /// End-to-end coverage for `${VAR}` substitution in `.npmrc`. /// -/// `::var` (the `std::env::var` bridge in +/// `::var` (the `std::env::var` bridge in /// `crates/config/src/api.rs`) is unreachable by every other test /// because `add_mocked_registry` writes literal values, so /// `env_replace` short-circuits at the no-`$` branch. @@ -264,7 +264,7 @@ fn should_install_circular_dependencies() { /// upstream's [`installing/deps-installer/test/install/auth.ts`](https://github.com/pnpm/pnpm/blob/601317e7a3/installing/deps-installer/test/install/auth.ts) /// is not exercised here. The mock registry doesn't gate on auth, so /// substituting the registry URL is the smallest scenario that drives -/// `::var` end-to-end. Token-substitution coverage +/// `::var` end-to-end. Token-substitution coverage /// belongs in a test against a registry that actually validates the /// header. #[test] diff --git a/crates/cmd-shim/src/bin_resolver.rs b/crates/cmd-shim/src/bin_resolver.rs index 941fa2d7c..3621bf01a 100644 --- a/crates/cmd-shim/src/bin_resolver.rs +++ b/crates/cmd-shim/src/bin_resolver.rs @@ -63,7 +63,7 @@ pub fn pkg_owns_bin(bin_name: &str, pkg_name: &str) -> bool { /// single-character `$`. This is the path-traversal guard. /// - Bin path must resolve under `pkg_path`. Prevents a malicious manifest /// from writing shims that exec a sibling package. -pub fn get_bins_from_package_manifest( +pub fn get_bins_from_package_manifest( manifest: &Value, pkg_path: &Path, ) -> Vec { @@ -74,7 +74,7 @@ pub fn get_bins_from_package_manifest( if let Some(bin_dir_rel) = manifest.get("directories").and_then(|d| d.get("bin")).and_then(Value::as_str) { - return commands_from_directories_bin::(bin_dir_rel, pkg_path); + return commands_from_directories_bin::(bin_dir_rel, pkg_path); } Vec::new() } @@ -87,7 +87,7 @@ pub fn get_bins_from_package_manifest( /// Symlinks are not followed; pnpm uses `tinyglobby` with /// `followSymbolicLinks: false`. Missing directory degrades to an empty /// list (pnpm's `ENOENT` short-circuit). -fn commands_from_directories_bin( +fn commands_from_directories_bin( bin_dir_rel: &str, pkg_path: &Path, ) -> Vec { @@ -99,7 +99,7 @@ fn commands_from_directories_bin( // tinyglobby ENOENT short-circuit. The trait's production impl // already drops per-entry errors inside its iterator, so an `Err` // here only fires when the walker can't even open `bin_dir`. - let Ok(paths) = Api::walk_files(&bin_dir) else { + let Ok(paths) = Sys::walk_files(&bin_dir) else { return Vec::new(); }; let mut commands = Vec::new(); diff --git a/crates/cmd-shim/src/bin_resolver/tests.rs b/crates/cmd-shim/src/bin_resolver/tests.rs index 0e4cba87a..4b873ae94 100644 --- a/crates/cmd-shim/src/bin_resolver/tests.rs +++ b/crates/cmd-shim/src/bin_resolver/tests.rs @@ -1,5 +1,5 @@ use super::{get_bins_from_package_manifest, pkg_owns_bin}; -use crate::{capabilities::RealApi, path_util::lexical_normalize}; +use crate::{capabilities::Host, path_util::lexical_normalize}; use pipe_trait::Pipe; use serde_json::json; use std::{ @@ -11,7 +11,7 @@ use tempfile::tempdir; #[test] fn bin_as_string_uses_package_name() { let manifest = json!({"name": "foo", "bin": "cli.js"}); - let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg/foo")); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg/foo")); assert_eq!(commands.len(), 1); assert_eq!(commands[0].name, "foo"); assert_eq!(commands[0].path, Path::new("/pkg/foo/cli.js")); @@ -20,7 +20,7 @@ fn bin_as_string_uses_package_name() { #[test] fn bin_as_string_strips_scope() { let manifest = json!({"name": "@scope/foo", "bin": "cli.js"}); - let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg/foo")); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg/foo")); assert_eq!(commands.len(), 1); assert_eq!(commands[0].name, "foo"); } @@ -34,7 +34,7 @@ fn bin_as_object_keeps_keys_and_strips_scope() { "@scope/extra": "bin/extra.js", }, }); - let mut commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + let mut commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); commands.sort_by(|a, b| a.name.cmp(&b.name)); assert_eq!(commands.len(), 2); assert_eq!(commands[0].name, "extra"); @@ -52,7 +52,7 @@ fn rejects_unsafe_bin_names() { "$": "dollar.js", }, }); - let mut names: Vec<_> = get_bins_from_package_manifest::(&manifest, Path::new("/p")) + let mut names: Vec<_> = get_bins_from_package_manifest::(&manifest, Path::new("/p")) .into_iter() .map(|c| c.name) .collect(); @@ -66,14 +66,14 @@ fn rejects_path_traversal_outside_package_root() { "name": "x", "bin": {"x": "../../../etc/passwd"}, }); - let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg/x")); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg/x")); assert!(commands.is_empty(), "must reject `..`-escapes from pkg root"); } #[test] fn no_bin_field_returns_empty() { let manifest = json!({"name": "x"}); - assert!(get_bins_from_package_manifest::(&manifest, Path::new("/p")).is_empty()); + assert!(get_bins_from_package_manifest::(&manifest, Path::new("/p")).is_empty()); } #[test] @@ -101,7 +101,7 @@ fn dollar_is_allowed_as_command_name() { "version": "1.0.0", "bin": {"$": "./undollar.js"}, }); - let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); assert_eq!(commands.len(), 1); assert_eq!(commands[0].name, "$"); } @@ -122,7 +122,7 @@ fn skip_dangerous_bin_names() { "~/bad": "./bad", }, }); - let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); assert_eq!(commands.len(), 1); assert_eq!(commands[0].name, "good"); } @@ -140,7 +140,7 @@ fn skip_dangerous_bin_locations() { "good": "./good", }, }); - let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg/foo")); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg/foo")); assert_eq!(commands.len(), 1); assert_eq!(commands[0].name, "good"); } @@ -155,7 +155,7 @@ fn scoped_bin_name_strips_scope_prefix() { "version": "1.0.0", "bin": {"@foo/a": "./a"}, }); - let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); assert_eq!(commands.len(), 1); assert_eq!(commands[0].name, "a"); } @@ -176,7 +176,7 @@ fn skip_scoped_bin_names_with_path_traversal() { "@scope/legit": "./good.js", }, }); - let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); assert_eq!(commands.len(), 1); assert_eq!(commands[0].name, "legit"); } @@ -189,7 +189,7 @@ fn malformed_bin_type_returns_empty() { for shape in [json!(42), json!(["a", "b"]), json!(null), json!(true)] { let manifest = json!({"name": "x", "version": "1.0.0", "bin": shape}); assert!( - get_bins_from_package_manifest::(&manifest, Path::new("/p")).is_empty(), + get_bins_from_package_manifest::(&manifest, Path::new("/p")).is_empty(), "malformed bin shape must be tolerated", ); } @@ -202,7 +202,7 @@ fn malformed_bin_type_returns_empty() { #[test] fn bin_string_with_missing_package_name_returns_empty() { let manifest = json!({"bin": "cli.js"}); - assert!(get_bins_from_package_manifest::(&manifest, Path::new("/p")).is_empty()); + assert!(get_bins_from_package_manifest::(&manifest, Path::new("/p")).is_empty()); } /// Object-form bin entries whose values aren't strings (number, null, etc.) @@ -219,7 +219,7 @@ fn bin_object_with_non_string_value_is_skipped() { "bad-null": null, }, }); - let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); assert_eq!(commands.len(), 1); assert_eq!(commands[0].name, "good"); } @@ -242,7 +242,7 @@ fn directories_bin_walks_files_recursively() { "version": "1.0.0", "directories": {"bin": "bin-dir"}, }); - let mut commands = get_bins_from_package_manifest::(&manifest, &pkg); + let mut commands = get_bins_from_package_manifest::(&manifest, &pkg); commands.sort_by(|a, b| a.name.cmp(&b.name)); assert_eq!(commands.len(), 2); assert_eq!(commands[0].name, "rootBin.js"); @@ -277,7 +277,7 @@ fn directories_bin_rejects_path_traversal() { "directories": {"bin": "../siblings"}, }); assert!( - get_bins_from_package_manifest::(&manifest, &pkg).is_empty(), + get_bins_from_package_manifest::(&manifest, &pkg).is_empty(), "is_subdir guard must reject `..`-escapes from the pkg root, even \ when the resolved directory exists and has files", ); @@ -306,7 +306,7 @@ fn directories_bin_rejects_real_path_traversal() { "version": "1.0.0", "directories": {"bin": "../secret"}, }); - assert!(get_bins_from_package_manifest::(&manifest, &pkg).is_empty()); + assert!(get_bins_from_package_manifest::(&manifest, &pkg).is_empty()); } /// `directories.bin` pointing at a non-existent subdirectory must @@ -321,7 +321,7 @@ fn directories_bin_missing_directory_returns_empty() { "version": "1.0.0", "directories": {"bin": "missing-dir"}, }); - assert!(get_bins_from_package_manifest::(&manifest, &pkg).is_empty()); + assert!(get_bins_from_package_manifest::(&manifest, &pkg).is_empty()); } /// `directories.bin` filters out files whose basename fails the @@ -342,7 +342,7 @@ fn directories_bin_filters_unsafe_file_names() { "version": "1.0.0", "directories": {"bin": "bin"}, }); - let mut commands = get_bins_from_package_manifest::(&manifest, &pkg); + let mut commands = get_bins_from_package_manifest::(&manifest, &pkg); commands.sort_by(|a, b| a.name.cmp(&b.name)); assert_eq!(commands.len(), 1); assert_eq!(commands[0].name, "good"); @@ -357,7 +357,7 @@ fn empty_bin_key_is_rejected() { "version": "1.0.0", "bin": {"": "ok.js", "good": "ok.js"}, }); - let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); assert_eq!(commands.len(), 1); assert_eq!(commands[0].name, "good"); } @@ -413,7 +413,7 @@ fn directories_bin_accepts_excess_parent_dirs_that_resolve_inside_pkg() { "version": "1.0.0", "directories": {"bin": "x/../../pkg/bin-dir"}, }); - let commands = get_bins_from_package_manifest::(&manifest, &pkg); + let commands = get_bins_from_package_manifest::(&manifest, &pkg); assert_eq!(commands.len(), 1); assert_eq!(commands[0].name, "cli"); } @@ -434,7 +434,7 @@ fn directories_bin_handles_curdir_in_relative_path() { "version": "1.0.0", "directories": {"bin": "./bin-dir"}, }); - let commands = get_bins_from_package_manifest::(&manifest, &pkg); + let commands = get_bins_from_package_manifest::(&manifest, &pkg); assert_eq!(commands.len(), 1); assert_eq!(commands[0].name, "cli"); } @@ -497,7 +497,7 @@ fn bin_field_takes_precedence_over_directories_bin() { "bin": "primary.js", "directories": {"bin": "legacy-bin"}, }); - let commands = get_bins_from_package_manifest::(&manifest, &pkg); + let commands = get_bins_from_package_manifest::(&manifest, &pkg); assert_eq!(commands.len(), 1, "bin field wins, directories.bin is ignored"); assert_eq!(commands[0].name, "tool"); } diff --git a/crates/cmd-shim/src/capabilities.rs b/crates/cmd-shim/src/capabilities.rs index c4d824e7a..23be9682a 100644 --- a/crates/cmd-shim/src/capabilities.rs +++ b/crates/cmd-shim/src/capabilities.rs @@ -1,11 +1,6 @@ //! Per-capability dependency-injection traits and the production -//! [`RealApi`] provider. Mirrors the pattern documented at -//! : -//! -//! 1. One trait per capability. -//! 2. Functions bind only what they consume (compose bounds). -//! 3. No `&self` on capability methods. -//! 4. Production callers turbofish the real impl explicitly. +//! [`Host`] provider. See the "Dependency injection for tests" +//! section of `CODE_STYLE_GUIDE.md` for the full set of principles. //! //! Tests inject unit-struct fakes to exercise IO error paths that the //! real filesystem can't reach portably (e.g. permission denied, @@ -27,7 +22,7 @@ use std::{ /// generic over this trait, so test fakes do not have to grow. /// /// The trait makes no claim about how many syscalls a particular -/// impl will use — the production `RealApi` impl opens the file, +/// impl will use — the production `Host` impl opens the file, /// seeks to `offset` (if non-zero), and reads, which is more than /// one. What it does promise is the semantic contract: read up to /// `buf.len()` bytes starting at `offset` into `buf`. @@ -48,7 +43,7 @@ pub trait FsReadFile { /// Read the entire contents of a file into a `String`. Used by /// [`crate::link_bins_of_packages`] to short-circuit on warm reinstalls /// where the existing shim already targets the same bin file. -pub trait FsReadString { +pub trait FsReadToString { fn read_to_string(path: &Path) -> io::Result; } @@ -145,9 +140,9 @@ pub trait FsEnsureExecutableBits { /// The production filesystem provider. Every method delegates straight /// to `std::fs`. -pub struct RealApi; +pub struct Host; -impl FsReadHead for RealApi { +impl FsReadHead for Host { fn read_head(path: &Path, offset: u64, buf: &mut [u8]) -> io::Result { use std::io::{Read, Seek, SeekFrom}; let mut file = std::fs::File::open(path)?; @@ -158,19 +153,19 @@ impl FsReadHead for RealApi { } } -impl FsReadFile for RealApi { +impl FsReadFile for Host { fn read_file(path: &Path) -> io::Result> { std::fs::read(path) } } -impl FsReadString for RealApi { +impl FsReadToString for Host { fn read_to_string(path: &Path) -> io::Result { std::fs::read_to_string(path) } } -impl FsReadDir for RealApi { +impl FsReadDir for Host { fn read_dir(path: &Path) -> io::Result> { // `flatten()` silently drops per-entry errors. This matches the // prior collect-then-flatten shape and the `tinyglobby`-style @@ -179,7 +174,7 @@ impl FsReadDir for RealApi { } } -impl FsWalkFiles for RealApi { +impl FsWalkFiles for Host { fn walk_files(path: &Path) -> io::Result> { // `flatten()` silently drops per-entry errors and matches // pnpm's `tinyglobby` ENOENT-on-subtree behaviour. The @@ -196,20 +191,20 @@ impl FsWalkFiles for RealApi { } } -impl FsCreateDirAll for RealApi { +impl FsCreateDirAll for Host { fn create_dir_all(path: &Path) -> io::Result<()> { std::fs::create_dir_all(path) } } -impl FsWrite for RealApi { +impl FsWrite for Host { fn write(path: &Path, bytes: &[u8]) -> io::Result<()> { std::fs::write(path, bytes) } } #[cfg(unix)] -impl FsSetExecutable for RealApi { +impl FsSetExecutable for Host { fn set_executable(path: &Path) -> io::Result<()> { use std::os::unix::fs::PermissionsExt; std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)) @@ -217,14 +212,14 @@ impl FsSetExecutable for RealApi { } #[cfg(not(unix))] -impl FsSetExecutable for RealApi { +impl FsSetExecutable for Host { fn set_executable(_path: &Path) -> io::Result<()> { Ok(()) } } #[cfg(unix)] -impl FsEnsureExecutableBits for RealApi { +impl FsEnsureExecutableBits for Host { fn ensure_executable_bits(path: &Path) -> io::Result<()> { use std::os::unix::fs::PermissionsExt; let metadata = std::fs::metadata(path)?; @@ -234,7 +229,7 @@ impl FsEnsureExecutableBits for RealApi { } #[cfg(not(unix))] -impl FsEnsureExecutableBits for RealApi { +impl FsEnsureExecutableBits for Host { fn ensure_executable_bits(_path: &Path) -> io::Result<()> { Ok(()) } diff --git a/crates/cmd-shim/src/link_bins.rs b/crates/cmd-shim/src/link_bins.rs index 6c7627afe..415b34676 100644 --- a/crates/cmd-shim/src/link_bins.rs +++ b/crates/cmd-shim/src/link_bins.rs @@ -1,7 +1,7 @@ use crate::{ bin_resolver::{Command, get_bins_from_package_manifest, pkg_owns_bin}, capabilities::{ - FsCreateDirAll, FsEnsureExecutableBits, FsReadDir, FsReadFile, FsReadHead, FsReadString, + FsCreateDirAll, FsEnsureExecutableBits, FsReadDir, FsReadFile, FsReadHead, FsReadToString, FsSetExecutable, FsWalkFiles, FsWrite, }, shim::{ @@ -110,11 +110,11 @@ pub enum LinkBinsError { /// /// Scoped packages are recursed: `node_modules/@scope/foo` becomes one /// candidate. This mirrors `binNamesAndPaths` in upstream `linkBins`. -pub fn link_bins(modules_dir: &Path, bins_dir: &Path) -> Result<(), LinkBinsError> +pub fn link_bins(modules_dir: &Path, bins_dir: &Path) -> Result<(), LinkBinsError> where - Api: FsReadDir + Sys: FsReadDir + FsReadFile - + FsReadString + + FsReadToString + FsReadHead + FsCreateDirAll + FsWalkFiles @@ -122,19 +122,19 @@ where + FsSetExecutable + FsEnsureExecutableBits, { - let packages = collect_packages_in_modules_dir::(modules_dir)?; - link_bins_of_packages::(&packages, bins_dir) + let packages = collect_packages_in_modules_dir::(modules_dir)?; + link_bins_of_packages::(&packages, bins_dir) } -fn collect_packages_in_modules_dir( +fn collect_packages_in_modules_dir( modules_dir: &Path, ) -> Result, LinkBinsError> where - Api: FsReadDir + FsReadFile, + Sys: FsReadDir + FsReadFile, { let mut packages = Vec::new(); - let entries = match Api::read_dir(modules_dir) { + let entries = match Sys::read_dir(modules_dir) { Ok(entries) => entries, Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(packages), Err(error) => { @@ -159,7 +159,7 @@ where // scope, so surface them as `ReadModulesDir`. Matches // the policy the per-`modules_dir` read above already // uses. - let scope_entries = match Api::read_dir(&path) { + let scope_entries = match Sys::read_dir(&path) { Ok(entries) => entries, Err(error) if error.kind() == io::ErrorKind::NotFound => continue, Err(error) => { @@ -167,14 +167,14 @@ where } }; for sub_path in scope_entries { - if let Some(pkg) = read_package::(&sub_path)? { + if let Some(pkg) = read_package::(&sub_path)? { packages.push(pkg); } } continue; } - if let Some(pkg) = read_package::(&path)? { + if let Some(pkg) = read_package::(&path)? { packages.push(pkg); } } @@ -182,11 +182,11 @@ where Ok(packages) } -fn read_package( +fn read_package( location: &Path, ) -> Result, LinkBinsError> { let manifest_path = location.join("package.json"); - let bytes = match Api::read_file(&manifest_path) { + let bytes = match Sys::read_file(&manifest_path) { Ok(bytes) => bytes, Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), Err(error) => return Err(LinkBinsError::ReadManifest { path: manifest_path, error }), @@ -210,12 +210,12 @@ fn read_package( /// conflicts via semver (a feature upstream uses for hoisting), since the /// virtual-store layout means each bin source is a unique /// `(package, version)` slot already. -pub fn link_bins_of_packages( +pub fn link_bins_of_packages( packages: &[PackageBinSource], bins_dir: &Path, ) -> Result<(), LinkBinsError> where - Api: FsReadString + Sys: FsReadToString + FsReadHead + FsCreateDirAll + FsWalkFiles @@ -227,7 +227,7 @@ where for pkg in packages { let pkg_name = pkg.manifest.get("name").and_then(Value::as_str).unwrap_or(""); - let commands = get_bins_from_package_manifest::(&pkg.manifest, &pkg.location); + let commands = get_bins_from_package_manifest::(&pkg.manifest, &pkg.location); for command in commands { match chosen.get(&command.name) { None => { @@ -248,7 +248,7 @@ where return Ok(()); } - Api::create_dir_all(bins_dir) + Sys::create_dir_all(bins_dir) .map_err(|error| LinkBinsError::CreateBinDir { dir: bins_dir.to_path_buf(), error })?; // Each shim's read-shebang + write-file + chmod sequence is independent @@ -256,7 +256,7 @@ where // The hot path is per-package-bin; without parallelism the per-shim // file I/O serialised across the whole `chosen` map. chosen.par_iter().try_for_each(|(bin_name, (command, _pkg))| { - write_shim::(&command.path, &bins_dir.join(bin_name)) + write_shim::(&command.path, &bins_dir.join(bin_name)) })?; Ok(()) @@ -297,11 +297,11 @@ fn pick_winner(bin_name: &str, existing: &str, candidate: &str) -> bool { /// they are no-ops (Windows has no equivalent permission concept), so /// the call sites stay portable and don't need their own /// `#[cfg(unix)]` gating. -fn write_shim(target_path: &Path, shim_path: &Path) -> Result<(), LinkBinsError> +fn write_shim(target_path: &Path, shim_path: &Path) -> Result<(), LinkBinsError> where - Api: FsReadString + FsReadHead + FsWrite + FsSetExecutable + FsEnsureExecutableBits, + Sys: FsReadToString + FsReadHead + FsWrite + FsSetExecutable + FsEnsureExecutableBits, { - let runtime = search_script_runtime::(target_path).map_err(|error| { + let runtime = search_script_runtime::(target_path).map_err(|error| { LinkBinsError::ProbeShimSource { path: target_path.to_path_buf(), error } })?; @@ -333,29 +333,29 @@ where // bodies are stable across pacquet versions (only the `` // segment moves), so byte equality is a sound equivalence check. let sh_marker_ok = matches!( - Api::read_to_string(shim_path), + Sys::read_to_string(shim_path), Ok(existing) if is_shim_pointing_at(&existing, target_path), ); let cmd_ok = matches!( - Api::read_to_string(&cmd_path), + Sys::read_to_string(&cmd_path), Ok(existing) if existing == cmd_body, ); let ps1_ok = matches!( - Api::read_to_string(&ps1_path), + Sys::read_to_string(&ps1_path), Ok(existing) if existing == ps1_body, ); let already_correct = sh_marker_ok && cmd_ok && ps1_ok; if !already_correct { - Api::write(shim_path, sh_body.as_bytes()) + Sys::write(shim_path, sh_body.as_bytes()) .map_err(|error| LinkBinsError::WriteShim { path: shim_path.to_path_buf(), error })?; - Api::write(&cmd_path, cmd_body.as_bytes()) + Sys::write(&cmd_path, cmd_body.as_bytes()) .map_err(|error| LinkBinsError::WriteShim { path: cmd_path.clone(), error })?; - Api::write(&ps1_path, ps1_body.as_bytes()) + Sys::write(&ps1_path, ps1_body.as_bytes()) .map_err(|error| LinkBinsError::WriteShim { path: ps1_path.clone(), error })?; } - Api::set_executable(shim_path) + Sys::set_executable(shim_path) .map_err(|error| LinkBinsError::Chmod { path: shim_path.to_path_buf(), error })?; // Make the underlying script executable too. pnpm calls // `fixBin(cmd.path, 0o755)` to do this; we apply the same minimum @@ -369,7 +369,7 @@ where // AppArmor deny, foreign uid) surfaces as `LinkBinsError::Chmod` // so real failures don't disappear silently. Mirrors pnpm's // `fixBin` ENOENT guard. - match Api::ensure_executable_bits(target_path) { + match Sys::ensure_executable_bits(target_path) { Ok(()) => {} Err(error) if error.kind() == io::ErrorKind::NotFound => {} Err(error) => { diff --git a/crates/cmd-shim/src/link_bins/tests.rs b/crates/cmd-shim/src/link_bins/tests.rs index 74d1cd556..601528ddf 100644 --- a/crates/cmd-shim/src/link_bins/tests.rs +++ b/crates/cmd-shim/src/link_bins/tests.rs @@ -1,8 +1,8 @@ use super::{LinkBinsError, PackageBinSource, link_bins, link_bins_of_packages}; use crate::{ capabilities::{ - FsCreateDirAll, FsEnsureExecutableBits, FsReadDir, FsReadFile, FsReadHead, FsReadString, - FsSetExecutable, FsWalkFiles, FsWrite, RealApi, + FsCreateDirAll, FsEnsureExecutableBits, FsReadDir, FsReadFile, FsReadHead, FsReadToString, + FsSetExecutable, FsWalkFiles, FsWrite, Host, }, shim::is_shim_pointing_at, }; @@ -37,7 +37,7 @@ fn writes_all_three_shim_flavors_per_bin() { let bins_dir = tmp.path().join("node_modules/.bin"); let manifest_value: Value = serde_json::from_slice(&read_file(pkg_dir.join("package.json")).unwrap()).unwrap(); - link_bins_of_packages::( + link_bins_of_packages::( &[PackageBinSource { location: pkg_dir.clone(), manifest: Arc::new(manifest_value) }], &bins_dir, ) @@ -77,7 +77,7 @@ fn writes_shim_for_bin_string() { let bins_dir = tmp.path().join("node_modules/.bin"); let manifest_value: Value = serde_json::from_slice(&read_file(pkg_dir.join("package.json")).unwrap()).unwrap(); - link_bins_of_packages::( + link_bins_of_packages::( &[PackageBinSource { location: pkg_dir.clone(), manifest: Arc::new(manifest_value) }], &bins_dir, ) @@ -105,7 +105,7 @@ fn writes_shim_for_bin_string() { } } -/// [`link_bins::`](link_bins) walks every package and its scoped +/// [`link_bins::`](link_bins) walks every package and its scoped /// children. Both regular and `@scope/...` packages must contribute their /// bins. #[test] @@ -129,7 +129,7 @@ fn link_bins_walks_modules_and_scopes() { create_dir_all(modules.join("not-a-package")).unwrap(); let bins = modules.join(".bin"); - link_bins::(&modules, &bins).unwrap(); + link_bins::(&modules, &bins).unwrap(); assert!(bins.join("foo").exists(), "foo shim must exist"); assert!(bins.join("bar").exists(), "scoped @s/bar shim must use bare name `bar`"); @@ -142,8 +142,7 @@ fn link_bins_walks_modules_and_scopes() { fn link_bins_handles_missing_modules_dir() { let tmp = tempdir().unwrap(); let bins_dir = tmp.path().join(".bin"); - link_bins::(&tmp.path().join("missing"), &bins_dir) - .expect("missing modules dir is Ok"); + link_bins::(&tmp.path().join("missing"), &bins_dir).expect("missing modules dir is Ok"); assert!(!bins_dir.exists(), "no shims means no bin dir created"); } @@ -159,7 +158,7 @@ fn link_bins_of_packages_no_op_when_no_bins() { let bins = tmp.path().join(".bin"); let manifest: Value = serde_json::from_slice(&read_file(pkg.join("package.json")).unwrap()).unwrap(); - link_bins_of_packages::( + link_bins_of_packages::( &[PackageBinSource { location: pkg, manifest: Arc::new(manifest) }], &bins, ) @@ -198,7 +197,7 @@ fn lexical_compare_breaks_tie_when_neither_owns() { let bins = tmp.path().join(".bin"); // Order beta-then-alpha to verify the choice doesn't depend on // discovery order. - link_bins_of_packages::( + link_bins_of_packages::( &[ PackageBinSource { location: beta.clone(), manifest: Arc::new(manifest_beta) }, PackageBinSource { location: alpha.clone(), manifest: Arc::new(manifest_alpha) }, @@ -224,7 +223,7 @@ fn link_bins_propagates_parse_manifest_error() { write_file(modules.join("broken/package.json"), "{ this is not json").unwrap(); let bins = modules.join(".bin"); - let err = link_bins::(&modules, &bins).expect_err("invalid manifest must surface"); + let err = link_bins::(&modules, &bins).expect_err("invalid manifest must surface"); assert!( matches!(err, LinkBinsError::ParseManifest { .. }), "expected ParseManifest, got {err:?}", @@ -246,14 +245,14 @@ fn link_bins_skips_existing_shim_with_matching_marker() { write_file(modules.join("foo/f.js"), "#!/usr/bin/env node\n").unwrap(); let bins = modules.join(".bin"); - link_bins::(&modules, &bins).unwrap(); + link_bins::(&modules, &bins).unwrap(); let original = read_to_string(bins.join("foo")).unwrap(); // Append a sentinel. If the second pass rewrites the shim, the // sentinel disappears. let sentinel = format!("{original}\n# SENTINEL"); write_file(bins.join("foo"), &sentinel).unwrap(); - link_bins::(&modules, &bins).unwrap(); + link_bins::(&modules, &bins).unwrap(); assert_eq!(read_to_string(bins.join("foo")).unwrap(), sentinel); } @@ -273,7 +272,7 @@ fn link_bins_rewrites_when_only_sh_flavor_exists() { write_file(modules.join("foo/f.js"), "#!/usr/bin/env node\n").unwrap(); let bins = modules.join(".bin"); - link_bins::(&modules, &bins).unwrap(); + link_bins::(&modules, &bins).unwrap(); // Simulate the partial-write / older-pacquet state: delete the // .cmd and .ps1 siblings, leaving only the `.sh` shim with its @@ -281,7 +280,7 @@ fn link_bins_rewrites_when_only_sh_flavor_exists() { remove_file(bins.join("foo.cmd")).unwrap(); remove_file(bins.join("foo.ps1")).unwrap(); - link_bins::(&modules, &bins).unwrap(); + link_bins::(&modules, &bins).unwrap(); assert!(bins.join("foo").exists(), ".sh shim must remain"); assert!(bins.join("foo.cmd").exists(), ".cmd sibling must be re-created on second pass"); @@ -289,7 +288,7 @@ fn link_bins_rewrites_when_only_sh_flavor_exists() { } /// [`link_bins_of_packages`] propagates a non-`NotFound` `read_dir` -/// error from the calling context. Use a fake `Api` that fails the +/// error from the calling context. Use a fake `Sys` that fails the /// initial `create_dir_all` to cover the [`LinkBinsError::CreateBinDir`] /// error variant that real fs can't trigger portably. #[test] @@ -307,7 +306,7 @@ fn link_bins_propagates_create_bin_dir_error_via_di() { unreachable!("not called when chosen is empty") } } - impl FsReadString for FailingCreateDir { + impl FsReadToString for FailingCreateDir { fn read_to_string(_: &Path) -> io::Result { unreachable!() } @@ -376,7 +375,7 @@ fn link_bins_propagates_write_shim_error_via_di() { unreachable!() } } - impl FsReadString for FailingWrite { + impl FsReadToString for FailingWrite { fn read_to_string(_: &Path) -> io::Result { // Pretend no existing shim, forcing the writer path. Err(io::Error::from(io::ErrorKind::NotFound)) @@ -445,7 +444,7 @@ fn link_bins_propagates_chmod_error_via_di() { unreachable!() } } - impl FsReadString for FailingChmod { + impl FsReadToString for FailingChmod { fn read_to_string(_: &Path) -> io::Result { Err(io::Error::from(io::ErrorKind::NotFound)) } @@ -517,7 +516,7 @@ fn link_bins_propagates_target_chmod_error_via_di() { unreachable!() } } - impl FsReadString for FailingTargetChmod { + impl FsReadToString for FailingTargetChmod { fn read_to_string(_: &Path) -> io::Result { Err(io::Error::from(io::ErrorKind::NotFound)) } @@ -591,7 +590,7 @@ fn link_bins_swallows_target_chmod_not_found_via_di() { unreachable!() } } - impl FsReadString for NotFoundTargetChmod { + impl FsReadToString for NotFoundTargetChmod { fn read_to_string(_: &Path) -> io::Result { Err(io::Error::from(io::ErrorKind::NotFound)) } @@ -661,7 +660,7 @@ fn link_bins_propagates_probe_shim_source_error_via_di() { unreachable!() } } - impl FsReadString for FailingProbe { + impl FsReadToString for FailingProbe { fn read_to_string(_: &Path) -> io::Result { unreachable!() } @@ -730,7 +729,7 @@ fn link_bins_propagates_read_manifest_error_via_di() { Err(io::Error::from(io::ErrorKind::PermissionDenied)) } } - impl FsReadString for DenyManifestRead { + impl FsReadToString for DenyManifestRead { fn read_to_string(_: &Path) -> io::Result { unreachable!() } @@ -807,7 +806,7 @@ fn ownership_breaks_bin_conflicts_when_existing_owns() { // Order npm-first; this exercises the (true, false) arm because // `npm` (existing) owns and `aaa-other` (candidate) doesn't. let bins = tmp.path().join(".bin"); - link_bins_of_packages::( + link_bins_of_packages::( &[ PackageBinSource { location: npm.clone(), manifest: Arc::new(manifest_npm) }, PackageBinSource { location: aaa_other.clone(), manifest: Arc::new(manifest_other) }, @@ -838,7 +837,7 @@ fn link_bins_propagates_modules_dir_read_error_via_di() { unreachable!() } } - impl FsReadString for FailingModulesRead { + impl FsReadToString for FailingModulesRead { fn read_to_string(_: &Path) -> io::Result { unreachable!() } @@ -913,7 +912,7 @@ fn ownership_breaks_bin_conflicts() { serde_json::from_slice(&read_file(aaa_other.join("package.json")).unwrap()).unwrap(); let bins = tmp.path().join(".bin"); - link_bins_of_packages::( + link_bins_of_packages::( &[ PackageBinSource { location: aaa_other.clone(), manifest: Arc::new(manifest_other) }, PackageBinSource { location: npm.clone(), manifest: Arc::new(manifest_npm) }, diff --git a/crates/cmd-shim/src/shim.rs b/crates/cmd-shim/src/shim.rs index 84e9651cd..9b021b022 100644 --- a/crates/cmd-shim/src/shim.rs +++ b/crates/cmd-shim/src/shim.rs @@ -44,10 +44,10 @@ fn extension_program(extension: &str) -> Option<&'static str> { /// doesn't fail the whole install. Other IO errors propagate, since pacquet /// has already verified the bin path resolves under the package root by /// this point and a real failure deserves to surface. -pub fn search_script_runtime(path: &Path) -> io::Result> { +pub fn search_script_runtime(path: &Path) -> io::Result> { let extension = path.extension().and_then(|s| s.to_str()).unwrap_or(""); - let runtime_from_shebang = read_shebang::(path)?; + let runtime_from_shebang = read_shebang::(path)?; if let Some(rt) = runtime_from_shebang { return Ok(Some(rt)); } @@ -59,9 +59,9 @@ pub fn search_script_runtime(path: &Path) -> io::Result(path: &Path) -> io::Result> { +fn read_shebang(path: &Path) -> io::Result> { let mut buffer = [0u8; 512]; - let read = match read_head_filled::(path, &mut buffer) { + let read = match read_head_filled::(path, &mut buffer) { Ok(read) => read, Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), Err(error) => return Err(error), @@ -85,10 +85,10 @@ fn read_shebang(path: &Path) -> io::Result(path: &Path, buf: &mut [u8]) -> io::Result { +pub fn read_head_filled(path: &Path, buf: &mut [u8]) -> io::Result { let mut total = 0; while total < buf.len() { - match Api::read_head(path, total as u64, &mut buf[total..])? { + match Sys::read_head(path, total as u64, &mut buf[total..])? { 0 => break, // EOF n => total += n, } diff --git a/crates/cmd-shim/src/shim/tests.rs b/crates/cmd-shim/src/shim/tests.rs index e331dd8d9..8800ff459 100644 --- a/crates/cmd-shim/src/shim/tests.rs +++ b/crates/cmd-shim/src/shim/tests.rs @@ -3,7 +3,7 @@ use super::{ is_shim_pointing_at, parse_shebang, parse_shebang_from_bytes, read_head_filled, relative_target, search_script_runtime, }; -use crate::capabilities::{FsReadHead, RealApi}; +use crate::capabilities::{FsReadHead, Host}; use crate::path_util::lexical_normalize; use std::{ io, @@ -250,7 +250,7 @@ fn search_script_runtime_reads_shebang_from_real_file() { let tmp = tempdir().unwrap(); let path = tmp.path().join("script"); std::fs::write(&path, "#!/usr/bin/env node\nbody\n").unwrap(); - let rt = search_script_runtime::(&path).unwrap().expect("runtime detected"); + let rt = search_script_runtime::(&path).unwrap().expect("runtime detected"); assert_eq!(rt.prog.as_deref(), Some("node")); } @@ -259,7 +259,7 @@ fn search_script_runtime_reads_shebang_from_real_file() { #[test] fn search_script_runtime_returns_none_for_missing_file() { let nonexistent = Path::new("/definitely/not/a/real/path/cli"); - assert_eq!(search_script_runtime::(nonexistent).unwrap(), None); + assert_eq!(search_script_runtime::(nonexistent).unwrap(), None); } /// [`search_script_runtime`] falls through to extension lookup when the @@ -271,7 +271,7 @@ fn search_script_runtime_falls_back_to_extension() { let tmp = tempdir().unwrap(); let path = tmp.path().join("script.js"); std::fs::write(&path, "console.log('no shebang')\n").unwrap(); - let rt = search_script_runtime::(&path).unwrap().expect("extension fallback"); + let rt = search_script_runtime::(&path).unwrap().expect("extension fallback"); assert_eq!(rt.prog.as_deref(), Some("node")); } @@ -283,7 +283,7 @@ fn search_script_runtime_returns_none_when_runtime_unknown() { let tmp = tempdir().unwrap(); let path = tmp.path().join("script.unknown_ext"); std::fs::write(&path, "no shebang here\n").unwrap(); - assert_eq!(search_script_runtime::(&path).unwrap(), None); + assert_eq!(search_script_runtime::(&path).unwrap(), None); } /// [`search_script_runtime`] propagates IO errors that aren't `NotFound`. @@ -325,7 +325,7 @@ fn search_script_runtime_reads_zero_bytes_then_falls_through() { assert_eq!(rt, None); } -/// [`RealApi::read_head`](RealApi) is the production capability. Tests +/// [`Host::read_head`](Host) is the production capability. Tests /// that exercise it indirectly cover most paths; this one pins the /// contract directly. #[test] @@ -335,18 +335,18 @@ fn real_fs_read_head_reads_up_to_buffer_size() { let path = tmp.path().join("data"); std::fs::write(&path, "hello world").unwrap(); let mut buf = [0u8; 1024]; - let read = RealApi::read_head(&path, 0, &mut buf).unwrap(); + let read = Host::read_head(&path, 0, &mut buf).unwrap(); assert_eq!(read, 11); assert_eq!(&buf[..read], b"hello world"); } -/// [`RealApi::read_head`](RealApi) propagates `NotFound` so the shebang reader can +/// [`Host::read_head`](Host) propagates `NotFound` so the shebang reader can /// distinguish a missing file from a real IO error and degrade to /// `Ok(None)`. #[test] fn real_fs_read_head_propagates_not_found() { let mut buf = [0u8; 16]; - let err = RealApi::read_head(Path::new("/no/such/file"), 0, &mut buf).unwrap_err(); + let err = Host::read_head(Path::new("/no/such/file"), 0, &mut buf).unwrap_err(); assert_eq!(err.kind(), io::ErrorKind::NotFound); } @@ -362,7 +362,7 @@ fn read_head_filled_real_fs_long_file_fills_buffer() { std::fs::write(&path, &payload).unwrap(); let mut buf = [0u8; 256]; - let read = read_head_filled::(&path, &mut buf).unwrap(); + let read = read_head_filled::(&path, &mut buf).unwrap(); assert_eq!(read, 256); assert_eq!(&buf[..], &payload[..256]); } @@ -377,7 +377,7 @@ fn read_head_filled_real_fs_short_file_returns_partial() { std::fs::write(&path, "#!/bin/sh\n").unwrap(); let mut buf = [0u8; 256]; - let read = read_head_filled::(&path, &mut buf).unwrap(); + let read = read_head_filled::(&path, &mut buf).unwrap(); assert_eq!(read, 10); assert_eq!(&buf[..read], b"#!/bin/sh\n"); } diff --git a/crates/config/src/api.rs b/crates/config/src/api.rs index a58669a04..b428eced0 100644 --- a/crates/config/src/api.rs +++ b/crates/config/src/api.rs @@ -1,22 +1,14 @@ -//! Capability traits and the project-wide [`RealApi`] provider. +//! Capability traits and the project-wide [`Host`] provider. //! -//! Mirrors the dependency-injection pattern documented in -//! [pnpm/pacquet#339](https://github.com/pnpm/pacquet/issues/339): one -//! trait per capability, one provider gathering every capability impl -//! used across the codebase, all methods static. Production callers -//! turbofish the real provider explicitly -//! (e.g. `Config::current::(...)`); tests substitute a -//! per-test unit struct that implements only the bounds the function -//! actually declares, with any per-test scenario data stored in a -//! `static` inside the test fn. -//! -//! Today the provider only exposes [`EnvVar`]. As more side-effecting -//! capabilities are introduced (filesystem, disk inspection, time, -//! …) their `impl … for RealApi` blocks land here too, so callers -//! never juggle multiple providers. Trait names keep their domain -//! prefix (`Fs*`, `GetDisk*`, `Env*`, …) so a reader can identify -//! which domain a generic bound belongs to without chasing -//! definitions. +//! Mirrors the dependency-injection pattern documented in the +//! "Dependency injection for tests" section of `CODE_STYLE_GUIDE.md`: +//! one trait per capability, one provider gathering every capability +//! impl used across the codebase, all methods static. Production +//! callers turbofish the real provider explicitly +//! (e.g. `Config::current::(...)`); tests substitute a per-test +//! unit struct that implements only the bounds the function actually +//! declares, with any per-test scenario data stored in a `static` +//! inside the test fn. /// Capability: read a process environment variable. /// @@ -35,17 +27,17 @@ pub trait EnvVar { } /// Project-wide capability provider. Production code threads -/// `RealApi` through generic call sites with an explicit turbofish: +/// `Host` through generic call sites with an explicit turbofish: /// /// ```ignore -/// let config = Config::current::(env::current_dir, home::home_dir, Default::default); +/// let config = Config::current::(env::current_dir, home::home_dir, Default::default); /// ``` /// /// Tests substitute their own zero-sized struct that implements only /// the trait bounds the function under test declares. -pub struct RealApi; +pub struct Host; -impl EnvVar for RealApi { +impl EnvVar for Host { fn var(name: &str) -> Option { std::env::var(name).ok() } diff --git a/crates/config/src/env_replace.rs b/crates/config/src/env_replace.rs index 8338e105f..7a74a0116 100644 --- a/crates/config/src/env_replace.rs +++ b/crates/config/src/env_replace.rs @@ -21,7 +21,7 @@ //! pnpm's behaviour even though plain shell `${VAR:-default}` would //! also use the default for the empty case. //! -//! Production callers thread `RealApi` (which delegates to +//! Production callers thread `Host` (which delegates to //! `std::env::var`) through the turbofish slot. Tests provide their //! own per-test unit struct, per the DI pattern from //! [pnpm/pacquet#339](https://github.com/pnpm/pacquet/issues/339). @@ -51,12 +51,12 @@ impl fmt::Display for EnvReplaceError { impl std::error::Error for EnvReplaceError {} /// Replace every `${VAR}` (or `${VAR:-default}`) placeholder in `text` -/// with the value [`Api::var`] returns. Returns an error on the first +/// with the value [`Sys::var`] returns. Returns an error on the first /// unresolvable placeholder so the caller can warn and skip the line, /// matching pnpm's `substituteEnv`. /// -/// [`Api::var`]: EnvVar::var -pub(crate) fn env_replace(text: &str) -> Result { +/// [`Sys::var`]: EnvVar::var +pub(crate) fn env_replace(text: &str) -> Result { let bytes = text.as_bytes(); let mut output = String::with_capacity(text.len()); let mut index = 0; @@ -103,7 +103,7 @@ pub(crate) fn env_replace(text: &str) -> Result (&inside[..separator], Some(&inside[separator + 2..])), None => (inside, None), }; - let value = Api::var(var_name).filter(|value| !value.is_empty()); + let value = Sys::var(var_name).filter(|value| !value.is_empty()); match (value, default) { (Some(value), _) => output.push_str(&value), (None, Some(default)) => output.push_str(default), diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 73b3f958f..e254259f2 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -5,7 +5,7 @@ pub mod matcher; mod npmrc_auth; mod workspace_yaml; -pub use crate::api::{EnvVar, RealApi}; +pub use crate::api::{EnvVar, Host}; use indexmap::IndexMap; use pacquet_patching::{PatchGroupRecord, ResolvePatchedDependenciesError, resolve_and_group}; @@ -708,13 +708,13 @@ impl Config { /// `pnpm-workspace.yaml` cannot be read or parsed, matching pnpm's /// [`readWorkspaceManifest`](https://github.com/pnpm/pnpm/blob/8eb1be4988/workspace/workspace-manifest-reader/src/index.ts). /// A missing file is not an error. - pub fn current( + pub fn current( current_dir: CurrentDir, home_dir: HomeDir, default: Default, ) -> Result where - Api: EnvVar, + Sys: EnvVar, CurrentDir: FnOnce() -> Result, HomeDir: FnOnce() -> Option, Default: FnOnce() -> Config, @@ -752,7 +752,7 @@ impl Config { .and_then(|dir| read_npmrc(dir)) .or_else(|| home_dir().and_then(|dir| read_npmrc(&dir))); let mut npmrc_auth = auth_source - .map(|text| crate::npmrc_auth::NpmrcAuth::from_ini::(&text)) + .map(|text| crate::npmrc_auth::NpmrcAuth::from_ini::(&text)) .unwrap_or_default(); npmrc_auth.apply_registry_and_warn(&mut config); // Proxy cascade fires unconditionally — even when no `.npmrc` @@ -760,7 +760,7 @@ impl Config { // [`config/reader/src/index.ts:591-600`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/index.ts#L591-L600) // is a normalization step on the resolved config, not a // function of `.npmrc` presence. - npmrc_auth.apply_proxy_cascade::(&mut config); + npmrc_auth.apply_proxy_cascade::(&mut config); // TLS + local-address are sourced from `.npmrc` only — pnpm // does not honor env vars (`NODE_EXTRA_CA_CERTS`, // `NODE_TLS_REJECT_UNAUTHORIZED`, etc.) for these keys @@ -893,7 +893,7 @@ mod tests { use pretty_assertions::assert_eq; use tempfile::tempdir; - use super::{Config, NodeLinker, PackageImportMethod, RealApi, fs}; + use super::{Config, Host, NodeLinker, PackageImportMethod, fs}; use crate::defaults::default_store_dir; use pacquet_store_dir::StoreDir; use pacquet_testing_utils::env_guard::EnvGuard; @@ -965,7 +965,7 @@ mod tests { let tmp = tempdir().unwrap(); fs::write(tmp.path().join(".npmrc"), "registry=https://cwd.example") .expect("write to .npmrc"); - let config = Config::current::( + let config = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || unreachable!("shouldn't reach home dir"), Config::new, @@ -983,7 +983,7 @@ mod tests { let non_auth_ini = "symlink=false\nlockfile=true\nhoist=false\nnode-linker=hoisted\n"; fs::write(tmp.path().join(".npmrc"), non_auth_ini).expect("write to .npmrc"); let defaults = Config::new(); - let config = Config::current::( + let config = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || None, Config::new, @@ -1007,7 +1007,7 @@ mod tests { let ini = "fetch-retries=99\nfetch-retry-factor=99\nfetch-retry-mintimeout=99\nfetch-retry-maxtimeout=99\n"; fs::write(tmp.path().join(".npmrc"), ini).expect("write to .npmrc"); let defaults = Config::new(); - let config = Config::current::( + let config = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || None, Config::new, @@ -1024,7 +1024,7 @@ mod tests { let tmp = tempdir().unwrap(); // write invalid utf-8 value to npmrc fs::write(tmp.path().join(".npmrc"), b"Hello \xff World").expect("write to .npmrc"); - let config = Config::current::( + let config = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || None, Config::new, @@ -1039,7 +1039,7 @@ mod tests { let home_dir = tempdir().unwrap(); fs::write(home_dir.path().join(".npmrc"), "registry=https://home.example") .expect("write to .npmrc"); - let config = Config::current::( + let config = Config::current::( || current_dir.path().to_path_buf().pipe(Ok::<_, ()>), || home_dir.path().to_path_buf().pipe(Some), Config::new, @@ -1058,7 +1058,7 @@ mod tests { .expect("write to .npmrc"); fs::write(tmp.path().join("pnpm-workspace.yaml"), "registry: https://from-yaml.test\n") .expect("write to pnpm-workspace.yaml"); - let config = Config::current::( + let config = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || unreachable!("shouldn't reach home dir"), Config::new, @@ -1076,7 +1076,7 @@ mod tests { .expect("write to pnpm-workspace.yaml"); // No `.npmrc` anywhere, but a parent dir has `pnpm-workspace.yaml` — // the yaml should still be applied. - let config = Config::current::( + let config = Config::current::( || nested.clone().pipe(Ok::<_, ()>), || None, Config::new, @@ -1089,7 +1089,7 @@ mod tests { pub fn test_current_folder_fallback_to_default() { let current_dir = tempdir().unwrap(); let home_dir = tempdir().unwrap(); - let config = Config::current::( + let config = Config::current::( || current_dir.path().to_path_buf().pipe(Ok::<_, ()>), || home_dir.path().to_path_buf().pipe(Some), || Config { symlink: false, ..Config::new() }, @@ -1114,7 +1114,7 @@ mod tests { #[test] pub fn gvs_default_is_off_and_paths_derive_cleanly() { let tmp = tempdir().unwrap(); - let config = Config::current::( + let config = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || None, Config::new, @@ -1142,7 +1142,7 @@ mod tests { let tmp = tempdir().unwrap(); fs::write(tmp.path().join("pnpm-workspace.yaml"), "enableGlobalVirtualStore: false\n") .expect("write to pnpm-workspace.yaml"); - let config = Config::current::( + let config = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || None, Config::new, @@ -1167,7 +1167,7 @@ mod tests { format!("enableGlobalVirtualStore: true\nvirtualStoreDir: {}\n", user_path.display()), ) .expect("write to pnpm-workspace.yaml"); - let config = Config::current::( + let config = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || None, Config::new, @@ -1205,7 +1205,7 @@ mod tests { ), ) .expect("write to pnpm-workspace.yaml"); - let config = Config::current::( + let config = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || None, Config::new, @@ -1225,7 +1225,7 @@ mod tests { /// fallback through `Config::current`. The injected-`EnvVar` tests /// in `npmrc_auth/tests.rs` cover the cascade branches /// exhaustively; this one only proves the wiring through - /// `RealApi::var` reaches `std::env::var` and that the cascade + /// `Host::var` reaches `std::env::var` and that the cascade /// fires even with no `.npmrc` present. #[test] pub fn proxy_env_fallback_applies_through_current() { @@ -1261,7 +1261,7 @@ mod tests { env::remove_var("npm_config_workspace_dir"); env::set_var("HTTPS_PROXY", "http://env.example:8080"); } - let config = Config::current::( + let config = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || None, Config::new, @@ -1285,7 +1285,7 @@ mod tests { // `: : :` is rejected by saphyr. fs::write(tmp.path().join("pnpm-workspace.yaml"), ": : :\n") .expect("write to pnpm-workspace.yaml"); - let result = Config::current::( + let result = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || None, Config::new, @@ -1314,7 +1314,7 @@ mod tests { fs::write(workspace_root.join("pnpm-workspace.yaml"), "packages:\n - packages/*\n") .expect("write to pnpm-workspace.yaml"); - let config = Config::current::( + let config = Config::current::( || subdir.clone().pipe(Ok::<_, ()>), || None, Config::new, @@ -1351,7 +1351,7 @@ mod tests { env::remove_var("npm_config_workspace_dir"); } let tmp = tempdir().unwrap(); - let config = Config::current::( + let config = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || None, Config::new, @@ -1392,7 +1392,7 @@ mod tests { env::set_var("NPM_CONFIG_WORKSPACE_DIR", env_workspace.path()); } - let config = Config::current::( + let config = Config::current::( || cwd_dir.path().to_path_buf().pipe(Ok::<_, ()>), || None, Config::new, @@ -1428,7 +1428,7 @@ mod tests { env::set_var("npm_config_workspace_dir", ""); } let tmp = tempdir().unwrap(); - let config = Config::current::( + let config = Config::current::( || tmp.path().to_path_buf().pipe(Ok::<_, ()>), || None, Config::new, diff --git a/crates/config/src/npmrc_auth.rs b/crates/config/src/npmrc_auth.rs index d6285d3a6..04187d030 100644 --- a/crates/config/src/npmrc_auth.rs +++ b/crates/config/src/npmrc_auth.rs @@ -138,7 +138,7 @@ impl NpmrcAuth { /// plus comments starting with `;` or `#`. We hand-parse rather than /// use a strongly-typed deserializer so unknown / malformed keys don't /// blow up parsing. - pub fn from_ini(text: &str) -> Self { + pub fn from_ini(text: &str) -> Self { let mut auth = NpmrcAuth::default(); for line in text.lines() { let line = line.trim(); @@ -153,14 +153,14 @@ impl NpmrcAuth { // Apply ${VAR} substitution to both the key and the value, // matching `readAndFilterNpmrc` in pnpm's `loadNpmrcFiles.ts`. - let key = match env_replace::(raw_key) { + let key = match env_replace::(raw_key) { Ok(value) => value, Err(error) => { auth.warnings.push(error.to_string()); raw_key.to_owned() } }; - let value = match env_replace::(raw_value) { + let value = match env_replace::(raw_value) { Ok(value) => value, Err(error) => { auth.warnings.push(error.to_string()); @@ -311,31 +311,31 @@ impl NpmrcAuth { /// Generic over [`EnvVar`] so cascade tests can drive every branch /// without mutating the process environment (no `EnvGuard` global /// lock). - pub fn apply_proxy_cascade(&mut self, config: &mut Config) { + pub fn apply_proxy_cascade(&mut self, config: &mut Config) { // Upstream's `getProcessEnv` tries literal-, upper-, and // lower-case in order (config/reader/src/index.ts:689-693). For // the proxy var names below the literal form is already either // fully upper or fully lower, so the triple collapses to two // real attempts. - fn env_pair(upper: &str, lower: &str) -> Option { - Api::var(upper).or_else(|| Api::var(lower)) + fn env_pair(upper: &str, lower: &str) -> Option { + Sys::var(upper).or_else(|| Sys::var(lower)) } config.proxy.https_proxy = self .https_proxy .take() .or_else(|| self.legacy_proxy.clone()) - .or_else(|| env_pair::("HTTPS_PROXY", "https_proxy")); + .or_else(|| env_pair::("HTTPS_PROXY", "https_proxy")); config.proxy.http_proxy = self .http_proxy .take() .or_else(|| config.proxy.https_proxy.clone()) - .or_else(|| env_pair::("HTTP_PROXY", "http_proxy")) - .or_else(|| env_pair::("PROXY", "proxy")); + .or_else(|| env_pair::("HTTP_PROXY", "http_proxy")) + .or_else(|| env_pair::("PROXY", "proxy")); config.proxy.no_proxy = self .no_proxy .take() - .or_else(|| env_pair::("NO_PROXY", "no_proxy")) + .or_else(|| env_pair::("NO_PROXY", "no_proxy")) .map(|raw| parse_no_proxy(&raw)); } @@ -394,9 +394,9 @@ impl NpmrcAuth { /// [`apply_proxy_cascade`]: NpmrcAuth::apply_proxy_cascade /// [`build_auth_headers`]: NpmrcAuth::build_auth_headers #[cfg(test)] - pub fn apply_to(mut self, config: &mut Config) { + pub fn apply_to(mut self, config: &mut Config) { self.apply_registry_and_warn(config); - self.apply_proxy_cascade::(config); + self.apply_proxy_cascade::(config); self.apply_tls_and_local_address(config); self.build_auth_headers(config); } diff --git a/crates/modules-yaml/src/lib.rs b/crates/modules-yaml/src/lib.rs index f67a737b8..665e16389 100644 --- a/crates/modules-yaml/src/lib.rs +++ b/crates/modules-yaml/src/lib.rs @@ -37,9 +37,8 @@ pub const DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH: u64 = 120; /// /// One trait per filesystem capability so each function declares only what /// it actually uses, and so test fakes only implement the methods that -/// will be exercised. Pattern follows the per-capability typeclass style -/// rather than `parallel-disk-usage`'s lumped `FsApi` at -/// . +/// will be exercised. See the "Dependency injection for tests" section +/// of `CODE_STYLE_GUIDE.md` for the full set of principles. pub trait FsReadToString { fn read_to_string(path: &Path) -> io::Result; } @@ -65,30 +64,30 @@ pub trait Clock { } /// Production implementation, backed by [`std::fs`] and [`SystemTime::now`]. -pub struct RealApi; +pub struct Host; -impl FsReadToString for RealApi { +impl FsReadToString for Host { #[inline] fn read_to_string(path: &Path) -> io::Result { fs::read_to_string(path) } } -impl FsCreateDirAll for RealApi { +impl FsCreateDirAll for Host { #[inline] fn create_dir_all(path: &Path) -> io::Result<()> { fs::create_dir_all(path) } } -impl FsWrite for RealApi { +impl FsWrite for Host { #[inline] fn write(path: &Path, contents: &[u8]) -> io::Result<()> { fs::write(path, contents) } } -impl Clock for RealApi { +impl Clock for Host { #[inline] fn now() -> SystemTime { SystemTime::now() @@ -362,16 +361,16 @@ pub enum WriteModulesError { /// `null` document, matching upstream `readModules` at /// . /// -/// Production callers turbofish [`RealApi`]: `read_modules_manifest::(dir)`. +/// Production callers turbofish [`Host`]: `read_modules_manifest::(dir)`. /// The bounds list the minimal capabilities ([`FsReadToString`] + /// [`Clock`]) so test fakes only need to implement the methods that are /// actually called. -pub fn read_modules_manifest(modules_dir: &Path) -> Result, ReadModulesError> +pub fn read_modules_manifest(modules_dir: &Path) -> Result, ReadModulesError> where - Api: FsReadToString + Clock, + Sys: FsReadToString + Clock, { let manifest_path = modules_dir.join(MODULES_FILENAME); - let content = match Api::read_to_string(&manifest_path) { + let content = match Sys::read_to_string(&manifest_path) { Ok(content) => content, Err(source) if source.kind() == io::ErrorKind::NotFound => return Ok(None), Err(source) => { @@ -386,7 +385,7 @@ where apply_legacy_shamefully_hoist(&mut manifest); resolve_virtual_store_dir(&mut manifest, modules_dir); if manifest.pruned_at.is_empty() { - manifest.pruned_at = httpdate::fmt_http_date(Api::now()); + manifest.pruned_at = httpdate::fmt_http_date(Sys::now()); } if manifest.virtual_store_dir_max_length == 0 { manifest.virtual_store_dir_max_length = DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH; @@ -407,14 +406,14 @@ where /// `clone()` inside the function. Per the CODE_STYLE_GUIDE rule that /// owned-vs-borrowed parameter choice should minimize copies. /// -/// Production callers turbofish [`RealApi`]: `write_modules_manifest::(dir, m)`. +/// Production callers turbofish [`Host`]: `write_modules_manifest::(dir, m)`. /// Bounds are minimal: only [`FsCreateDirAll`] and [`FsWrite`] are required. -pub fn write_modules_manifest( +pub fn write_modules_manifest( modules_dir: &Path, mut manifest: Modules, ) -> Result<(), WriteModulesError> where - Api: FsCreateDirAll + FsWrite, + Sys: FsCreateDirAll + FsWrite, { manifest.skipped.sort(); drop_legacy_hoisted_aliases_when_unreferenced(&mut manifest); @@ -426,12 +425,12 @@ where } let serialized = serde_json::to_string_pretty(&manifest).map_err(WriteModulesError::SerializeJson)?; - Api::create_dir_all(modules_dir).map_err(|source| WriteModulesError::CreateDir { + Sys::create_dir_all(modules_dir).map_err(|source| WriteModulesError::CreateDir { path: modules_dir.to_path_buf(), source, })?; let manifest_path = modules_dir.join(MODULES_FILENAME); - Api::write(&manifest_path, serialized.as_bytes()) + Sys::write(&manifest_path, serialized.as_bytes()) .map_err(|source| WriteModulesError::WriteFile { path: manifest_path, source }) } diff --git a/crates/modules-yaml/tests/fakes.rs b/crates/modules-yaml/tests/fakes.rs index 6142f4821..09b6c86a1 100644 --- a/crates/modules-yaml/tests/fakes.rs +++ b/crates/modules-yaml/tests/fakes.rs @@ -2,9 +2,8 @@ //! and time-dependent branches that are awkward or impossible to provoke //! with the real filesystem. Each fake implements only the capability //! trait the function under test consumes, so a read fake never has to -//! declare `write`. This is the interface-segregation refinement of the -//! lumped `FsApi` pattern at -//! . +//! declare `write`. See the "Dependency injection for tests" section +//! of `CODE_STYLE_GUIDE.md` for the full set of principles. use chrono::{TimeZone, Utc}; use pacquet_modules_yaml::{ diff --git a/crates/modules-yaml/tests/index.rs b/crates/modules-yaml/tests/index.rs index a65b647f9..4a7a49365 100644 --- a/crates/modules-yaml/tests/index.rs +++ b/crates/modules-yaml/tests/index.rs @@ -5,7 +5,7 @@ //! sibling files (`real_fs.rs`, `fakes.rs`). use pacquet_modules_yaml::{ - HoistKind, Modules, RealApi, read_modules_manifest, write_modules_manifest, + HoistKind, Host, Modules, read_modules_manifest, write_modules_manifest, }; use pipe_trait::Pipe; use pretty_assertions::assert_eq; @@ -44,8 +44,8 @@ fn write_modules_manifest_and_read_modules_manifest() { "virtualStoreDirMaxLength": 120, })); - write_modules_manifest::(modules_dir, modules_yaml.clone()).expect("write manifest"); - let actual = read_modules_manifest::(modules_dir).expect("read manifest"); + write_modules_manifest::(modules_dir, modules_yaml.clone()).expect("write manifest"); + let actual = read_modules_manifest::(modules_dir).expect("read manifest"); assert_eq!(actual, Some(modules_yaml)); let raw: Value = modules_dir @@ -69,7 +69,7 @@ fn read_legacy_shamefully_hoist_true_manifest() { let manifest = env!("CARGO_MANIFEST_DIR") .pipe(Path::new) .join("tests/fixtures/old-shamefully-hoist") - .pipe_as_ref(read_modules_manifest::) + .pipe_as_ref(read_modules_manifest::) .expect("read manifest") .expect("modules manifest exists"); @@ -99,7 +99,7 @@ fn read_legacy_shamefully_hoist_false_manifest() { let manifest = env!("CARGO_MANIFEST_DIR") .pipe(Path::new) .join("tests/fixtures/old-no-shamefully-hoist") - .pipe_as_ref(read_modules_manifest::) + .pipe_as_ref(read_modules_manifest::) .expect("read manifest") .expect("modules manifest exists"); @@ -151,8 +151,8 @@ fn write_modules_manifest_creates_node_modules_directory() { "virtualStoreDirMaxLength": 120, })); - write_modules_manifest::(&modules_dir, modules_yaml.clone()).expect("write manifest"); - let actual = read_modules_manifest::(&modules_dir).expect("read manifest"); + write_modules_manifest::(&modules_dir, modules_yaml.clone()).expect("write manifest"); + let actual = read_modules_manifest::(&modules_dir).expect("read manifest"); assert_eq!(actual, Some(modules_yaml)); } @@ -162,7 +162,7 @@ fn read_empty_modules_manifest_returns_none() { let modules_yaml = env!("CARGO_MANIFEST_DIR") .pipe(Path::new) .join("tests/fixtures/empty-modules-yaml") - .pipe_as_ref(read_modules_manifest::) + .pipe_as_ref(read_modules_manifest::) .expect("read manifest"); assert_eq!(modules_yaml, None); } diff --git a/crates/modules-yaml/tests/real_fs.rs b/crates/modules-yaml/tests/real_fs.rs index 1b2b8ccb9..64a63021a 100644 --- a/crates/modules-yaml/tests/real_fs.rs +++ b/crates/modules-yaml/tests/real_fs.rs @@ -8,9 +8,7 @@ //! ported, so these direct unit tests guard the behavior in the meantime. use indexmap::IndexSet; -use pacquet_modules_yaml::{ - DepPath, Modules, RealApi, read_modules_manifest, write_modules_manifest, -}; +use pacquet_modules_yaml::{DepPath, Host, Modules, read_modules_manifest, write_modules_manifest}; use pipe_trait::Pipe; use pretty_assertions::assert_eq; use serde_json::{Value, json}; @@ -33,7 +31,7 @@ fn read_preserves_absolute_virtual_store_dir() { fs::write(modules_dir.join(".modules.yaml"), raw).expect("write fixture"); let manifest = modules_dir - .pipe_as_ref(read_modules_manifest::) + .pipe_as_ref(read_modules_manifest::) .expect("read manifest") .expect("manifest exists"); assert_eq!(Path::new(&manifest.virtual_store_dir), custom_store); @@ -55,7 +53,7 @@ fn write_relativizes_non_descendant_virtual_store_dir() { "virtualStoreDir": &sibling_store, })); - write_modules_manifest::(&modules_dir, manifest).expect("write manifest"); + write_modules_manifest::(&modules_dir, manifest).expect("write manifest"); let raw: Value = modules_dir .join(".modules.yaml") .pipe(fs::read_to_string) @@ -77,7 +75,7 @@ fn write_sorts_skipped_array() { "skipped": ["zeta", "alpha", "mu"], })); - write_modules_manifest::(modules_dir, manifest).expect("write manifest"); + write_modules_manifest::(modules_dir, manifest).expect("write manifest"); let raw: Value = modules_dir .join(".modules.yaml") .pipe(fs::read_to_string) @@ -99,7 +97,7 @@ fn write_removes_null_public_hoist_pattern() { "publicHoistPattern": null, })); - write_modules_manifest::(modules_dir, manifest).expect("write manifest"); + write_modules_manifest::(modules_dir, manifest).expect("write manifest"); let raw: Value = modules_dir .join(".modules.yaml") .pipe(fs::read_to_string) @@ -136,7 +134,7 @@ fn dep_path_serializes_transparently() { [DepPath::from("/sharp/0.32.0".to_string())].into_iter().collect(); assert_eq!(manifest.ignored_builds.as_ref(), Some(&expected_ignored)); - write_modules_manifest::(modules_dir, manifest).expect("write manifest"); + write_modules_manifest::(modules_dir, manifest).expect("write manifest"); let raw: Value = modules_dir .join(".modules.yaml") .pipe(fs::read_to_string) @@ -181,8 +179,8 @@ fn hoisted_locations_round_trips() { Some(&vec!["node_modules/accepts".to_string()]), ); - write_modules_manifest::(modules_dir, manifest.clone()).expect("write manifest"); - let actual = read_modules_manifest::(modules_dir) + write_modules_manifest::(modules_dir, manifest.clone()).expect("write manifest"); + let actual = read_modules_manifest::(modules_dir) .expect("read manifest") .expect("manifest exists"); assert_eq!(actual.hoisted_locations, manifest.hoisted_locations); @@ -211,7 +209,7 @@ fn absent_hoisted_locations_is_omitted_on_write() { let manifest = manifest_from_json(json!({ "layoutVersion": 5 })); assert!(manifest.hoisted_locations.is_none(), "fixture seed"); - write_modules_manifest::(modules_dir, manifest).expect("write manifest"); + write_modules_manifest::(modules_dir, manifest).expect("write manifest"); let raw: Value = modules_dir .join(".modules.yaml") .pipe(fs::read_to_string) diff --git a/crates/package-manager/src/install.rs b/crates/package-manager/src/install.rs index 764ceb5dc..9d6d29362 100644 --- a/crates/package-manager/src/install.rs +++ b/crates/package-manager/src/install.rs @@ -11,8 +11,8 @@ use pacquet_lockfile::{ LoadLockfileError, Lockfile, SaveLockfileError, StalenessReason, satisfies_package_manifest, }; use pacquet_modules_yaml::{ - DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH, IncludedDependencies, LayoutVersion, Modules, - NodeLinker as ModulesNodeLinker, RealApi, WriteModulesError, write_modules_manifest, + DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH, Host, IncludedDependencies, LayoutVersion, Modules, + NodeLinker as ModulesNodeLinker, WriteModulesError, write_modules_manifest, }; use pacquet_network::ThrottledClient; use pacquet_package_manifest::{DependencyGroup, PackageManifest}; @@ -457,7 +457,7 @@ where // directory layout, hoist patterns, included dependency groups, // store dir, and registries so a later install (or another // tool) can detect a layout change and prune accordingly. - write_modules_manifest::( + write_modules_manifest::( &config.modules_dir, build_modules_manifest( config, diff --git a/crates/package-manager/src/install/tests.rs b/crates/package-manager/src/install/tests.rs index c2b3c1e01..6f3e1d797 100644 --- a/crates/package-manager/src/install/tests.rs +++ b/crates/package-manager/src/install/tests.rs @@ -2,7 +2,7 @@ use super::{Install, InstallError}; use pacquet_config::Config; use pacquet_lockfile::Lockfile; use pacquet_modules_yaml::{ - DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH, LayoutVersion, Modules, NodeLinker, RealApi, + DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH, Host, LayoutVersion, Modules, NodeLinker, read_modules_manifest, write_modules_manifest, }; use pacquet_package_manifest::{DependencyGroup, PackageManifest}; @@ -653,7 +653,7 @@ async fn install_writes_modules_yaml() { package_manager, .. } = modules_dir - .pipe_as_ref(read_modules_manifest::) + .pipe_as_ref(read_modules_manifest::) .expect("read .modules.yaml") .expect("modules manifest exists"); @@ -1709,7 +1709,7 @@ async fn frozen_install_preserves_seeded_skipped_across_reinstall() { skipped: seeded_keys.iter().map(|s| (*s).to_string()).collect(), ..Default::default() }; - write_modules_manifest::(&modules_dir, seed_modules).expect("seed .modules.yaml"); + write_modules_manifest::(&modules_dir, seed_modules).expect("seed .modules.yaml"); // Empty lockfile drives the constraint-free fast path. The // seed must survive verbatim. @@ -1741,7 +1741,7 @@ async fn frozen_install_preserves_seeded_skipped_across_reinstall() { .expect("frozen-lockfile install should succeed"); let written = modules_dir - .pipe_as_ref(read_modules_manifest::) + .pipe_as_ref(read_modules_manifest::) .expect("read .modules.yaml") .expect("modules manifest exists"); @@ -1864,7 +1864,7 @@ async fn frozen_install_silently_swallows_unreachable_optional_tarball() { // that never updates `opts.skipped`, so a future install retries // the fetch (in case the URL becomes reachable again). let written = modules_dir - .pipe_as_ref(read_modules_manifest::) + .pipe_as_ref(read_modules_manifest::) .expect("read .modules.yaml") .expect("modules manifest exists"); assert!( @@ -2055,7 +2055,7 @@ async fn frozen_install_no_optional_drops_optional_only_snapshots() { // Transient — must not bleed into the persistent // `.modules.yaml.skipped` set. let written = modules_dir - .pipe_as_ref(read_modules_manifest::) + .pipe_as_ref(read_modules_manifest::) .expect("read .modules.yaml") .expect("modules manifest exists"); assert!( @@ -2304,7 +2304,7 @@ async fn hoisted_node_linker_empty_lockfile_writes_modules_yaml() { .expect("hoisted-linker install with empty lockfile should succeed"); let written = modules_dir - .pipe_as_ref(read_modules_manifest::) + .pipe_as_ref(read_modules_manifest::) .expect("read .modules.yaml") .expect("modules manifest exists"); diff --git a/crates/package-manager/src/install_frozen_lockfile.rs b/crates/package-manager/src/install_frozen_lockfile.rs index b000d4eca..9251f73bc 100644 --- a/crates/package-manager/src/install_frozen_lockfile.rs +++ b/crates/package-manager/src/install_frozen_lockfile.rs @@ -15,7 +15,7 @@ use pacquet_cmd_shim::LinkBinsError; use pacquet_config::{Config, NodeLinker, matcher::create_matcher}; use pacquet_executor::ScriptsPrependNodePath as ExecScriptsPrependNodePath; use pacquet_lockfile::{Lockfile, PackageKey, PackageMetadata, ProjectSnapshot, SnapshotEntry}; -use pacquet_modules_yaml::{RealApi, read_modules_manifest}; +use pacquet_modules_yaml::{Host, read_modules_manifest}; use pacquet_network::ThrottledClient; use pacquet_package_manifest::DependencyGroup; use pacquet_patching::{ @@ -323,7 +323,7 @@ where // A read error (corrupt yaml, permissions) is degraded to // an empty seed — `.modules.yaml` is a cache artifact, not // an authoritative source. Missing file → empty seed. - let seed = match read_modules_manifest::(&config.modules_dir) { + let seed = match read_modules_manifest::(&config.modules_dir) { Ok(Some(manifest)) => SkippedSnapshots::from_strings(&manifest.skipped), Ok(None) => SkippedSnapshots::new(), Err(error) => { diff --git a/crates/package-manager/src/install_without_lockfile.rs b/crates/package-manager/src/install_without_lockfile.rs index 992abf13d..fad569a6f 100644 --- a/crates/package-manager/src/install_without_lockfile.rs +++ b/crates/package-manager/src/install_without_lockfile.rs @@ -7,7 +7,7 @@ use dashmap::DashSet; use derive_more::{Display, Error}; use futures_util::future; use miette::Diagnostic; -use pacquet_cmd_shim::{LinkBinsError, RealApi, link_bins}; +use pacquet_cmd_shim::{Host, LinkBinsError, link_bins}; use pacquet_config::Config; use pacquet_network::ThrottledClient; use pacquet_package_manifest::{DependencyGroup, PackageManifest}; @@ -207,7 +207,7 @@ impl<'a, DependencyGroupList> InstallWithoutLockfile<'a, DependencyGroupList> { // iterator was already consumed by the install loop above; pnpm's // own `linkBins(modulesDir, binsDir)` overload uses the same // strategy. - link_bins::(&config.modules_dir, &config.modules_dir.join(".bin")) + link_bins::(&config.modules_dir, &config.modules_dir.join(".bin")) .map_err(InstallWithoutLockfileError::LinkBins)?; // No lockfile here, so no prefetched manifests are available — diff --git a/crates/package-manager/src/link_bins.rs b/crates/package-manager/src/link_bins.rs index d7a06eae2..05570cdf7 100644 --- a/crates/package-manager/src/link_bins.rs +++ b/crates/package-manager/src/link_bins.rs @@ -2,8 +2,8 @@ use crate::{PackageManifests, SkippedSnapshots}; use derive_more::{Display, Error}; use miette::Diagnostic; use pacquet_cmd_shim::{ - FsCreateDirAll, FsEnsureExecutableBits, FsReadDir, FsReadFile, FsReadHead, FsReadString, - FsSetExecutable, FsWalkFiles, FsWrite, LinkBinsError, PackageBinSource, RealApi, + FsCreateDirAll, FsEnsureExecutableBits, FsReadDir, FsReadFile, FsReadHead, FsReadToString, + FsSetExecutable, FsWalkFiles, FsWrite, Host, LinkBinsError, PackageBinSource, link_bins_of_packages, }; use pacquet_lockfile::{LockfileResolution, PackageKey, PackageMetadata, PkgName, SnapshotEntry}; @@ -61,7 +61,7 @@ pub fn link_direct_dep_bins(modules_dir: &Path, dep_names: &[String]) -> Result< if bin_sources.is_empty() { return Ok(()); } - link_bins_of_packages::(&bin_sources, &modules_dir.join(".bin")) + link_bins_of_packages::(&bin_sources, &modules_dir.join(".bin")) } /// Error type of [`LinkVirtualStoreBins`]. @@ -154,19 +154,19 @@ pub struct LinkVirtualStoreBins<'a> { impl<'a> LinkVirtualStoreBins<'a> { pub fn run(self) -> Result<(), LinkVirtualStoreBinsError> { - self.run_with::() + self.run_with::() } /// DI-driven entry. Production callers go through [`Self::run`] which - /// turbofishes [`RealApi`]; tests inject fakes that fail specific fs + /// turbofishes [`Host`]; tests inject fakes that fail specific fs /// operations to cover error paths the real fs can't trigger /// portably. See the per-capability DI pattern at /// . - pub fn run_with(self) -> Result<(), LinkVirtualStoreBinsError> + pub fn run_with(self) -> Result<(), LinkVirtualStoreBinsError> where - Api: FsReadDir + Sys: FsReadDir + FsReadFile - + FsReadString + + FsReadToString + FsReadHead + FsCreateDirAll + FsWalkFiles @@ -177,7 +177,7 @@ impl<'a> LinkVirtualStoreBins<'a> { let LinkVirtualStoreBins { layout, snapshots, packages, package_manifests, skipped } = self; if let Some(snapshots) = snapshots { let has_bin_set = build_has_bin_set(packages); - run_lockfile_driven::( + run_lockfile_driven::( layout, snapshots, has_bin_set.as_ref(), @@ -190,7 +190,7 @@ impl<'a> LinkVirtualStoreBins<'a> { // frozen installs, which #432 doesn't activate GVS for, so // reading from `layout.package_store_dir()` reproduces // today's behaviour exactly when GVS is off. - run_with_readdir::(layout.package_store_dir()) + run_with_readdir::(layout.package_store_dir()) } } } @@ -255,7 +255,7 @@ fn build_has_bin_set( /// cold-batch packages that prefetch missed, a fallback /// `package.json` read through the existing symlink at /// `/node_modules/`. -fn run_lockfile_driven( +fn run_lockfile_driven( layout: &crate::VirtualStoreLayout, snapshots: &HashMap, has_bin_set: Option<&HashSet>, @@ -263,8 +263,8 @@ fn run_lockfile_driven( skipped: &SkippedSnapshots, ) -> Result<(), LinkVirtualStoreBinsError> where - Api: FsReadFile - + FsReadString + Sys: FsReadFile + + FsReadToString + FsReadHead + FsCreateDirAll + FsWalkFiles @@ -352,7 +352,7 @@ where // prefetched manifest map yet. Reading from disk // here is the same code path as the non-lockfile // install — see [`run_with_readdir`]. - match read_package::(&child_location) { + match read_package::(&child_location) { Ok(Some(pkg)) => bin_sources.push(pkg), Ok(None) => {} Err(error) => return Err(LinkVirtualStoreBinsError::LinkBins(error)), @@ -363,7 +363,7 @@ where if bin_sources.is_empty() { return Ok(()); } - link_bins_of_packages::(&bin_sources, &bins_dir) + link_bins_of_packages::(&bin_sources, &bins_dir) .map_err(LinkVirtualStoreBinsError::LinkBins) }) } @@ -394,11 +394,11 @@ fn pkg_dir_under(modules_dir: &Path, name: &PkgName) -> PathBuf { /// then walk each slot's `node_modules` to discover children. Used /// only by [`crate::InstallWithoutLockfile`] today; the lockfile /// path bypasses every directory enumeration in here. -fn run_with_readdir(virtual_store_dir: &Path) -> Result<(), LinkVirtualStoreBinsError> +fn run_with_readdir(virtual_store_dir: &Path) -> Result<(), LinkVirtualStoreBinsError> where - Api: FsReadDir + Sys: FsReadDir + FsReadFile - + FsReadString + + FsReadToString + FsReadHead + FsCreateDirAll + FsWalkFiles @@ -406,7 +406,7 @@ where + FsSetExecutable + FsEnsureExecutableBits, { - let slots = match Api::read_dir(virtual_store_dir) { + let slots = match Sys::read_dir(virtual_store_dir) { Ok(slots) => slots, Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), Err(error) => { @@ -433,11 +433,11 @@ where // is trivial; the lockfile-driven path bypasses this by // treating the slot's own pkg dir as an invariant of // [`crate::create_virtual_dir_by_snapshot`]. - if Api::read_dir(&self_pkg_dir).is_err() { + if Sys::read_dir(&self_pkg_dir).is_err() { return Ok(()); } let bins_dir = self_pkg_dir.join("node_modules/.bin"); - link_bins_excluding::(&modules_dir, &bins_dir, &self_pkg_dir) + link_bins_excluding::(&modules_dir, &bins_dir, &self_pkg_dir) .map_err(LinkVirtualStoreBinsError::LinkBins) }) } @@ -457,7 +457,7 @@ where /// /// Returns `None` only when the slot name fails to parse — there's no /// filesystem probe for the resolved candidate. The previous version -/// stat-equivalent-ed the path with `Api::read_dir` to short-circuit +/// stat-equivalent-ed the path with `Sys::read_dir` to short-circuit /// missing slots, but on a 1267-package fixture that was 1267 /// wasted `open(O_DIRECTORY) + close` round-trips on the hot path of /// every warm install. The slot's own package directory is an @@ -495,15 +495,15 @@ fn find_slot_own_package_dir(slot_dir: &Path, modules_dir: &Path) -> Option( +fn link_bins_excluding( modules_dir: &Path, bins_dir: &Path, exclude: &Path, ) -> Result<(), LinkBinsError> where - Api: FsReadDir + Sys: FsReadDir + FsReadFile - + FsReadString + + FsReadToString + FsReadHead + FsCreateDirAll + FsWalkFiles @@ -513,7 +513,7 @@ where { let mut packages: Vec = Vec::new(); - let entries = match Api::read_dir(modules_dir) { + let entries = match Sys::read_dir(modules_dir) { Ok(entries) => entries, Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), Err(error) => { @@ -537,7 +537,7 @@ where // the bins for every package under this scope silently // disappear, so surface them instead of letting them // hide. - let scope_entries = match Api::read_dir(&path) { + let scope_entries = match Sys::read_dir(&path) { Ok(entries) => entries, Err(error) if error.kind() == io::ErrorKind::NotFound => continue, Err(error) => { @@ -548,7 +548,7 @@ where if paths_eq(&sub_path, exclude) { continue; } - if let Some(pkg) = read_package::(&sub_path)? { + if let Some(pkg) = read_package::(&sub_path)? { packages.push(pkg); } } @@ -558,7 +558,7 @@ where if paths_eq(&path, exclude) { continue; } - if let Some(pkg) = read_package::(&path)? { + if let Some(pkg) = read_package::(&path)? { packages.push(pkg); } } @@ -567,14 +567,14 @@ where return Ok(()); } - link_bins_of_packages::(&packages, bins_dir) + link_bins_of_packages::(&packages, bins_dir) } -fn read_package( +fn read_package( location: &Path, ) -> Result, LinkBinsError> { let manifest_path = location.join("package.json"); - let bytes = match Api::read_file(&manifest_path) { + let bytes = match Sys::read_file(&manifest_path) { Ok(bytes) => bytes, Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), Err(error) => return Err(LinkBinsError::ReadManifest { path: manifest_path, error }), diff --git a/crates/package-manager/src/link_bins/tests.rs b/crates/package-manager/src/link_bins/tests.rs index 2022cca7e..502392905 100644 --- a/crates/package-manager/src/link_bins/tests.rs +++ b/crates/package-manager/src/link_bins/tests.rs @@ -426,7 +426,7 @@ fn link_direct_dep_bins_skips_dep_with_missing_manifest() { #[test] fn link_virtual_store_bins_propagates_read_error_via_di() { use pacquet_cmd_shim::{ - FsCreateDirAll, FsEnsureExecutableBits, FsReadDir, FsReadFile, FsReadHead, FsReadString, + FsCreateDirAll, FsEnsureExecutableBits, FsReadDir, FsReadFile, FsReadHead, FsReadToString, FsSetExecutable, FsWalkFiles, FsWrite, }; use std::io; @@ -442,7 +442,7 @@ fn link_virtual_store_bins_propagates_read_error_via_di() { unreachable!() } } - impl FsReadString for DenyVirtualStore { + impl FsReadToString for DenyVirtualStore { fn read_to_string(_: &Path) -> io::Result { unreachable!() } diff --git a/crates/reporter/src/lib.rs b/crates/reporter/src/lib.rs index 89b62092f..518c7b3b7 100644 --- a/crates/reporter/src/lib.rs +++ b/crates/reporter/src/lib.rs @@ -741,7 +741,7 @@ fn now_millis() -> u128 { /// Capability for obtaining the host name written into the [bunyan]-shaped /// envelope. /// -/// Backed by a real syscall in production via [`RealApi`]. Tests can supply +/// Backed by a real syscall in production via [`Host`]. Tests can supply /// their own implementation when behavior depends on the value. /// /// [bunyan]: https://github.com/trentm/node-bunyan @@ -753,9 +753,9 @@ pub trait GetHostName { /// /// Each trait method calls into the real underlying system facility (for /// [`GetHostName`], the `gethostname` syscall via the [`gethostname`] crate). -pub struct RealApi; +pub struct Host; -impl GetHostName for RealApi { +impl GetHostName for Host { fn get_host_name() -> String { gethostname::gethostname().to_string_lossy().into_owned() } @@ -763,9 +763,9 @@ impl GetHostName for RealApi { // Process-wide cache of the host name. The value cannot change at runtime, // and `gethostname` is one syscall we'd otherwise repeat on every emit. -// Initialized lazily through `RealApi::get_host_name` so tests that exercise +// Initialized lazily through `Host::get_host_name` so tests that exercise // the capability trait directly can do so without paying for the syscall. -static HOSTNAME: LazyLock = LazyLock::new(RealApi::get_host_name); +static HOSTNAME: LazyLock = LazyLock::new(Host::get_host_name); #[cfg(test)] mod tests; diff --git a/crates/reporter/src/tests.rs b/crates/reporter/src/tests.rs index c6ee2c023..7556ad6fa 100644 --- a/crates/reporter/src/tests.rs +++ b/crates/reporter/src/tests.rs @@ -6,9 +6,9 @@ use serde_json::Value; use crate::{ AddedRoot, BrokenModulesLog, ContextLog, DependencyType, Envelope, FetchingProgressLog, - FetchingProgressMessage, GetHostName, IgnoredScriptsLog, LifecycleLog, LifecycleMessage, + FetchingProgressMessage, GetHostName, Host, IgnoredScriptsLog, LifecycleLog, LifecycleMessage, LifecycleStdio, LogEvent, LogLevel, PackageImportMethod, PackageImportMethodLog, - PackageManifestLog, PackageManifestMessage, ProgressLog, ProgressMessage, RealApi, RemovedRoot, + PackageManifestLog, PackageManifestMessage, ProgressLog, ProgressMessage, RemovedRoot, Reporter, RequestRetryError, RequestRetryLog, RootLog, RootMessage, SilentReporter, SkippedOptionalDependencyLog, SkippedOptionalPackage, SkippedOptionalReason, Stage, StageLog, StatsLog, StatsMessage, SummaryLog, @@ -814,11 +814,11 @@ fn get_host_name_capability_is_mockable() { assert_eq!(FakeApi::get_host_name(), "fixture-host"); } -/// [`RealApi::get_host_name`] returns the value of `gethostname(2)`, +/// [`Host::get_host_name`] returns the value of `gethostname(2)`, /// which any real environment populates with at least one byte. #[test] fn real_api_returns_a_non_empty_host_name() { - let host = RealApi::get_host_name(); - eprintln!("RealApi::get_host_name() = {host:?}"); + let host = Host::get_host_name(); + eprintln!("Host::get_host_name() = {host:?}"); assert!(!host.is_empty()); }