From 5b52fa1092b30c2c1b677e084b22a676901ca285 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 24 Aug 2026 18:20:00 +0000 Subject: [PATCH 1/2] devpod's on-disk layout gets the module it was missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `clients::devpod` is the seam for devpod-the-command. devpod-the-filesystem had no seam at all, so `/contexts//workspaces//…` was rebuilt wherever it was wanted: three times in `flows/lifecycle.rs`, once in `flows/provision/verdict_cache.rs`, and four more in test fixtures that reproduced the convention literally. The comment at lifecycle.rs:2861 claimed the paths went through shared helpers "so devpod's on-disk layout is spelled out in one place and not three". It was spelled in four, and the test fixtures made it eight. `devlaunch-core/tests/devpod_layout.rs` is what makes the claim checkable rather than aspirational: it walks the crate's sources and fails if any file but `clients/devpod_home.rs` contains a string literal starting `contexts`. Before this change it named all eight, which is the honest red — the failure a layout copy causes is not a wrong answer but a second copy that agrees with the first until devpod moves it, and nothing but a guard catches that. A second test asserts the adapter *does* spell it, so the guard cannot pass by the layout having been deleted. Prose is deliberately not matched (a doc comment costs nothing when devpod moves the layout), nor is devpod `config.yaml`'s top-level `contexts:` key, which is a different fact about a different file. `dl`'s end-to-end tests are out of scope on purpose: they stand outside the crate and check that what dl wrote landed where devpod will look for it, which is exactly the assertion that must not be routed through the code under test. Moved into `clients::devpod_home`, whole: `devpod_home`, the record and result path builders, `CreateRecord`/`create_record`, the contexts walk, `sole_workspace_result`, `repoint_devpod_source` and `RepointFailure`. Plus `Host::devpod_config`, which was joining `config.yaml` onto the same bare path. Four decisions worth the words: - **The home is taken, not resolved.** `DevpodHome::at` takes the directory and the binary resolves it once, which is the convention `dl/src/commands.rs` already states at its `workspace_delete` call. `DevpodHome::locate` is the one place that reads `DEVPOD_HOME`, and it splits into a thin environment read over a pure `located(configured, home_dir)`, the shape `osext::home_dir` uses. The old test could only assert that this machine has a home directory; the pure half pins that an empty `DEVPOD_HOME` falls through to `~/.devpod` rather than naming the current directory, without mutating an environment the whole test binary shares. - **`repoint` takes `(context, workspace_id, source)`, not `&Adoptable`.** An adapter under `clients/` that named a reconcile-flow type would be a seam pointing the wrong way. Its doc — devpod v0.26.1 has no subcommand that changes an existing workspace's source, so the choice is one field of one JSON file or no repair at all — carries across, and is what makes writing into devpod's own file an adapter's job rather than a leak. - **`create_record` and `sole_workspace_result` stay free functions taking `Option<&DevpodHome>`, rather than becoming methods.** "This machine has no devpod home" is one of the answers each gives, and absorbing it in the module keeps five call sites from each deciding again what a missing home means — they would not all decide it the same way, and one of them reads the absence as `!= NeverCompleted`. - **The pass-through signatures take the type.** `workspace_delete`, `purge_all_data` and `apply_reconciliation` took a `&Path` named `devpod_home`; a bare path is what anybody can join, which is how there came to be eight copies. This changes `api::workspace_delete`'s promised signature — the one row of `public-api.api.txt` this touches — and the argument for paying that is the same one: the parameter was always devpod's home and now says so. The win this protects: `flows::lifecycle`'s `mod tests` is private again. It was `pub(crate)` solely so `flows/provision.rs` and `flows/provision/verdict_cache.rs` could import `tests::devpod_home_with` — a test fixture in the crate's internal surface because a devpod-home module did not exist. The fixture now sits beside the layout it builds as a plain `#[cfg(test)]` item at module scope, which is what keeps it shareable without a second `pub(crate) mod tests` in its new home. Nothing outside imported anything else from there. Both snapshots are hand-edited: `cargo-public-api` needs a nightly toolchain this container cannot install, so CI's regeneration is authoritative over these rows. --- CHANGELOG.md | 20 + rust/devlaunch-core/public-api.api.txt | 2 +- rust/devlaunch-core/public-api.rest.txt | 65 ++- rust/devlaunch-core/src/clients/devpod.rs | 7 +- .../devlaunch-core/src/clients/devpod_home.rs | 465 ++++++++++++++++++ rust/devlaunch-core/src/clients/mod.rs | 3 + rust/devlaunch-core/src/flows/launch.rs | 54 +- rust/devlaunch-core/src/flows/lifecycle.rs | 420 ++-------------- rust/devlaunch-core/src/flows/provision.rs | 13 +- .../src/flows/provision/verdict_cache.rs | 59 ++- rust/devlaunch-core/tests/devpod_layout.rs | 107 ++++ rust/dl/src/commands.rs | 11 +- rust/dl/src/launch.rs | 8 +- rust/dl/src/render.rs | 4 +- rust/dl/tests/launch.rs | 2 +- 15 files changed, 745 insertions(+), 495 deletions(-) create mode 100644 rust/devlaunch-core/src/clients/devpod_home.rs create mode 100644 rust/devlaunch-core/tests/devpod_layout.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ab2e010..b1c09352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 self-review is recorded as a notice on the run, and a review that predates the head is warned about rather than failed on. +### Changed + +- **devpod's on-disk layout is one module's, and that module now exists.** + `clients::devpod` was the seam for devpod-the-command; devpod-the-filesystem + had none, so `/contexts//workspaces//…` was rebuilt + wherever it was needed — four times in `devlaunch-core`'s implementation and + four more in its test fixtures, under a comment claiming it was spelled "in one + place and not three". `clients::devpod_home::DevpodHome` owns it now, including + the one write (`--reconcile` re-points a record's `source.localFolder`, which + devpod v0.26.1 offers no subcommand for), and `devlaunch-core`'s new + `tests/devpod_layout.rs` fails if any other module in the crate spells it + again. `lifecycle::devpod_home()` becomes `DevpodHome::locate()`, and + `lifecycle::RepointFailure` moves with the write that raises it; + `api::workspace_delete`, `lifecycle::purge_all_data` and + `lifecycle::apply_reconciliation` take a `DevpodHome` where they took a bare + path. It also takes a test module back out of the crate's internal surface: + `flows::lifecycle`'s `mod tests` was `pub(crate)` only so two other flows could + import a devpod-home fixture from it, and the fixture now lives with the + layout it builds. + ## [0.13.0] - 2026-08-24 ### Changed diff --git a/rust/devlaunch-core/public-api.api.txt b/rust/devlaunch-core/public-api.api.txt index 12764ab0..8cc009b6 100644 --- a/rust/devlaunch-core/public-api.api.txt +++ b/rust/devlaunch-core/public-api.api.txt @@ -33,5 +33,5 @@ pub fn devlaunch_core::api::identity(&str) -> core::result::Result serde_json::value::Value pub fn devlaunch_core::api::parse(&str) -> devlaunch_core::domain::spec::WorkspaceSpec<'_> pub fn devlaunch_core::api::resolve_devcontainer_ref(&str) -> core::result::Result -pub fn devlaunch_core::api::workspace_delete(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &devlaunch_core::flows::workspace_clone::WorkspaceCloneManager<'_>, &mut devlaunch_core::domain::metadata::MetadataStorage, core::option::Option<&std::path::Path>, &str, devlaunch_core::flows::lifecycle::Insistence, &mut dyn devlaunch_core::notices::Notices) -> core::result::Result +pub fn devlaunch_core::api::workspace_delete(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &devlaunch_core::flows::workspace_clone::WorkspaceCloneManager<'_>, &mut devlaunch_core::domain::metadata::MetadataStorage, core::option::Option<&devlaunch_core::clients::devpod_home::DevpodHome>, &str, devlaunch_core::flows::lifecycle::Insistence, &mut dyn devlaunch_core::notices::Notices) -> core::result::Result pub fn devlaunch_core::api::workspace_stop(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &str) -> core::result::Result diff --git a/rust/devlaunch-core/public-api.rest.txt b/rust/devlaunch-core/public-api.rest.txt index 0096b7af..e6c234d9 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -147,6 +147,40 @@ pub fn devlaunch_core::clients::devpod::Workspace::eq(&self, &devlaunch_core::cl impl core::fmt::Debug for devlaunch_core::clients::devpod::Workspace pub fn devlaunch_core::clients::devpod::Workspace::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for devlaunch_core::clients::devpod::Workspace +pub mod devlaunch_core::clients::devpod_home +pub struct devlaunch_core::clients::devpod_home::DevpodHome +impl devlaunch_core::clients::devpod_home::DevpodHome +pub fn devlaunch_core::clients::devpod_home::DevpodHome::at(impl core::convert::Into) -> Self +pub fn devlaunch_core::clients::devpod_home::DevpodHome::locate() -> core::option::Option +pub fn devlaunch_core::clients::devpod_home::DevpodHome::path(&self) -> &std::path::Path +impl core::clone::Clone for devlaunch_core::clients::devpod_home::DevpodHome +pub fn devlaunch_core::clients::devpod_home::DevpodHome::clone(&self) -> devlaunch_core::clients::devpod_home::DevpodHome +impl core::cmp::Eq for devlaunch_core::clients::devpod_home::DevpodHome +impl core::cmp::PartialEq for devlaunch_core::clients::devpod_home::DevpodHome +pub fn devlaunch_core::clients::devpod_home::DevpodHome::eq(&self, &devlaunch_core::clients::devpod_home::DevpodHome) -> bool +impl core::fmt::Debug for devlaunch_core::clients::devpod_home::DevpodHome +pub fn devlaunch_core::clients::devpod_home::DevpodHome::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::StructuralPartialEq for devlaunch_core::clients::devpod_home::DevpodHome +pub enum devlaunch_core::clients::devpod_home::RepointFailure +pub devlaunch_core::clients::devpod_home::RepointFailure::NotADevpodRecord +pub devlaunch_core::clients::devpod_home::RepointFailure::NotADevpodRecord::path: std::path::PathBuf +pub devlaunch_core::clients::devpod_home::RepointFailure::NotJson +pub devlaunch_core::clients::devpod_home::RepointFailure::NotJson::path: std::path::PathBuf +pub devlaunch_core::clients::devpod_home::RepointFailure::NotJson::reason: alloc::string::String +pub devlaunch_core::clients::devpod_home::RepointFailure::Unreadable +pub devlaunch_core::clients::devpod_home::RepointFailure::Unreadable::path: std::path::PathBuf +pub devlaunch_core::clients::devpod_home::RepointFailure::Unreadable::reason: alloc::string::String +pub devlaunch_core::clients::devpod_home::RepointFailure::Unwritable +pub devlaunch_core::clients::devpod_home::RepointFailure::Unwritable::path: std::path::PathBuf +pub devlaunch_core::clients::devpod_home::RepointFailure::Unwritable::reason: alloc::string::String +impl core::clone::Clone for devlaunch_core::clients::devpod_home::RepointFailure +pub fn devlaunch_core::clients::devpod_home::RepointFailure::clone(&self) -> devlaunch_core::clients::devpod_home::RepointFailure +impl core::cmp::Eq for devlaunch_core::clients::devpod_home::RepointFailure +impl core::cmp::PartialEq for devlaunch_core::clients::devpod_home::RepointFailure +pub fn devlaunch_core::clients::devpod_home::RepointFailure::eq(&self, &devlaunch_core::clients::devpod_home::RepointFailure) -> bool +impl core::fmt::Debug for devlaunch_core::clients::devpod_home::RepointFailure +pub fn devlaunch_core::clients::devpod_home::RepointFailure::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::StructuralPartialEq for devlaunch_core::clients::devpod_home::RepointFailure pub mod devlaunch_core::clients::gh pub enum devlaunch_core::clients::gh::GhEvent pub devlaunch_core::clients::gh::GhEvent::CouldNotRun(devlaunch_core::clients::gh::GhUnavailable) @@ -1042,7 +1076,7 @@ pub fn devlaunch_core::flows::launch::resolve_triple(&mut devlaunch_core::flows: pub mod devlaunch_core::flows::lifecycle pub enum devlaunch_core::flows::lifecycle::Adoption pub devlaunch_core::flows::lifecycle::Adoption::Refused -pub devlaunch_core::flows::lifecycle::Adoption::Refused::failure: devlaunch_core::flows::lifecycle::RepointFailure +pub devlaunch_core::flows::lifecycle::Adoption::Refused::failure: devlaunch_core::clients::devpod_home::RepointFailure pub devlaunch_core::flows::lifecycle::Adoption::Refused::workspace_id: alloc::string::String pub devlaunch_core::flows::lifecycle::Adoption::Repointed(alloc::boxed::Box) impl core::clone::Clone for devlaunch_core::flows::lifecycle::Adoption @@ -1278,26 +1312,6 @@ pub fn devlaunch_core::flows::lifecycle::RemovalRefused::eq(&self, &devlaunch_co impl core::fmt::Debug for devlaunch_core::flows::lifecycle::RemovalRefused pub fn devlaunch_core::flows::lifecycle::RemovalRefused::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for devlaunch_core::flows::lifecycle::RemovalRefused -pub enum devlaunch_core::flows::lifecycle::RepointFailure -pub devlaunch_core::flows::lifecycle::RepointFailure::NotADevpodRecord -pub devlaunch_core::flows::lifecycle::RepointFailure::NotADevpodRecord::path: std::path::PathBuf -pub devlaunch_core::flows::lifecycle::RepointFailure::NotJson -pub devlaunch_core::flows::lifecycle::RepointFailure::NotJson::path: std::path::PathBuf -pub devlaunch_core::flows::lifecycle::RepointFailure::NotJson::reason: alloc::string::String -pub devlaunch_core::flows::lifecycle::RepointFailure::Unreadable -pub devlaunch_core::flows::lifecycle::RepointFailure::Unreadable::path: std::path::PathBuf -pub devlaunch_core::flows::lifecycle::RepointFailure::Unreadable::reason: alloc::string::String -pub devlaunch_core::flows::lifecycle::RepointFailure::Unwritable -pub devlaunch_core::flows::lifecycle::RepointFailure::Unwritable::path: std::path::PathBuf -pub devlaunch_core::flows::lifecycle::RepointFailure::Unwritable::reason: alloc::string::String -impl core::clone::Clone for devlaunch_core::flows::lifecycle::RepointFailure -pub fn devlaunch_core::flows::lifecycle::RepointFailure::clone(&self) -> devlaunch_core::flows::lifecycle::RepointFailure -impl core::cmp::Eq for devlaunch_core::flows::lifecycle::RepointFailure -impl core::cmp::PartialEq for devlaunch_core::flows::lifecycle::RepointFailure -pub fn devlaunch_core::flows::lifecycle::RepointFailure::eq(&self, &devlaunch_core::flows::lifecycle::RepointFailure) -> bool -impl core::fmt::Debug for devlaunch_core::flows::lifecycle::RepointFailure -pub fn devlaunch_core::flows::lifecycle::RepointFailure::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl core::marker::StructuralPartialEq for devlaunch_core::flows::lifecycle::RepointFailure pub enum devlaunch_core::flows::lifecycle::SpawnRefused pub devlaunch_core::flows::lifecycle::SpawnRefused::Blocked(devlaunch_runner::OsFailure) pub devlaunch_core::flows::lifecycle::SpawnRefused::ProgramNotFound @@ -1533,19 +1547,18 @@ pub fn devlaunch_core::flows::lifecycle::Withheld::eq(&self, &devlaunch_core::fl impl core::fmt::Debug for devlaunch_core::flows::lifecycle::Withheld pub fn devlaunch_core::flows::lifecycle::Withheld::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for devlaunch_core::flows::lifecycle::Withheld -pub fn devlaunch_core::flows::lifecycle::apply_reconciliation(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &mut devlaunch_core::domain::metadata::MetadataStorage, &std::path::Path, &devlaunch_core::flows::lifecycle::ReconcilePlan, &mut dyn devlaunch_core::notices::Notices) -> devlaunch_core::flows::lifecycle::ReconcileReport +pub fn devlaunch_core::flows::lifecycle::apply_reconciliation(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &mut devlaunch_core::domain::metadata::MetadataStorage, &devlaunch_core::clients::devpod_home::DevpodHome, &devlaunch_core::flows::lifecycle::ReconcilePlan, &mut dyn devlaunch_core::notices::Notices) -> devlaunch_core::flows::lifecycle::ReconcileReport pub fn devlaunch_core::flows::lifecycle::child_work(&std::path::Path, devlaunch_core::flows::lifecycle::RefreshReason) -> devlaunch_core::flows::lifecycle::ChildWork -pub fn devlaunch_core::flows::lifecycle::devpod_home() -> core::option::Option pub fn devlaunch_core::flows::lifecycle::guard_removal(&str, devlaunch_core::domain::workspace_state::Unsaved, devlaunch_core::flows::lifecycle::Insistence) -> devlaunch_core::flows::lifecycle::Guarded pub fn devlaunch_core::flows::lifecycle::objection(&devlaunch_core::domain::workspace_state::Unsaved) -> core::option::Option pub fn devlaunch_core::flows::lifecycle::prune_clones(&mut devlaunch_core::flows::listing::CommandContext<'_>, &devlaunch_core::flows::workspace_clone::WorkspaceCloneManager<'_>, &mut devlaunch_core::domain::metadata::MetadataStorage, &devlaunch_core::flows::lifecycle::PrunePlan, &mut dyn devlaunch_core::notices::Notices) -> core::result::Result pub fn devlaunch_core::flows::lifecycle::prune_plan(&devlaunch_core::flows::workspace_clone::WorkspaceCloneManager<'_>, &devlaunch_core::domain::metadata::MetadataStorage, &[devlaunch_core::clients::devpod::Workspace], &devlaunch_core::flows::lifecycle::ClonePlacement, devlaunch_core::flows::lifecycle::Insistence, &mut dyn devlaunch_core::notices::Notices) -> core::result::Result -pub fn devlaunch_core::flows::lifecycle::purge_all_data(&mut devlaunch_core::flows::listing::CommandContext<'_>, &devlaunch_core::flows::lifecycle::PurgePlan, core::option::Option<&std::path::Path>, &mut dyn core::ops::function::FnMut(devlaunch_core::flows::lifecycle::PurgeStep)) -> core::result::Result +pub fn devlaunch_core::flows::lifecycle::purge_all_data(&mut devlaunch_core::flows::listing::CommandContext<'_>, &devlaunch_core::flows::lifecycle::PurgePlan, core::option::Option<&devlaunch_core::clients::devpod_home::DevpodHome>, &mut dyn core::ops::function::FnMut(devlaunch_core::flows::lifecycle::PurgeStep)) -> core::result::Result pub fn devlaunch_core::flows::lifecycle::purge_plan(&mut devlaunch_core::flows::listing::CommandContext<'_>, &std::path::Path) -> core::result::Result pub fn devlaunch_core::flows::lifecycle::reconcile_plan(&devlaunch_core::flows::workspace_clone::WorkspaceCloneManager<'_>, &devlaunch_core::domain::metadata::MetadataStorage, &[devlaunch_core::clients::devpod::Workspace], &devlaunch_core::flows::lifecycle::ClonePlacement, &mut dyn devlaunch_core::notices::Notices) -> devlaunch_core::flows::lifecycle::ReconcilePlan pub fn devlaunch_core::flows::lifecycle::sweep_repo_fetches(&devlaunch_core::flows::repo_manager::RepositoryManager<'_>, &mut devlaunch_core::domain::metadata::MetadataStorage) -> devlaunch_core::flows::lifecycle::SweepReport pub fn devlaunch_core::flows::lifecycle::unsaved_work_in(&devlaunch_core::flows::workspace_clone::WorkspaceCloneManager<'_>, &devlaunch_core::domain::metadata::MetadataStorage, &devlaunch_core::clients::git::Git<'_>, &std::path::Path, &str, &mut dyn devlaunch_core::notices::Notices) -> devlaunch_core::domain::workspace_state::Unsaved -pub fn devlaunch_core::flows::lifecycle::workspace_delete(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &devlaunch_core::flows::workspace_clone::WorkspaceCloneManager<'_>, &mut devlaunch_core::domain::metadata::MetadataStorage, core::option::Option<&std::path::Path>, &str, devlaunch_core::flows::lifecycle::Insistence, &mut dyn devlaunch_core::notices::Notices) -> core::result::Result +pub fn devlaunch_core::flows::lifecycle::workspace_delete(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &devlaunch_core::flows::workspace_clone::WorkspaceCloneManager<'_>, &mut devlaunch_core::domain::metadata::MetadataStorage, core::option::Option<&devlaunch_core::clients::devpod_home::DevpodHome>, &str, devlaunch_core::flows::lifecycle::Insistence, &mut dyn devlaunch_core::notices::Notices) -> core::result::Result pub fn devlaunch_core::flows::lifecycle::workspace_state(&dyn devlaunch_runner::Runner, &str) -> core::result::Result pub fn devlaunch_core::flows::lifecycle::workspace_stop(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &str) -> core::result::Result pub mod devlaunch_core::flows::listing @@ -1803,7 +1816,7 @@ pub mod devlaunch_core::flows::provision pub mod devlaunch_core::flows::provision::verdict_cache pub struct devlaunch_core::flows::provision::verdict_cache::VerdictCache impl devlaunch_core::flows::provision::verdict_cache::VerdictCache -pub fn devlaunch_core::flows::provision::verdict_cache::VerdictCache::under(&std::path::Path, core::option::Option) -> Self +pub fn devlaunch_core::flows::provision::verdict_cache::VerdictCache::under(&std::path::Path, core::option::Option) -> Self impl core::clone::Clone for devlaunch_core::flows::provision::verdict_cache::VerdictCache pub fn devlaunch_core::flows::provision::verdict_cache::VerdictCache::clone(&self) -> devlaunch_core::flows::provision::verdict_cache::VerdictCache impl core::cmp::Eq for devlaunch_core::flows::provision::verdict_cache::VerdictCache diff --git a/rust/devlaunch-core/src/clients/devpod.rs b/rust/devlaunch-core/src/clients/devpod.rs index 7c970173..13a4ad35 100644 --- a/rust/devlaunch-core/src/clients/devpod.rs +++ b/rust/devlaunch-core/src/clients/devpod.rs @@ -553,10 +553,9 @@ pub struct Workspace { pub(crate) provider: String, pub(crate) ide: String, /// The devpod context this workspace belongs to. Ids are unique per context, - /// not globally, so it is half of the workspace's address on disk — devpod - /// keeps each workspace's own record at - /// `/contexts//workspaces//workspace.json`, which - /// is what `--reconcile` re-points. + /// not globally, so it is half of the workspace's address on disk: it is one + /// of the two things [`super::devpod_home::DevpodHome::record`] needs to name + /// the record `--reconcile` re-points. pub(crate) context: String, } diff --git a/rust/devlaunch-core/src/clients/devpod_home.rs b/rust/devlaunch-core/src/clients/devpod_home.rs new file mode 100644 index 00000000..15d5c4e4 --- /dev/null +++ b/rust/devlaunch-core/src/clients/devpod_home.rs @@ -0,0 +1,465 @@ +//! devpod's own records on disk, as one layout rather than a path passed around. +//! +//! [`super::devpod`] is the seam for devpod-the-*command*. This is the seam for +//! devpod-the-*filesystem*: the directory devpod keeps its state in, and the +//! convention it keeps it under — +//! +//! ```text +//! /contexts//workspaces//workspace.json +//! /contexts//workspaces//workspace_result.json +//! ``` +//! +//! That convention is devpod's, not devlaunch's, and it is knowledge the way a +//! response format is knowledge: it can move under a devpod upgrade, and every +//! place that spelled it would then be wrong on its own. So it is spelled here, +//! and `devlaunch-core/tests/devpod_layout.rs` holds the rest of the crate to +//! that. +//! +//! # Reading it is the point; one write is the exception +//! +//! Everything here but [`DevpodHome::repoint`] reads. That write is argued for at +//! the method, and it is what makes this an adapter rather than a path helper: the +//! module owns devpod's file format on the way out as well as the way in, so the +//! flows above it never open one. +//! +//! # The home is taken, not resolved +//! +//! [`DevpodHome::at`] takes the directory; [`DevpodHome::locate`] is the one place +//! that reads the environment for it, and the binary calls it once and hands the +//! answer down — the same convention every other environment answer in dl follows +//! (`dl/src/commands.rs`, at the `workspace_delete` call). A flow that resolved its +//! own could disagree with the one the launch already resolved. + +use std::ffi::OsString; +use std::path::{Path, PathBuf}; + +use crate::flows::repo_manager::system_words; + +/// The directory devpod keeps its own records in. +/// +/// A type rather than a `PathBuf` because the layout underneath it is the whole of +/// what this module knows: a bare path can be joined by anybody, and eight places +/// in this crate did. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct DevpodHome { + root: PathBuf, +} + +impl DevpodHome { + /// devpod's home at this directory, whether or not anything is there yet. + pub fn at(root: impl Into) -> Self { + Self { root: root.into() } + } + + /// Where this machine's devpod keeps its records, or nothing on a machine with + /// no home directory to put them under. + /// + /// Honours `DEVPOD_HOME` for the same reason the rest of dl does: it is what + /// scopes devpod, and the test suite sets it. The thin half of the pair — it + /// reads the process environment and nothing else, so [`Self::located`] is + /// where the cases are pinned. + pub fn locate() -> Option { + Self::located(std::env::var_os("DEVPOD_HOME"), crate::osext::home_dir) + } + + /// The pure core of [`Self::locate`]: `configured` is what `DEVPOD_HOME` holds + /// (or its absence), `home_dir` is consulted only when it holds nothing usable. + /// + /// An *empty* `DEVPOD_HOME` falls through to the home directory rather than + /// naming the current directory, which is what an empty path would mean — a + /// variable someone cleared must not silently point devpod's records at + /// wherever dl was run from. + fn located( + configured: Option, + home_dir: impl FnOnce() -> Option, + ) -> Option { + match configured { + Some(home) if !home.is_empty() => Some(Self::at(home)), + _ => home_dir().map(|home| Self::at(home.join(".devpod"))), + } + } + + /// The directory itself, for the callers that only have to name it. + pub fn path(&self) -> &Path { + &self.root + } + + /// devpod's config file, which holds every context and its options. + pub(crate) fn config(&self) -> PathBuf { + self.root.join("config.yaml") + } + + /// The directory holding every context's records. + fn contexts(&self) -> PathBuf { + self.root.join("contexts") + } + + /// One workspace's directory under one context — the address both record files + /// hang off, and the only place the layout is spelled. + fn workspace_dir(&self, context: &str, workspace_id: &str) -> PathBuf { + self.contexts() + .join(context) + .join("workspaces") + .join(workspace_id) + } + + /// devpod's own record for one workspace. + pub(crate) fn record(&self, context: &str, workspace_id: &str) -> PathBuf { + self.workspace_dir(context, workspace_id) + .join("workspace.json") + } + + /// devpod's record of a *completed* create for one workspace. + /// + /// devpod writes this beside [`Self::record`] on its way out of a successful + /// `up`, and it is where the container's remote user lives + /// (`.MergedConfig.remoteUser`). A create that died in its lifecycle hooks + /// leaves the record and no result — which is also why `devpod ssh` into one + /// lands as root: there is no recorded user for it to become. + pub(crate) fn result(&self, context: &str, workspace_id: &str) -> PathBuf { + self.workspace_dir(context, workspace_id) + .join("workspace_result.json") + } + + /// The one context whose records name this workspace, or nothing when the + /// question has no single answer. + /// + /// Nothing for all four ways of not having one: a `contexts` directory that + /// would not read, no context holding the id, two contexts holding it, and a + /// context whose name is not UTF-8 (skipped, because a path this cannot spell + /// is one devpod's own id lookup cannot have written). + /// + /// Split out because [`create_record`] and [`sole_workspace_result`] both have + /// to walk the contexts *the same way* and read the same ambiguity out of them: + /// the two answer different questions about one workspace, and a second copy of + /// this loop is how they would come to disagree about which context that + /// workspace is in. + fn sole_context_holding(&self, workspace_id: &str) -> Option { + let contexts = std::fs::read_dir(self.contexts()).ok()?; + let mut sole: Option = None; + for context in contexts.flatten() { + let Some(name) = context.file_name().to_str().map(str::to_owned) else { + continue; + }; + if !self.record(&name, workspace_id).is_file() { + continue; + } + if sole.is_some() { + return None; + } + sole = Some(name); + } + sole + } + + /// Rewrite one devpod workspace record's source folder. + /// + /// **devpod's own file, written directly, and that is a decision rather than a + /// convenience.** devpod v0.26.1 has no subcommand that changes an existing + /// workspace's source: the surface is build, delete, list, logs, ssh, status, + /// stop and up, and the only one that sets a source is a create, which needs a + /// container daemon and would destroy the very record being repaired. So the + /// choice is between one field of one JSON file and not repairing anything. + /// Writing it from *inside* the module that already owns devpod's layout is + /// what keeps that a seam rather than a leak. + /// + /// Only `source.localFolder` is touched, and the file is rewritten from what + /// devpod itself last wrote, so every key devpod knows about and dl does not — + /// `uid`, provider options, timestamps — survives untouched. Written through a + /// temporary file in the same directory and renamed over the original, so a + /// failure partway leaves devpod's record whole rather than truncated. + pub(crate) fn repoint( + &self, + context: &str, + workspace_id: &str, + source: &Path, + ) -> Result<(), RepointFailure> { + let path = self.record(context, workspace_id); + let text = std::fs::read_to_string(&path).map_err(|error| RepointFailure::Unreadable { + path: path.clone(), + reason: system_words(&error), + })?; + let mut record: serde_json::Value = + serde_json::from_str(&text).map_err(|error| RepointFailure::NotJson { + path: path.clone(), + reason: error.to_string(), + })?; + let Some(recorded) = record + .as_object_mut() + .and_then(|record| record.get_mut("source")) + .and_then(serde_json::Value::as_object_mut) + else { + return Err(RepointFailure::NotADevpodRecord { path }); + }; + recorded.insert( + "localFolder".to_owned(), + serde_json::Value::String(source.display().to_string()), + ); + // Two-space indentation, as devpod and Python's `json.dumps(…, indent=2)` + // both write it: a file two tools take turns rewriting should not churn its + // shape. + let rewritten = + serde_json::to_string_pretty(&record).map_err(|error| RepointFailure::Unwritable { + path: path.clone(), + reason: error.to_string(), + })?; + let temp = path.with_extension("dl-tmp"); + if let Err(error) = std::fs::write(&temp, rewritten) { + let _ = std::fs::remove_file(&temp); + return Err(RepointFailure::Unwritable { + path, + reason: system_words(&error), + }); + } + if let Err(error) = std::fs::rename(&temp, &path) { + let _ = std::fs::remove_file(&temp); + return Err(RepointFailure::Unwritable { + path, + reason: system_words(&error), + }); + } + Ok(()) + } +} + +/// Whether devpod finished creating a workspace, as far as its own records say. +/// +/// Three arms rather than a bool, for the reason [`crate::domain::workspace_state`] +/// needed three: "no evidence it finished" and "no evidence either way" are +/// different facts, and only the first is a reason to act. A caller that acted on +/// the second would rebuild every workspace on a host whose devpod home it cannot +/// read. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum CreateRecord { + /// devpod wrote its create result, so the workspace was set up. + Completed, + /// devpod holds a record for this workspace and no result beside it: an `up` + /// started and did not finish. + NeverCompleted, + /// Nothing here can answer — no devpod home, no record under any context, a + /// directory that would not read, or one id in two contexts. + Unknown, +} + +/// What devpod's own records say about whether this workspace's create finished. +/// +/// Takes an `Option` rather than being a method on [`DevpodHome`] because "this +/// machine has no devpod home" is one of the answers. Absorbing it here is what +/// keeps four call sites from each deciding again what a missing home means, and +/// they would not all decide it the same way. +/// +/// Every context is searched rather than `default` being assumed: ids are unique +/// per context, [`crate::flows::launch::Placement`] carries no context, and asking +/// devpod for one would put a second round trip on the warm attach path this +/// exists to guard. Two contexts holding the same id answer [`CreateRecord::Unknown`] +/// rather than picking one — the ambiguity is the answer. +pub(crate) fn create_record(devpod_home: Option<&DevpodHome>, workspace_id: &str) -> CreateRecord { + let Some(home) = devpod_home else { + return CreateRecord::Unknown; + }; + let Some(context) = home.sole_context_holding(workspace_id) else { + return CreateRecord::Unknown; + }; + if home.result(&context, workspace_id).is_file() { + CreateRecord::Completed + } else { + CreateRecord::NeverCompleted + } +} + +/// The one `workspace_result.json` this workspace has, if it has exactly one. +/// +/// devpod rewrites that file on its way out of every *completed* `up`, whoever ran +/// the `up` — `dl`, VS Code, a hand-typed `devpod up` — which is what makes its +/// mtime a usable "has this container been rebuilt since?" anchor for +/// [`crate::flows::provision::verdict_cache`]. The record beside it is not: devpod +/// writes `workspace.json` on the way *in* and leaves it alone afterwards, so a +/// container recreated ten times still carries the first create's timestamps. +/// +/// Nothing rather than a guess in every ambiguous case, because the caller's only +/// use for the answer is to *skip* work: no devpod home, no record, two contexts +/// holding the id, or a record with no result beside it (an `up` that died in its +/// hooks) all mean the same thing to it — there is no file whose mtime can be +/// trusted to move when this container is rebuilt, so trust nothing. +pub(crate) fn sole_workspace_result( + devpod_home: Option<&DevpodHome>, + workspace_id: &str, +) -> Option { + let home = devpod_home?; + let context = home.sole_context_holding(workspace_id)?; + let result = home.result(&context, workspace_id); + result.is_file().then_some(result) +} + +/// Why one devpod record could not be re-pointed. +/// +/// Unreadable and not-JSON are two arms where Python's one `except` clause caught +/// both: an `OSError` and a decode error read the same to its f-string but not to +/// a caller, and one `reason: String` could not say which had happened. Both +/// render through the same sentence, so the split changes no output. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum RepointFailure { + /// devpod's record's bytes could not be read; `reason` is the OS's words. + Unreadable { path: PathBuf, reason: String }, + /// The bytes are not JSON; `reason` is the parser's words. + NotJson { path: PathBuf, reason: String }, + /// Not the shape this repair understands. Refusing is the whole of the + /// response: a source key dl cannot read is one it cannot safely replace. + NotADevpodRecord { path: PathBuf }, + /// The rewritten record could not be put in place. devpod's own file is left + /// whole: the write goes to a sibling temp file and is renamed over it. + Unwritable { path: PathBuf, reason: String }, +} + +/// A devpod home under a temporary directory, holding whatever these workspaces +/// were left holding. `Some(())` writes the create result beside the record; +/// `None` leaves the record alone, which is what an aborted `up` leaves behind. +/// +/// Lives beside the layout it builds rather than in the tests that use it — three +/// modules' tests want one, and the layout is spelled here or it is spelled three +/// more times. A plain `#[cfg(test)]` item at module scope, not an export of a test +/// module: `flows::lifecycle::tests` was `pub(crate)` for exactly this fixture, +/// which put a test module into the crate's internal surface because this module +/// did not exist. +#[cfg(test)] +pub(crate) fn devpod_home_with(entries: &[(&str, &str, Option<()>)]) -> ScratchHome { + let dir = tempfile::tempdir().expect("a scratch devpod home"); + let home = DevpodHome::at(dir.path()); + for (context, workspace_id, result) in entries { + let record = home.record(context, workspace_id); + std::fs::create_dir_all(record.parent().expect("a record directory")) + .expect("a record directory"); + std::fs::write(record, "{}").expect("a record"); + if result.is_some() { + std::fs::write(home.result(context, workspace_id), "{}").expect("a result"); + } + } + ScratchHome { _dir: dir, home } +} + +/// A [`DevpodHome`] and the temporary directory keeping it alive. +/// +/// Derefs to the home, so a test that holds one reads as if it held the home +/// itself; the directory is a field only because dropping it would take the +/// records away. +#[cfg(test)] +pub(crate) struct ScratchHome { + _dir: tempfile::TempDir, + home: DevpodHome, +} + +#[cfg(test)] +impl std::ops::Deref for ScratchHome { + type Target = DevpodHome; + + fn deref(&self) -> &DevpodHome { + &self.home + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_create_result_beside_the_record_reads_as_completed() { + let home = devpod_home_with(&[("default", "myws", Some(()))]); + assert_eq!(create_record(Some(&home), "myws"), CreateRecord::Completed); + } + + /// The shape a `postCreateCommand` that exited non-zero leaves behind: devpod + /// wrote the workspace record on the way in and never wrote the result on the + /// way out. Measured against devpod 0.26.1. + #[test] + fn a_record_with_no_result_reads_as_never_completed() { + let home = devpod_home_with(&[("default", "myws", None)]); + assert_eq!( + create_record(Some(&home), "myws"), + CreateRecord::NeverCompleted + ); + } + + /// Three ways to have no answer, and none of them may read as + /// `NeverCompleted` -- each would rebuild a workspace that is perfectly fine. + #[test] + fn nothing_to_read_reads_as_unknown() { + let home = devpod_home_with(&[("default", "other", None)]); + assert_eq!(create_record(None, "myws"), CreateRecord::Unknown); + assert_eq!(create_record(Some(&home), "myws"), CreateRecord::Unknown); + let empty = tempfile::tempdir().expect("an empty home"); + assert_eq!( + create_record(Some(&DevpodHome::at(empty.path())), "myws"), + CreateRecord::Unknown + ); + } + + /// A context directory whose name is not text devlaunch can spell holds no + /// record it could have read anyway, so the walk steps over it and the context + /// that *does* hold one still answers. Losing the answer to it would rebuild a + /// healthy workspace — and, since `flows::lifecycle`'s volume sweep shares this + /// walk, leave that workspace's volumes on disk for a name nothing was going to + /// read. + #[test] + fn a_context_directory_nobody_can_spell_does_not_cost_the_others_their_answer() { + use std::os::unix::ffi::OsStrExt as _; + + let home = devpod_home_with(&[("default", "myws", Some(()))]); + let unspellable = std::ffi::OsStr::from_bytes(b"\xff\xfe-not-utf8").to_os_string(); + std::fs::create_dir_all(home.contexts().join(unspellable)).expect("a context directory"); + + assert_eq!(create_record(Some(&home), "myws"), CreateRecord::Completed); + } + + /// Ids are unique per context, not globally, so one id in two contexts is an + /// ambiguity rather than a finding. Answering from whichever context the + /// directory iteration reached first would rebuild a healthy workspace on the + /// strength of a different context's abandoned one. + #[test] + fn one_id_in_two_contexts_reads_as_unknown() { + let home = devpod_home_with(&[("default", "myws", Some(())), ("work", "myws", None)]); + assert_eq!(create_record(Some(&home), "myws"), CreateRecord::Unknown); + } + + /// A record dl cannot read as one is refused rather than replaced: the file is + /// left exactly as devpod wrote it. + #[test] + fn a_devpod_record_dl_cannot_read_as_one_is_refused_rather_than_replaced() { + let dir = tempfile::tempdir().expect("a scratch home"); + let home = DevpodHome::at(dir.path().join("devpod")); + let path = home.record("default", "ws"); + std::fs::create_dir_all(path.parent().expect("a record directory")) + .expect("a record directory"); + std::fs::write(&path, r#"{"id": "ws", "source": "a string"}"#).expect("a record"); + + assert_eq!( + home.repoint("default", "ws", &dir.path().join("clone")), + Err(RepointFailure::NotADevpodRecord { path: path.clone() }) + ); + assert_eq!( + std::fs::read_to_string(&path).expect("still there"), + r#"{"id": "ws", "source": "a string"}"#, + "a source key dl cannot read is one it cannot safely replace" + ); + } + + /// `DEVPOD_HOME` wins when it names somewhere, and an empty one is not + /// somewhere. Pinned through the pure half so every case can be asserted + /// without mutating an environment the whole test binary shares. + #[test] + fn a_devpod_home_comes_from_the_environment_or_the_home_directory() { + let passwd = || Some(PathBuf::from("/home/someone")); + assert_eq!( + DevpodHome::located(Some(OsString::from("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/elsewhere/devpod")), passwd), + Some(DevpodHome::at("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/elsewhere/devpod")) + ); + assert_eq!( + DevpodHome::located(Some(OsString::new()), passwd), + Some(DevpodHome::at("/home/someone/.devpod")) + ); + assert_eq!( + DevpodHome::located(None, passwd), + Some(DevpodHome::at("/home/someone/.devpod")) + ); + assert_eq!(DevpodHome::located(None, || None), None); + } +} diff --git a/rust/devlaunch-core/src/clients/mod.rs b/rust/devlaunch-core/src/clients/mod.rs index 1aba8e97..ea290674 100644 --- a/rust/devlaunch-core/src/clients/mod.rs +++ b/rust/devlaunch-core/src/clients/mod.rs @@ -1,5 +1,8 @@ // binary surface — not part of the frozen wf API (#251 §7) pub mod devpod; +// devpod-the-filesystem, where `devpod` above is devpod-the-command. +// binary surface — not part of the frozen wf API (#251 §7) +pub mod devpod_home; // Crate-private, unlike its siblings: nothing outside core names docker, and the // one thing devlaunch asks it (remove these volumes) is reported through // `flows::lifecycle`'s own vocabulary rather than docker's. diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index ee49e082..09325265 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -63,6 +63,7 @@ use std::path::{Path, PathBuf}; use std::time::{Duration, SystemTime}; use crate::clients::devpod::{self, Call, ContainerState, ListingUnreadable, NotRun}; +use crate::clients::devpod_home::{CreateRecord, DevpodHome, create_record}; use crate::clients::gh::{self, GhEvent, StagedToken, Token, TokenLookup}; use crate::clients::ssh; use crate::domain::locks::{self, Contention, LockError}; @@ -178,7 +179,7 @@ pub struct Host { /// the context-options cache all hang off this. pub(crate) cache_dir: PathBuf, /// devpod's own home, whose `config.yaml` mtime expires the options cache. - pub(crate) devpod_home: Option, + pub(crate) devpod_home: Option, } impl Host { @@ -199,7 +200,7 @@ impl Host { stderr_tty: is_a_terminal(libc::STDERR_FILENO), ssh_config: ssh::config_path(), cache_dir: cache_dir.into(), - devpod_home: lifecycle::devpod_home(), + devpod_home: DevpodHome::locate(), } } @@ -227,9 +228,7 @@ impl Host { /// devpod's own config file, which holds every context and its options. pub(crate) fn devpod_config(&self) -> Option { - self.devpod_home - .as_ref() - .map(|home| home.join("config.yaml")) + self.devpod_home.as_ref().map(DevpodHome::config) } } @@ -1099,8 +1098,7 @@ fn up_under_stage( && serialization.waited() && !request.wants_more_than_a_running_workspace() && is_running(context.runner(), identity) - && lifecycle::create_record(host.devpod_home.as_deref(), identity) - != lifecycle::CreateRecord::NeverCompleted + && create_record(host.devpod_home.as_ref(), identity) != CreateRecord::NeverCompleted { notices.say(LaunchNotice::BroughtUpBySibling { workspace_id: identity.to_owned(), @@ -2706,16 +2704,13 @@ impl<'a, 'r, 'l> Launch<'a, 'r, 'l> { // unfinished, since `up` is what it was doing, and a host whose devpod // records will not read attaches exactly as it did before. if placement.is_running() { - match lifecycle::create_record( - self.host.devpod_home.as_deref(), - placement.workspace_id(), - ) { - lifecycle::CreateRecord::NeverCompleted => { + match create_record(self.host.devpod_home.as_ref(), placement.workspace_id()) { + CreateRecord::NeverCompleted => { self.notices.say(LaunchNotice::CreateNeverFinished { workspace_id: placement.workspace_id().to_owned(), }); } - lifecycle::CreateRecord::Completed | lifecycle::CreateRecord::Unknown => { + CreateRecord::Completed | CreateRecord::Unknown => { // Whatever brought this workspace up finished before this launch // asked: nothing to build and nothing to wait for. If a prewarm was // fired for it, this is what a prewarm that paid off looks like. @@ -2800,8 +2795,8 @@ impl<'a, 'r, 'l> Launch<'a, 'r, 'l> { // for to fix a workspace, and the documented recovery would be the one path // that declines to run. if placement.is_running() - && lifecycle::create_record(self.host.devpod_home.as_deref(), placement.workspace_id()) - != lifecycle::CreateRecord::NeverCompleted + && create_record(self.host.devpod_home.as_ref(), placement.workspace_id()) + != CreateRecord::NeverCompleted { self.notices.say(LaunchNotice::AlreadyRunning { workspace_id: placement.workspace_id().to_owned(), @@ -3087,7 +3082,7 @@ mod tests { ..gh::HostEnv::default() }, cache_dir: dir.path().to_path_buf(), - devpod_home: Some(dir.path().join("devpod")), + devpod_home: Some(DevpodHome::at(dir.path().join("devpod"))), ..Host::default() }; Self { @@ -3128,10 +3123,9 @@ mod tests { /// the result beside it, which is what devpod writes on its way out of a /// successful `up`. fn with_create_completed(self, workspace_id: &str) -> Self { - let dir = self.devpod_record_dir(workspace_id); - std::fs::create_dir_all(&dir).expect("a devpod record directory"); - std::fs::write(dir.join("workspace.json"), "{}").expect("a workspace record"); - std::fs::write(dir.join("workspace_result.json"), "{}").expect("a create result"); + let home = self.devpod_home(); + self.write_record(workspace_id); + std::fs::write(home.result("default", workspace_id), "{}").expect("a create result"); self } @@ -3139,21 +3133,25 @@ mod tests { /// no result beside it. A `postCreateCommand` that exits non-zero leaves /// exactly this, with the container still up. fn with_create_aborted(self, workspace_id: &str) -> Self { - let dir = self.devpod_record_dir(workspace_id); - std::fs::create_dir_all(&dir).expect("a devpod record directory"); - std::fs::write(dir.join("workspace.json"), "{}").expect("a workspace record"); + self.write_record(workspace_id); self } - fn devpod_record_dir(&self, workspace_id: &str) -> PathBuf { + fn devpod_home(&self) -> &DevpodHome { self.host .devpod_home .as_ref() .expect("a scratch devpod home") - .join("contexts") - .join("default") - .join("workspaces") - .join(workspace_id) + } + + /// The `workspace.json` devpod writes on its way *in*, at whatever path + /// `clients::devpod_home` says it goes — asked rather than rebuilt, so this + /// fixture cannot go on passing after devpod moves its layout. + fn write_record(&self, workspace_id: &str) { + let record = self.devpod_home().record("default", workspace_id); + std::fs::create_dir_all(record.parent().expect("a devpod record directory")) + .expect("a devpod record directory"); + std::fs::write(record, "{}").expect("a workspace record"); } fn cache_dir(&self) -> &Path { diff --git a/rust/devlaunch-core/src/flows/lifecycle.rs b/rust/devlaunch-core/src/flows/lifecycle.rs index 0d77b8c7..34466664 100644 --- a/rust/devlaunch-core/src/flows/lifecycle.rs +++ b/rust/devlaunch-core/src/flows/lifecycle.rs @@ -69,6 +69,7 @@ use crate::clients::devpod::{ self, Call, ContainerState, ListingUnreadable, NotRun, StatusUnreadable, Workspace, WorkspaceSource, }; +use crate::clients::devpod_home::{DevpodHome, RepointFailure, sole_workspace_result}; use crate::clients::docker; use crate::clients::git::Git; use crate::domain::locks::{self, LockError}; @@ -82,7 +83,7 @@ use crate::flows::listing::{ }; use crate::flows::repo_manager::{ BACKGROUND_FETCH_TIMEOUT, CacheNotice, Fetched, LazyFetchError, Refusal, Removal, - RepositoryManager, present, remove_tree_as_far_as_it_goes, system_words, + RepositoryManager, present, remove_tree_as_far_as_it_goes, }; use crate::flows::workspace_clone::{RemoveWorkspaceError, Removed, WorkspaceCloneManager}; use crate::notices::{Notices, Wrapped}; @@ -974,7 +975,7 @@ pub fn workspace_delete( refresh: &mut Refresh<'_>, clones: &WorkspaceCloneManager<'_>, storage: &mut MetadataStorage, - devpod_home: Option<&Path>, + devpod_home: Option<&DevpodHome>, workspace_id: &str, insistence: Insistence, notices: &mut dyn Notices, @@ -1092,7 +1093,7 @@ fn sweep_volumes(runner: &dyn Runner, named: Option>) -> Volume /// as "removed nothing" — an `up` that died in its lifecycle hooks leaves the /// workspace record with no result beside it, which is exactly this case. fn devcontainer_volumes( - devpod_home: Option<&Path>, + devpod_home: Option<&DevpodHome>, workspace_id: &str, ) -> Option> { let result = sole_workspace_result(devpod_home, workspace_id)?; @@ -1315,7 +1316,7 @@ impl PurgeOutcome { pub fn purge_all_data( context: &mut CommandContext<'_>, plan: &PurgePlan, - devpod_home: Option<&Path>, + devpod_home: Option<&DevpodHome>, on_step: &mut dyn FnMut(PurgeStep), ) -> Result { for workspace in &plan.ownership.mine { @@ -2758,231 +2759,6 @@ fn repository_of(source: &Path, root: &Path) -> Option<(String, String)> { Some((owner, repo)) } -/// The directory devpod keeps its own records in. -/// -/// Honours `DEVPOD_HOME` for the same reason the rest of dl does: it is what scopes -/// devpod, and the test suite sets it. -pub fn devpod_home() -> Option { - match std::env::var_os("DEVPOD_HOME") { - Some(home) if !home.is_empty() => Some(PathBuf::from(home)), - _ => crate::osext::home_dir().map(|home| home.join(".devpod")), - } -} - -/// devpod's own record for one workspace. -pub(crate) fn devpod_workspace_record( - devpod_home: &Path, - context: &str, - workspace_id: &str, -) -> PathBuf { - devpod_home - .join("contexts") - .join(context) - .join("workspaces") - .join(workspace_id) - .join("workspace.json") -} - -/// devpod's record of a *completed* create for one workspace. -/// -/// devpod writes this beside [`devpod_workspace_record`] on its way out of a -/// successful `up`, and it is where the container's remote user lives -/// (`.MergedConfig.remoteUser`). A create that died in its lifecycle hooks leaves -/// the record and no result — which is also why `devpod ssh` into one lands as -/// root: there is no recorded user for it to become. -pub(crate) fn devpod_workspace_result( - devpod_home: &Path, - context: &str, - workspace_id: &str, -) -> PathBuf { - devpod_home - .join("contexts") - .join(context) - .join("workspaces") - .join(workspace_id) - .join("workspace_result.json") -} - -/// Whether devpod finished creating a workspace, as far as its own records say. -/// -/// Three arms rather than a bool, for the reason [`crate::domain::workspace_state`] -/// needed three: "no evidence it finished" and "no evidence either way" are -/// different facts, and only the first is a reason to act. A caller that acted on -/// the second would rebuild every workspace on a host whose devpod home it cannot -/// read. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub(crate) enum CreateRecord { - /// devpod wrote its create result, so the workspace was set up. - Completed, - /// devpod holds a record for this workspace and no result beside it: an `up` - /// started and did not finish. - NeverCompleted, - /// Nothing here can answer — no devpod home, no record under any context, a - /// directory that would not read, or one id in two contexts. - Unknown, -} - -/// What devpod's own records say about whether this workspace's create finished. -/// -/// Every context is searched rather than `default` being assumed: ids are unique -/// per context, [`crate::flows::launch::Placement`] carries no context, and asking -/// devpod for one would put a second round trip on the warm attach path this -/// exists to guard. Two contexts holding the same id answer [`CreateRecord::Unknown`] -/// rather than picking one — the ambiguity is the answer. -pub(crate) fn create_record(devpod_home: Option<&Path>, workspace_id: &str) -> CreateRecord { - let Some(home) = devpod_home else { - return CreateRecord::Unknown; - }; - let Some(context) = sole_context_holding(home, workspace_id) else { - return CreateRecord::Unknown; - }; - if devpod_workspace_result(home, &context, workspace_id).is_file() { - CreateRecord::Completed - } else { - CreateRecord::NeverCompleted - } -} - -/// The one context whose records name this workspace, or nothing when the question -/// has no single answer. -/// -/// Nothing for all four ways of not having one: a `contexts` directory that would -/// not read, no context holding the id, two contexts holding it, and a context -/// whose name is not UTF-8 (skipped, because a path this cannot spell is one -/// devpod's own id lookup cannot have written). -/// -/// Split out of [`create_record`] because [`sole_workspace_result`] has to walk the -/// contexts *the same way* and read the same ambiguity out of them: the two answer -/// different questions about one workspace, and a second copy of this loop is how -/// they would come to disagree about which context that workspace is in. -fn sole_context_holding(devpod_home: &Path, workspace_id: &str) -> Option { - let contexts = std::fs::read_dir(devpod_home.join("contexts")).ok()?; - let mut sole: Option = None; - for context in contexts.flatten() { - // The paths are built by the same pair of helpers `--reconcile` uses, so - // devpod's on-disk layout is spelled out in one place and not three. - let Some(name) = context.file_name().to_str().map(str::to_owned) else { - continue; - }; - if !devpod_workspace_record(devpod_home, &name, workspace_id).is_file() { - continue; - } - if sole.is_some() { - return None; - } - sole = Some(name); - } - sole -} - -/// The one `workspace_result.json` this workspace has, if it has exactly one. -/// -/// devpod rewrites that file on its way out of every *completed* `up`, whoever ran -/// the `up` — `dl`, VS Code, a hand-typed `devpod up` — which is what makes its -/// mtime a usable "has this container been rebuilt since?" anchor for -/// [`crate::flows::provision::verdict_cache`]. The record beside it is not: devpod -/// writes `workspace.json` on the way *in* and leaves it alone afterwards, so a -/// container recreated ten times still carries the first create's timestamps. -/// -/// Nothing rather than a guess in every ambiguous case, because the caller's only -/// use for the answer is to *skip* work: no devpod home, no record, two contexts -/// holding the id, or a record with no result beside it (an `up` that died in its -/// hooks) all mean the same thing to it — there is no file whose mtime can be -/// trusted to move when this container is rebuilt, so trust nothing. -pub(crate) fn sole_workspace_result( - devpod_home: Option<&Path>, - workspace_id: &str, -) -> Option { - let home = devpod_home?; - let context = sole_context_holding(home, workspace_id)?; - let result = devpod_workspace_result(home, &context, workspace_id); - result.is_file().then_some(result) -} - -/// Why one devpod record could not be re-pointed. -/// -/// Unreadable and not-JSON are two arms where Python's one `except` clause caught -/// both: an `OSError` and a decode error read the same to its f-string but not to -/// a caller, and one `reason: String` could not say which had happened. Both -/// render through the same sentence, so the split changes no output. -#[derive(Debug, Clone, PartialEq, Eq)] -pub enum RepointFailure { - /// devpod's record's bytes could not be read; `reason` is the OS's words. - Unreadable { path: PathBuf, reason: String }, - /// The bytes are not JSON; `reason` is the parser's words. - NotJson { path: PathBuf, reason: String }, - /// Not the shape this repair understands. Refusing is the whole of the - /// response: a source key dl cannot read is one it cannot safely replace. - NotADevpodRecord { path: PathBuf }, - /// The rewritten record could not be put in place. devpod's own file is left - /// whole: the write goes to a sibling temp file and is renamed over it. - Unwritable { path: PathBuf, reason: String }, -} - -/// Rewrite one devpod workspace record's source folder. -/// -/// **devpod's own file, written directly, and that is a decision rather than a -/// convenience.** devpod v0.26.1 has no subcommand that changes an existing -/// workspace's source: the surface is build, delete, list, logs, ssh, status, stop -/// and up, and the only one that sets a source is a create, which needs a container -/// daemon and would destroy the very record being repaired. So the choice is -/// between one field of one JSON file and not repairing anything. -/// -/// Only `source.localFolder` is touched, and the file is rewritten from what devpod -/// itself last wrote, so every key devpod knows about and dl does not — `uid`, -/// provider options, timestamps — survives untouched. Written through a temporary -/// file in the same directory and renamed over the original, so a failure partway -/// leaves devpod's record whole rather than truncated. -pub(crate) fn repoint_devpod_source( - devpod_home: &Path, - adoptable: &Adoptable, -) -> Result<(), RepointFailure> { - let path = devpod_workspace_record(devpod_home, &adoptable.context, &adoptable.workspace_id); - let text = std::fs::read_to_string(&path).map_err(|error| RepointFailure::Unreadable { - path: path.clone(), - reason: system_words(&error), - })?; - let mut record: serde_json::Value = - serde_json::from_str(&text).map_err(|error| RepointFailure::NotJson { - path: path.clone(), - reason: error.to_string(), - })?; - let Some(source) = record - .as_object_mut() - .and_then(|record| record.get_mut("source")) - .and_then(serde_json::Value::as_object_mut) - else { - return Err(RepointFailure::NotADevpodRecord { path }); - }; - source.insert( - "localFolder".to_owned(), - serde_json::Value::String(adoptable.clone.display().to_string()), - ); - // Two-space indentation, as devpod and Python's `json.dumps(…, indent=2)` both - // write it: a file two tools take turns rewriting should not churn its shape. - let rewritten = - serde_json::to_string_pretty(&record).map_err(|error| RepointFailure::Unwritable { - path: path.clone(), - reason: error.to_string(), - })?; - let temp = path.with_extension("dl-tmp"); - if let Err(error) = std::fs::write(&temp, rewritten) { - let _ = std::fs::remove_file(&temp); - return Err(RepointFailure::Unwritable { - path, - reason: system_words(&error), - }); - } - if let Err(error) = std::fs::rename(&temp, &path) { - let _ = std::fs::remove_file(&temp); - return Err(RepointFailure::Unwritable { - path, - reason: system_words(&error), - }); - } - Ok(()) -} - /// What became of one of the plan's adoptions. /// /// Each carries its own ending, so a caller reads how an adoption went straight @@ -3049,13 +2825,17 @@ pub fn apply_reconciliation( context: &mut CommandContext<'_>, refresh: &mut Refresh<'_>, storage: &mut MetadataStorage, - devpod_home: &Path, + devpod_home: &DevpodHome, plan: &ReconcilePlan, notices: &mut dyn Notices, ) -> ReconcileReport { let mut adoptions = Vec::new(); for adoptable in &plan.adopting { - if let Err(failure) = repoint_devpod_source(devpod_home, adoptable) { + if let Err(failure) = devpod_home.repoint( + &adoptable.context, + &adoptable.workspace_id, + &adoptable.clone, + ) { adoptions.push(Adoption::Refused { workspace_id: adoptable.workspace_id.clone(), failure, @@ -3084,7 +2864,7 @@ pub fn apply_reconciliation( } #[cfg(test)] -pub(crate) mod tests { +mod tests { //! What the lifecycle commands do, at three seams. //! //! **The argv seam.** Every devpod call's whole argv, through the fake runner, @@ -3134,6 +2914,7 @@ pub(crate) mod tests { use devlaunch_test_support::{FakeRunner, Response}; use super::*; + use crate::clients::devpod_home::{ScratchHome, devpod_home_with}; use crate::clients::{devpod, docker}; use crate::domain::metadata::MetadataStorage; use crate::domain::model::{BaseRepository, Timestamp, WorktreeInfo}; @@ -3142,33 +2923,6 @@ pub(crate) mod tests { use crate::flows::repo_manager::{RefusalReason, RemoveTreeError, bare_dir}; use crate::flows::workspace_clone::GitLfs; - /// One devpod home holding whatever these workspaces were left holding. - /// `Some(())` writes the create result beside the record; `None` leaves the - /// record alone, which is what an aborted `up` leaves behind. - /// - /// Shared with `flows::provision`'s tests rather than copied into them: the - /// layout it builds is devpod's, spelled here beside the helpers that read it - /// ([`devpod_workspace_record`], [`devpod_workspace_result`]), and a second - /// copy under another module is a fixture that can drift out of agreement with - /// the code it is a fixture for. - pub(crate) fn devpod_home_with(entries: &[(&str, &str, Option<()>)]) -> tempfile::TempDir { - let home = tempfile::tempdir().expect("a scratch devpod home"); - for (context, workspace_id, result) in entries { - let dir = home - .path() - .join("contexts") - .join(context) - .join("workspaces") - .join(workspace_id); - std::fs::create_dir_all(&dir).expect("a record directory"); - std::fs::write(dir.join("workspace.json"), "{}").expect("a record"); - if result.is_some() { - std::fs::write(dir.join("workspace_result.json"), "{}").expect("a result"); - } - } - home - } - /// A devpod home whose create result for `workspace_id` records what devpod /// substituted into that workspace's devcontainer. /// @@ -3181,9 +2935,9 @@ pub(crate) mod tests { workspace_id: &str, local_workspace_folder: &str, devcontainer_id: &str, - ) -> tempfile::TempDir { + ) -> ScratchHome { let home = devpod_home_with(&[("default", workspace_id, Some(()))]); - let result = devpod_workspace_result(home.path(), "default", workspace_id); + let result = home.result("default", workspace_id); std::fs::write( &result, serde_json::json!({ @@ -3201,77 +2955,6 @@ pub(crate) mod tests { home } - #[test] - fn a_create_result_beside_the_record_reads_as_completed() { - let home = devpod_home_with(&[("default", "myws", Some(()))]); - assert_eq!( - create_record(Some(home.path()), "myws"), - CreateRecord::Completed - ); - } - - /// The shape a `postCreateCommand` that exited non-zero leaves behind: devpod - /// wrote the workspace record on the way in and never wrote the result on the - /// way out. Measured against devpod 0.26.1. - #[test] - fn a_record_with_no_result_reads_as_never_completed() { - let home = devpod_home_with(&[("default", "myws", None)]); - assert_eq!( - create_record(Some(home.path()), "myws"), - CreateRecord::NeverCompleted - ); - } - - /// Three ways to have no answer, and none of them may read as - /// `NeverCompleted` -- each would rebuild a workspace that is perfectly fine. - #[test] - fn nothing_to_read_reads_as_unknown() { - let home = devpod_home_with(&[("default", "other", None)]); - assert_eq!(create_record(None, "myws"), CreateRecord::Unknown); - assert_eq!( - create_record(Some(home.path()), "myws"), - CreateRecord::Unknown - ); - let empty = tempfile::tempdir().expect("an empty home"); - assert_eq!( - create_record(Some(empty.path()), "myws"), - CreateRecord::Unknown - ); - } - - /// A context directory whose name is not text devlaunch can spell holds no - /// record it could have read anyway, so the walk steps over it and the context - /// that *does* hold one still answers. Losing the answer to it would rebuild a - /// healthy workspace — and, since [`devcontainer_volumes`] shares this walk, - /// leave that workspace's volumes on disk for a name nothing was going to read. - #[test] - fn a_context_directory_nobody_can_spell_does_not_cost_the_others_their_answer() { - use std::os::unix::ffi::OsStrExt as _; - - let home = devpod_home_with(&[("default", "myws", Some(()))]); - let unspellable = std::ffi::OsStr::from_bytes(b"\xff\xfe-not-utf8").to_os_string(); - std::fs::create_dir_all(home.path().join("contexts").join(unspellable)) - .expect("a context directory"); - - assert_eq!( - create_record(Some(home.path()), "myws"), - CreateRecord::Completed - ); - } - - /// Ids are unique per context, not globally, so one id in two contexts is an - /// ambiguity rather than a finding. Answering from whichever context the - /// directory iteration reached first would rebuild a healthy workspace on the - /// strength of a different context's abandoned one. - #[test] - fn one_id_in_two_contexts_reads_as_unknown() { - let home = devpod_home_with(&[("default", "myws", Some(())), ("work", "myws", None)]); - assert_eq!( - create_record(Some(home.path()), "myws"), - CreateRecord::Unknown - ); - } - #[test] fn a_devpod_that_cannot_be_run_fails_the_stage_but_one_that_refuses_does_not() { // Python's `@timing.staged("devpod-up") get_workspace_state` returns None @@ -4339,7 +4022,7 @@ pub(crate) mod tests { let mut context = CommandContext::new(&devpod); let plan = purge_plan(&mut context, &cache).expect("a plan"); - purge_all_data(&mut context, &plan, Some(home.path()), &mut |_| {}).expect("devpod ran"); + purge_all_data(&mut context, &plan, Some(&home), &mut |_| {}).expect("devpod ran"); assert_eq!( devpod.docker_argvs(), @@ -4375,7 +4058,7 @@ pub(crate) mod tests { let mut context = CommandContext::new(&devpod); let plan = purge_plan(&mut context, &cache).expect("a plan"); - purge_all_data(&mut context, &plan, Some(home.path()), &mut |_| {}).expect("devpod ran"); + purge_all_data(&mut context, &plan, Some(&home), &mut |_| {}).expect("devpod ran"); assert_eq!(devpod.docker_argvs(), Vec::>::new()); } @@ -4397,7 +4080,7 @@ pub(crate) mod tests { let mut context = CommandContext::new(&devpod); let plan = purge_plan(&mut context, &cache).expect("a plan"); - purge_all_data(&mut context, &plan, Some(home.path()), &mut |step| { + purge_all_data(&mut context, &plan, Some(&home), &mut |step| { steps.push(step) }) .expect("devpod ran"); @@ -4436,7 +4119,7 @@ pub(crate) mod tests { let mut context = CommandContext::new(&devpod); let plan = purge_plan(&mut context, &cache).expect("a plan"); - let outcome = purge_all_data(&mut context, &plan, Some(home.path()), &mut |step| { + let outcome = purge_all_data(&mut context, &plan, Some(&home), &mut |step| { steps.push(step) }) .expect("devpod ran"); @@ -5059,7 +4742,7 @@ pub(crate) mod tests { /// a test that made them the same could not tell the two sources apart. struct Deleting { world: World, - home: tempfile::TempDir, + home: ScratchHome, updater: SelfInvocation, cache_path: PathBuf, } @@ -5091,7 +4774,7 @@ pub(crate) mod tests { &mut refresh, &clones, &mut self.world.storage, - Some(self.home.path()), + Some(&self.home), "r-main-aa", Insistence::NotInsisted, &mut notices, @@ -5285,17 +4968,16 @@ pub(crate) mod tests { fn one_id_in_two_contexts_names_nothing() { for second_up_finished in [false, true] { let home = devpod_home_recording("myws", "/host/clones/opened-as", "dc9a8b7c"); - let record = devpod_workspace_record(home.path(), "work", "myws"); + let record = home.record("work", "myws"); std::fs::create_dir_all(record.parent().expect("a parent")) .expect("a record directory"); std::fs::write(&record, "{}").expect("a record"); if second_up_finished { - std::fs::write(devpod_workspace_result(home.path(), "work", "myws"), "{}") - .expect("a create result"); + std::fs::write(home.result("work", "myws"), "{}").expect("a create result"); } assert!( - devcontainer_volumes(Some(home.path()), "myws").is_none(), + devcontainer_volumes(Some(&home), "myws").is_none(), "second context finished its up: {second_up_finished}" ); } @@ -7117,8 +6799,8 @@ pub(crate) mod tests { } /// devpod's own record for a workspace, in devpod's on-disk shape. - fn devpod_record(devpod_home: &Path, workspace_id: &str, source: &Path) -> PathBuf { - let path = devpod_workspace_record(devpod_home, "default", workspace_id); + fn devpod_record(devpod_home: &DevpodHome, workspace_id: &str, source: &Path) -> PathBuf { + let path = devpod_home.record("default", workspace_id); std::fs::create_dir_all(path.parent().expect("a parent")).expect("the record directory"); std::fs::write( &path, @@ -7176,7 +6858,7 @@ pub(crate) mod tests { // The join is by path and never by id: the id is what the scheme change moved, // and the source path devpod kept still names owner, repo and branch. let mut world = World::empty(); - let devpod_home = world.tmp().join("devpod"); + let devpod_home = DevpodHome::at(world.tmp().join("devpod")); let clone = a_bare_clone_directory(&world.repo_dir.join("r-feature-auth-aaa")); world.record("r-feature-auth-aaa", "feature/auth", &clone); let old = world.repo_dir.join("feature-auth"); @@ -7201,7 +6883,7 @@ pub(crate) mod tests { #[test] fn applying_a_plan_repoints_devpods_own_record_and_keeps_its_other_keys() { let mut world = World::empty(); - let devpod_home = world.tmp().join("devpod"); + let devpod_home = DevpodHome::at(world.tmp().join("devpod")); let clone = a_bare_clone_directory(&world.repo_dir.join("r-feature-auth-aaa")); world.record("r-feature-auth-aaa", "feature/auth", &clone); let old = world.repo_dir.join("feature-auth"); @@ -7253,7 +6935,7 @@ pub(crate) mod tests { // A wrongly-adopted workspace costs a rebuild; a wrongly-deleted one costs // whatever was in it. let mut world = World::empty(); - let devpod_home = world.tmp().join("devpod"); + let devpod_home = DevpodHome::at(world.tmp().join("devpod")); let old = world.repo_dir.join("no-such-clone"); devpod_record(&devpod_home, "ws-old", &old); world.devpod.lists(&[listed("ws-old", &old)]); @@ -7288,7 +6970,7 @@ pub(crate) mod tests { // both the directory `feature-auth`, so one devpod record can name two // branches' clones and a map would hand it whichever was written last. let mut world = World::empty(); - let devpod_home = world.tmp().join("devpod"); + let devpod_home = DevpodHome::at(world.tmp().join("devpod")); let slash = a_bare_clone_directory(&world.repo_dir.join("r-feature-auth-aaa")); let colon = a_bare_clone_directory(&world.repo_dir.join("r-feature-auth-bbb")); world.record("r-feature-auth-aaa", "feature/auth", &slash); @@ -7311,7 +6993,7 @@ pub(crate) mod tests { // Picking one would be a coin flip decided by listing order, and the loser // would still be broken with nothing said about why. let mut world = World::empty(); - let devpod_home = world.tmp().join("devpod"); + let devpod_home = DevpodHome::at(world.tmp().join("devpod")); let clone = a_bare_clone_directory(&world.repo_dir.join("r-main-aaa")); world.record("r-main-aaa", "main", &clone); let old = world.repo_dir.join("main"); @@ -7341,7 +7023,7 @@ pub(crate) mod tests { // Adopting it would point two workspaces at one directory and leave the working // one sharing its checkout with a dead one. let mut world = World::empty(); - let devpod_home = world.tmp().join("devpod"); + let devpod_home = DevpodHome::at(world.tmp().join("devpod")); let clone = a_bare_clone_directory(&world.repo_dir.join("r-main-aaa")); world.record("r-main-aaa", "main", &clone); let old = world.repo_dir.join("main"); @@ -7365,7 +7047,7 @@ pub(crate) mod tests { #[test] fn running_it_twice_changes_nothing() { let mut world = World::empty(); - let devpod_home = world.tmp().join("devpod"); + let devpod_home = DevpodHome::at(world.tmp().join("devpod")); let clone = a_bare_clone_directory(&world.repo_dir.join("r-main-aaa")); world.record("r-main-aaa", "main", &clone); let old = world.repo_dir.join("main"); @@ -7407,7 +7089,7 @@ pub(crate) mod tests { // devpod's record is re-pointed first and metadata's id written second, and the // failure of the first must leave the second alone. let mut world = World::empty(); - let devpod_home = world.tmp().join("devpod"); + let devpod_home = DevpodHome::at(world.tmp().join("devpod")); let clone = a_bare_clone_directory(&world.repo_dir.join("r-main-aaa")); world.record("r-main-aaa", "main", &clone); let old = world.repo_dir.join("main"); @@ -7444,38 +7126,4 @@ pub(crate) mod tests { "the id is written only once devpod's record says the same thing" ); } - - #[test] - fn a_devpod_record_dl_cannot_read_as_one_is_refused_rather_than_replaced() { - let dir = temp_dir(); - let devpod_home = dir.path().join("devpod"); - let path = devpod_workspace_record(&devpod_home, "default", "ws"); - std::fs::create_dir_all(path.parent().expect("a parent")).expect("the directory"); - std::fs::write(&path, r#"{"id": "ws", "source": "a string"}"#).expect("a record"); - let adoptable = Adoptable { - workspace_id: "ws".to_owned(), - context: "default".to_owned(), - sourced_at: "/gone".to_owned(), - clone: dir.path().join("clone"), - record: WorktreeInfo::new(OWNER, REPO, "main", dir.path().join("clone"), "r-main-aa"), - }; - - assert_eq!( - repoint_devpod_source(&devpod_home, &adoptable), - Err(RepointFailure::NotADevpodRecord { path: path.clone() }) - ); - assert_eq!( - std::fs::read_to_string(&path).expect("still there"), - r#"{"id": "ws", "source": "a string"}"#, - "a source key dl cannot read is one it cannot safely replace" - ); - } - - #[test] - fn a_devpod_home_comes_from_the_environment_or_the_home_directory() { - // Honoured for the same reason the rest of dl honours it: it is what scopes - // devpod, and the suite sets it. - let home = devpod_home().expect("this machine has a home directory"); - assert!(home.is_absolute(), "{home:?}"); - } } diff --git a/rust/devlaunch-core/src/flows/provision.rs b/rust/devlaunch-core/src/flows/provision.rs index 8c7b3ba0..6e3e04e1 100644 --- a/rust/devlaunch-core/src/flows/provision.rs +++ b/rust/devlaunch-core/src/flows/provision.rs @@ -4735,7 +4735,7 @@ fi /// A devpod home, a cache, and the verdict cache over the two of them. struct Remembering { - devpod_home: tempfile::TempDir, + devpod_home: crate::clients::devpod_home::ScratchHome, cache: tempfile::TempDir, } @@ -4743,7 +4743,7 @@ fi /// One workspace, created and finished, as devpod's records leave it. fn new() -> Self { Self { - devpod_home: crate::flows::lifecycle::tests::devpod_home_with(&[( + devpod_home: crate::clients::devpod_home::devpod_home_with(&[( "default", "myws", Some(()), @@ -4755,16 +4755,15 @@ fi fn verdicts(&self) -> VerdictCache { VerdictCache::under( self.cache.path(), - Some(self.devpod_home.path().to_path_buf()), + Some(crate::clients::devpod_home::DevpodHome::at( + self.devpod_home.path(), + )), ) } /// devpod completing another `up`, which is a rewritten result file. fn brought_up_again(&self) { - let result = self - .devpod_home - .path() - .join("contexts/default/workspaces/myws/workspace_result.json"); + let result = self.devpod_home.result("default", "myws"); let file = std::fs::OpenOptions::new() .write(true) .open(&result) diff --git a/rust/devlaunch-core/src/flows/provision/verdict_cache.rs b/rust/devlaunch-core/src/flows/provision/verdict_cache.rs index 276a0719..8ed74bb9 100644 --- a/rust/devlaunch-core/src/flows/provision/verdict_cache.rs +++ b/rust/devlaunch-core/src/flows/provision/verdict_cache.rs @@ -65,7 +65,7 @@ use serde::{Deserialize, Serialize}; use super::{Switches, ToolsSwitch, ZellijSwitch}; -use crate::flows::lifecycle; +use crate::clients::devpod_home::{DevpodHome, sole_workspace_result}; /// The directory the markers live in, under devlaunch's cache directory. const MARKERS_DIR: &str = "tool-verdicts"; @@ -84,12 +84,12 @@ const MARKERS_DIR: &str = "tool-verdicts"; #[derive(Clone, Debug, PartialEq, Eq)] pub struct VerdictCache { markers_dir: PathBuf, - devpod_home: Option, + devpod_home: Option, } impl VerdictCache { /// The cache devlaunch keeps under `cache_dir`, checked against `devpod_home`. - pub fn under(cache_dir: &Path, devpod_home: Option) -> Self { + pub fn under(cache_dir: &Path, devpod_home: Option) -> Self { Self { markers_dir: cache_dir.join(MARKERS_DIR), devpod_home, @@ -123,9 +123,7 @@ impl VerdictCache { if marker.switches != MarkerSwitches::of(switches) { return false; } - let Some(result) = - lifecycle::sole_workspace_result(self.devpod_home.as_deref(), workspace_id) - else { + let Some(result) = sole_workspace_result(self.devpod_home.as_ref(), workspace_id) else { return false; }; Stamp::of(&result) == Some(marker.result_mtime) @@ -143,7 +141,7 @@ impl VerdictCache { /// longer matches, so the next launch travels — one redundant round trip, which /// is the direction that is allowed to be wrong. pub(crate) fn observe(&self, workspace_id: &str) -> Option { - let result = lifecycle::sole_workspace_result(self.devpod_home.as_deref(), workspace_id)?; + let result = sole_workspace_result(self.devpod_home.as_ref(), workspace_id)?; Stamp::of(&result).map(Observed) } @@ -310,20 +308,19 @@ mod tests { use std::time::Duration; use super::*; - use crate::flows::lifecycle::tests::devpod_home_with; + use crate::clients::devpod_home::devpod_home_with; /// A cache directory nothing else writes to. fn cache() -> tempfile::TempDir { tempfile::tempdir().expect("a scratch cache directory") } - /// The result file `devpod_home_with` wrote for this workspace. - fn result_in(home: &Path, workspace_id: &str) -> PathBuf { - home.join("contexts") - .join("default") - .join("workspaces") - .join(workspace_id) - .join("workspace_result.json") + /// The result file `devpod_home_with` wrote for this workspace, asked of the + /// home itself rather than rebuilt: devpod's layout is + /// `clients::devpod_home`'s to spell, and a fixture that spelled its own copy + /// would go on passing after devpod moved it. + fn result_in(home: &DevpodHome, workspace_id: &str) -> PathBuf { + home.result("default", workspace_id) } /// The pair the flow always calls together: observe the container, then record @@ -354,7 +351,7 @@ mod tests { // failure is permanent for the life of the container and completely silent. let home = devpod_home_with(&[("default", "ws", Some(()))]); let cache = cache(); - let verdicts = VerdictCache::under(cache.path(), Some(home.path().to_path_buf())); + let verdicts = VerdictCache::under(cache.path(), Some(DevpodHome::at(home.path()))); let no_zellij = Switches { tools: ToolsSwitch::Install, @@ -382,7 +379,7 @@ mod tests { // already exists. let home = devpod_home_with(&[("default", "ws", Some(()))]); let cache = cache(); - let verdicts = VerdictCache::under(cache.path(), Some(home.path().to_path_buf())); + let verdicts = VerdictCache::under(cache.path(), Some(DevpodHome::at(home.path()))); recorded(&verdicts, "ws"); assert!(verdicts.trusted("ws", Switches::INSTALLING)); @@ -423,7 +420,7 @@ mod tests { fn a_recorded_verdict_is_trusted_while_the_container_stands() { let home = devpod_home_with(&[("default", "myws", Some(()))]); let cache = cache(); - let verdicts = VerdictCache::under(cache.path(), Some(home.path().to_path_buf())); + let verdicts = VerdictCache::under(cache.path(), Some(DevpodHome::at(home.path()))); assert!( !verdicts.trusted("myws", Switches::INSTALLING), @@ -434,7 +431,7 @@ mod tests { assert!(verdicts.trusted("myws", Switches::INSTALLING)); // And it survives being read by a second value over the same directories, // which is what "the next `dl` process" is. - let later = VerdictCache::under(cache.path(), Some(home.path().to_path_buf())); + let later = VerdictCache::under(cache.path(), Some(DevpodHome::at(home.path()))); assert!(later.trusted("myws", Switches::INSTALLING)); } @@ -445,7 +442,7 @@ mod tests { // names and the verdict word are pinned rather than left to serde's whim. let home = devpod_home_with(&[("default", "myws", Some(()))]); let cache = cache(); - let verdicts = VerdictCache::under(cache.path(), Some(home.path().to_path_buf())); + let verdicts = VerdictCache::under(cache.path(), Some(DevpodHome::at(home.path()))); recorded(&verdicts, "myws"); @@ -464,11 +461,11 @@ mod tests { // that no longer matches. let home = devpod_home_with(&[("default", "myws", Some(()))]); let cache = cache(); - let verdicts = VerdictCache::under(cache.path(), Some(home.path().to_path_buf())); + let verdicts = VerdictCache::under(cache.path(), Some(DevpodHome::at(home.path()))); recorded(&verdicts, "myws"); assert!(verdicts.trusted("myws", Switches::INSTALLING)); - rewritten(&result_in(home.path(), "myws"), Duration::from_secs(30)); + rewritten(&result_in(&home, "myws"), Duration::from_secs(30)); assert!(!verdicts.trusted("myws", Switches::INSTALLING)); } @@ -480,12 +477,12 @@ mod tests { // and it is still not the container the verdict was about. let home = devpod_home_with(&[("default", "myws", Some(()))]); let cache = cache(); - let verdicts = VerdictCache::under(cache.path(), Some(home.path().to_path_buf())); + let verdicts = VerdictCache::under(cache.path(), Some(DevpodHome::at(home.path()))); recorded(&verdicts, "myws"); let file = std::fs::OpenOptions::new() .write(true) - .open(result_in(home.path(), "myws")) + .open(result_in(&home, "myws")) .expect("the result file"); let was = file .metadata() @@ -507,7 +504,7 @@ mod tests { // held by two devpod contexts at once. let home = devpod_home_with(&[("default", "myws", Some(()))]); let cache = cache(); - let verdicts = VerdictCache::under(cache.path(), Some(home.path().to_path_buf())); + let verdicts = VerdictCache::under(cache.path(), Some(DevpodHome::at(home.path()))); recorded(&verdicts, "myws"); let marker = cache.path().join(MARKERS_DIR).join("myws.json"); let good = std::fs::read_to_string(&marker).expect("a marker"); @@ -537,7 +534,7 @@ mod tests { #[test] fn an_ambiguous_or_unfindable_workspace_is_not_trusted() { - // The three ways `lifecycle::sole_workspace_result` declines to answer: two + // The three ways `sole_workspace_result` declines to answer: two // contexts holding one id, a record with no result beside it (an `up` that // died in its hooks), and no devpod home to look in at all. Each leaves the // host with no file whose mtime it can key on. @@ -547,8 +544,8 @@ mod tests { let cache = cache(); for home in [ - Some(ambiguous.path().to_path_buf()), - Some(unfinished.path().to_path_buf()), + Some(DevpodHome::at(ambiguous.path())), + Some(DevpodHome::at(unfinished.path())), None, ] { let verdicts = VerdictCache::under(cache.path(), home.clone()); @@ -564,7 +561,7 @@ mod tests { // that a result file appearing later would silently validate. let home = devpod_home_with(&[("default", "myws", None)]); let cache = cache(); - let verdicts = VerdictCache::under(cache.path(), Some(home.path().to_path_buf())); + let verdicts = VerdictCache::under(cache.path(), Some(DevpodHome::at(home.path()))); recorded(&verdicts, "myws"); @@ -578,7 +575,7 @@ mod tests { ("default", "other", Some(())), ]); let cache = cache(); - let verdicts = VerdictCache::under(cache.path(), Some(home.path().to_path_buf())); + let verdicts = VerdictCache::under(cache.path(), Some(DevpodHome::at(home.path()))); recorded(&verdicts, "myws"); @@ -591,7 +588,7 @@ mod tests { // The round trip the equality check depends on: what `Stamp::of` reads back // is the instant the file carries, to the nanosecond the filesystem kept. let home = devpod_home_with(&[("default", "myws", Some(()))]); - let result = result_in(home.path(), "myws"); + let result = result_in(&home, "myws"); let stamp = Stamp::of(&result).expect("an mtime"); let modified = std::fs::metadata(&result) diff --git a/rust/devlaunch-core/tests/devpod_layout.rs b/rust/devlaunch-core/tests/devpod_layout.rs new file mode 100644 index 00000000..18e094b0 --- /dev/null +++ b/rust/devlaunch-core/tests/devpod_layout.rs @@ -0,0 +1,107 @@ +//! devpod's on-disk layout, spelled in one module and nowhere else. +//! +//! `clients::devpod` is the seam for devpod-the-*command*; `clients::devpod_home` +//! is the seam for devpod-the-*filesystem*. Everything under +//! `/contexts//workspaces//` is that second module's +//! knowledge: which directory a record is in, what the result beside it is called, +//! and how the contexts are walked to find either. +//! +//! This is a guard rather than a behaviour test, because the failure it catches is +//! not a wrong answer — it is a second copy of the layout that agrees with the +//! first until devpod changes it. Before this test there were eight copies inside +//! `devlaunch-core`: three in `flows::lifecycle`'s implementation, one in +//! `flows::provision::verdict_cache`, and four more in test fixtures, each +//! reproducing the convention literally. `flows/lifecycle.rs` carried a comment +//! claiming the layout was "spelled out in one place and not three" while it was +//! spelled in four. +//! +//! Scoped to `devlaunch-core/src`. `dl`'s end-to-end tests spell devpod's paths on +//! purpose: they stand outside the crate and check that what dl wrote landed where +//! devpod will look for it, which is exactly the assertion that must not be routed +//! through the code under test. + +use std::path::{Path, PathBuf}; + +/// The module allowed to name devpod's record layout. +const ADAPTER: &str = "clients/devpod_home.rs"; + +/// How the layout gets spelled: a string literal whose first component is +/// `contexts`, either on its own (`.join("contexts")`) or as the head of a joined +/// path (`"contexts/default/workspaces/myws/workspace_result.json"`). +/// +/// Prose is not a spelling — a doc comment describing the layout costs nothing +/// when devpod moves it, so neither pattern matches unquoted text. Nor does +/// devpod's `config.yaml`, whose top-level `contexts:` key is a different fact +/// about a different file. +const SPELLINGS: [&str; 2] = ["\"contexts\"", "\"contexts/"]; + +fn crate_src() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")).join("src") +} + +/// Every `.rs` file under `src`, relative to it, sorted. +fn sources(root: &Path) -> Vec { + let mut found = Vec::new(); + walk(root, root, &mut found); + found.sort(); + found +} + +fn walk(root: &Path, dir: &Path, found: &mut Vec) { + for entry in std::fs::read_dir(dir).expect("a readable source directory") { + let path = entry.expect("a readable directory entry").path(); + if path.is_dir() { + walk(root, &path, found); + } else if path.extension().is_some_and(|ext| ext == "rs") { + found.push( + path.strip_prefix(root) + .expect("a path under the source root") + .to_path_buf(), + ); + } + } +} + +/// Which files spell the layout, and on which lines. +fn spellings_in(root: &Path) -> Vec<(PathBuf, usize)> { + let mut found = Vec::new(); + for relative in sources(root) { + let text = std::fs::read_to_string(root.join(&relative)).expect("a readable source file"); + for (index, line) in text.lines().enumerate() { + if SPELLINGS.iter().any(|spelling| line.contains(spelling)) { + found.push((relative.clone(), index + 1)); + } + } + } + found +} + +#[test] +fn only_the_devpod_home_adapter_spells_devpods_on_disk_layout() { + let root = crate_src(); + let strays: Vec = spellings_in(&root) + .into_iter() + .filter(|(file, _)| file != Path::new(ADAPTER)) + .map(|(file, line)| format!(" src/{}:{line}", file.display())) + .collect(); + assert!( + strays.is_empty(), + "devpod's on-disk layout is spelled outside {ADAPTER}:\n{}", + strays.join("\n") + ); +} + +/// The other half, so the guard cannot pass by the layout having been spelled +/// nowhere at all — which is how a guard like this quietly stops guarding. +#[test] +fn the_devpod_home_adapter_does_spell_it() { + let root = crate_src(); + let inside = spellings_in(&root) + .into_iter() + .filter(|(file, _)| file == Path::new(ADAPTER)) + .count(); + assert!( + inside > 0, + "no module spells devpod's on-disk layout, so this guard is guarding nothing" + ); +} diff --git a/rust/dl/src/commands.rs b/rust/dl/src/commands.rs index 52b07493..f7f3bb8e 100644 --- a/rust/dl/src/commands.rs +++ b/rust/dl/src/commands.rs @@ -10,6 +10,7 @@ use std::io::Write as _; use std::path::Path; use devlaunch_core::clients::devpod::{ListingUnreadable, NotRun}; +use devlaunch_core::clients::devpod_home::DevpodHome; use devlaunch_core::domain::spec::DevcontainerPath; use devlaunch_core::flows::completion::{self, FileState, InstallError, Installed, RcChange}; use devlaunch_core::flows::completion_cache::{self, Refreshed}; @@ -560,7 +561,7 @@ fn render_workspace<'r>( /// A `devpod up` that dies in `postCreateCommand` is the case that matters, and the /// one an earlier draft of this got wrong by keying on the exit code. It leaves the /// container **running**, devpod's record written and the clone cut — which is why -/// [`lifecycle::create_record`] exists at all — so an unattended +/// `clients::devpod_home::create_record` exists at all — so an unattended /// `dl owner/repo --rm -- make test` against a broken devcontainer would leak /// precisely the workspace the flag was reached for. A session devpod refused /// outright, and an OpenSSH that is not installed, leave the same thing behind and @@ -719,7 +720,7 @@ fn render_remove<'r>( // answer is: the process that knows what its environment says hands the // answer down. `None` is a machine with no home directory, where devpod has // no records to read and so no volume names to derive. - lifecycle::devpod_home().as_deref(), + DevpodHome::locate().as_ref(), &workspace_id, insistence, &mut notices, @@ -794,8 +795,8 @@ fn purge_devlaunch_data(context: &mut CommandContext<'_>, cache: &Path, yes: boo } // Read after the question, because a purge that was declined asks devpod // nothing and reads nothing. - let devpod_home = lifecycle::devpod_home(); - let purged = lifecycle::purge_all_data(context, &plan, devpod_home.as_deref(), &mut |step| { + let devpod_home = DevpodHome::locate(); + let purged = lifecycle::purge_all_data(context, &plan, devpod_home.as_ref(), &mut |step| { match render::purge_step(&step) { // Said before the round trip that may take a while, which is why this // is a callback and not a report: "Deleting workspace X" assembled @@ -1013,7 +1014,7 @@ fn render_reconcile( println!("Aborted."); return Ending::Done; } - let Some(devpod_home) = lifecycle::devpod_home() else { + let Some(devpod_home) = DevpodHome::locate() else { return refuse_startup(&StartupError::NoHomeDirectory); }; let Records { storage, .. } = &mut records; diff --git a/rust/dl/src/launch.rs b/rust/dl/src/launch.rs index 79d88ae3..66738fa0 100644 --- a/rust/dl/src/launch.rs +++ b/rust/dl/src/launch.rs @@ -32,13 +32,14 @@ use std::path::Path; +use devlaunch_core::clients::devpod_home::DevpodHome; use devlaunch_core::domain::spec::DevcontainerPath; use devlaunch_core::flows::completion_cache; use devlaunch_core::flows::launch::{ self, Host, Launch, LaunchAborted, LaunchRefusal, LaunchVerb, Launched, Plan, Provision, Session, }; -use devlaunch_core::flows::lifecycle::{self, Refresh}; +use devlaunch_core::flows::lifecycle::Refresh; use devlaunch_core::flows::listing::CommandContext; use devlaunch_core::flows::provision::verdict_cache::VerdictCache; use devlaunch_core::flows::provision::{ @@ -140,7 +141,7 @@ impl ToolProvisioning { // A `None` devpod home here means something else again: no file to // check a remembered verdict against, so nothing is ever trusted and // every pass travels, exactly as it did before the cache existed. - verdicts: VerdictCache::under(cache, lifecycle::devpod_home()), + verdicts: VerdictCache::under(cache, DevpodHome::locate()), } } } @@ -258,8 +259,7 @@ pub(crate) enum Reached { /// attached to. A devpod workspace, a container, and a clone may all exist, /// **including when the `up` failed**: a create that dies in its lifecycle hooks /// leaves the container running and devpod's record written, which is the whole - /// reason [`lifecycle::create_record`](devlaunch_core::flows::lifecycle::create_record) - /// exists. + /// reason `clients::devpod_home::create_record` exists. TheWorkspace, /// The launch stopped before devpod was asked for anything: an unsafe spec, a /// workspace nothing answers to, a default branch that could not be named, a diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 9e05d72a..1be6dde2 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -10,6 +10,7 @@ use std::io; use std::path::Path; use devlaunch_core::clients::devpod::{ListingUnreadable, NotAListing, NotRun}; +use devlaunch_core::clients::devpod_home::RepointFailure; use devlaunch_core::clients::gh::{GhEvent, GhUnavailable}; use devlaunch_core::clients::ssh::{NotRun as SshNotRun, UnsafeRequest}; use devlaunch_core::domain::config; @@ -26,8 +27,7 @@ use devlaunch_core::flows::launch::{ }; use devlaunch_core::flows::lifecycle::{ KeptBecause, LifecycleNotice, NotAdopted, Objection, Promotion, PrunePlan, PruneReport, - PurgeOutcome, PurgePlan, PurgeStep, ReconcilePlan, RemovalRefused, RepointFailure, Unlocatable, - VolumeRefusal, + PurgeOutcome, PurgePlan, PurgeStep, ReconcilePlan, RemovalRefused, Unlocatable, VolumeRefusal, }; use devlaunch_core::flows::listing::{LastUsed, SizeCell, Sizes, TableRow, WorkspaceTable}; use devlaunch_core::flows::migration::{Listing, MigrationReport}; diff --git a/rust/dl/tests/launch.rs b/rust/dl/tests/launch.rs index af911b1e..857978c8 100644 --- a/rust/dl/tests/launch.rs +++ b/rust/dl/tests/launch.rs @@ -1616,7 +1616,7 @@ fn rm_on_exit_removes_a_workspace_whose_session_devpod_never_ran() { fn rm_on_exit_collects_a_workspace_whose_up_failed() { // The case keying the cleanup on the exit code got wrong, and the one that // matters most: a `devpod up` that dies leaves the container running, devpod's - // record written and the clone cut — see `lifecycle::create_record`, which exists + // record written and the clone cut — see `clients::devpod_home::create_record`, which exists // because of it. An unattended `dl owner/repo --rm -- ` against a broken // devcontainer would otherwise leak exactly the workspace the flag was reached // for, on every run, silently. From f5a2777d91f31b0f581281a68a98c972711bf0dd Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 24 Aug 2026 18:45:06 +0000 Subject: [PATCH 2/2] `system_words` moves to the leaf, so no client names a flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `clients/devpod_home.rs` was importing `crate::flows::repo_manager::system_words` — the only `use crate::flows::` under `src/clients/` in the crate, and none existed before this branch. `lib.rs:15` says the layers run strictly downward and "a client never names a flow", so the import is exactly the seam-pointing-the- wrong-way this branch's own body rejected `repoint(&Adoptable)` for. Naming a reconcile-flow *module* is the same objection as naming a reconcile-flow *type*. It moves to `osext`, which is where it belonged before it had two callers. `osext` is the process boundary ported to Python's `os` semantics: `env_str` is `os.environ.get`, `strip` is `str.strip()`, `home_dir` is `posixpath.expanduser("~")`, `temp_dir` is `tempfile.gettempdir()` — and `system_words` is `OSError.strerror`, a reading of the host that exists because `std::io::Error`'s `Display` appends `" (os error {errno})"` where Python's does not. Nothing about it was ever a flow; it sat in `repo_manager` because that is where its first caller was. `osext` is also a leaf that depends on nothing, which is the only kind of module both a flow and a client may name, so the shared helper stops being an upward edge for either of them. `public-api.rest.txt` gets the `DevpodHome` block moved after `RepointFailure`, which is where the generator puts it and where CI's `public-api` job said it should be. The convention is one module up: `clients::devpod` lists its six enums first and `struct Workspace` last, and `flows::lifecycle` does the same. Enums before structs; `RepointFailure` is an enum and `DevpodHome` is a struct. Nothing else in either snapshot changes — sorting both revisions of the file shows an identical multiset of rows, so this is ordering and only ordering, and `public-api.api.txt`'s one hand-edited row was already right. Two smaller things while here. `lib.rs:8`'s roster of tool clients still read "`devpod`, `git`, `gh`, `ssh`" after a fifth was added. And the new module's docs linked `DevpodHome::repoint` and `Self::located`, both of which are private to the crate, from a `pub mod` — two fresh `private_intra_doc_links` warnings in CI's rustdoc output. They become plain code spans; the sentences meant them as names rather than as destinations. `DevpodHome::at` and `::path` stay `pub`. They have no consumer outside the crate and `lib.rs:37` would keep them `pub(crate)` for that, but `locate()` is not a substitute for `at()` for anyone holding a directory, and whether an outside caller needs to construct one at all is the `api::workspace_delete` question that belongs to #410. Narrowing them here would settle that question by default and in the direction #410 may want reversed. --- rust/devlaunch-core/public-api.rest.txt | 26 ++++++++--------- .../devlaunch-core/src/clients/devpod_home.rs | 6 ++-- rust/devlaunch-core/src/flows/repo_manager.rs | 20 +------------ rust/devlaunch-core/src/lib.rs | 5 ++-- rust/devlaunch-core/src/osext.rs | 28 +++++++++++++++++++ 5 files changed, 48 insertions(+), 37 deletions(-) diff --git a/rust/devlaunch-core/public-api.rest.txt b/rust/devlaunch-core/public-api.rest.txt index e6c234d9..c2d2773a 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -148,19 +148,6 @@ impl core::fmt::Debug for devlaunch_core::clients::devpod::Workspace pub fn devlaunch_core::clients::devpod::Workspace::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for devlaunch_core::clients::devpod::Workspace pub mod devlaunch_core::clients::devpod_home -pub struct devlaunch_core::clients::devpod_home::DevpodHome -impl devlaunch_core::clients::devpod_home::DevpodHome -pub fn devlaunch_core::clients::devpod_home::DevpodHome::at(impl core::convert::Into) -> Self -pub fn devlaunch_core::clients::devpod_home::DevpodHome::locate() -> core::option::Option -pub fn devlaunch_core::clients::devpod_home::DevpodHome::path(&self) -> &std::path::Path -impl core::clone::Clone for devlaunch_core::clients::devpod_home::DevpodHome -pub fn devlaunch_core::clients::devpod_home::DevpodHome::clone(&self) -> devlaunch_core::clients::devpod_home::DevpodHome -impl core::cmp::Eq for devlaunch_core::clients::devpod_home::DevpodHome -impl core::cmp::PartialEq for devlaunch_core::clients::devpod_home::DevpodHome -pub fn devlaunch_core::clients::devpod_home::DevpodHome::eq(&self, &devlaunch_core::clients::devpod_home::DevpodHome) -> bool -impl core::fmt::Debug for devlaunch_core::clients::devpod_home::DevpodHome -pub fn devlaunch_core::clients::devpod_home::DevpodHome::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl core::marker::StructuralPartialEq for devlaunch_core::clients::devpod_home::DevpodHome pub enum devlaunch_core::clients::devpod_home::RepointFailure pub devlaunch_core::clients::devpod_home::RepointFailure::NotADevpodRecord pub devlaunch_core::clients::devpod_home::RepointFailure::NotADevpodRecord::path: std::path::PathBuf @@ -181,6 +168,19 @@ pub fn devlaunch_core::clients::devpod_home::RepointFailure::eq(&self, &devlaunc impl core::fmt::Debug for devlaunch_core::clients::devpod_home::RepointFailure pub fn devlaunch_core::clients::devpod_home::RepointFailure::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for devlaunch_core::clients::devpod_home::RepointFailure +pub struct devlaunch_core::clients::devpod_home::DevpodHome +impl devlaunch_core::clients::devpod_home::DevpodHome +pub fn devlaunch_core::clients::devpod_home::DevpodHome::at(impl core::convert::Into) -> Self +pub fn devlaunch_core::clients::devpod_home::DevpodHome::locate() -> core::option::Option +pub fn devlaunch_core::clients::devpod_home::DevpodHome::path(&self) -> &std::path::Path +impl core::clone::Clone for devlaunch_core::clients::devpod_home::DevpodHome +pub fn devlaunch_core::clients::devpod_home::DevpodHome::clone(&self) -> devlaunch_core::clients::devpod_home::DevpodHome +impl core::cmp::Eq for devlaunch_core::clients::devpod_home::DevpodHome +impl core::cmp::PartialEq for devlaunch_core::clients::devpod_home::DevpodHome +pub fn devlaunch_core::clients::devpod_home::DevpodHome::eq(&self, &devlaunch_core::clients::devpod_home::DevpodHome) -> bool +impl core::fmt::Debug for devlaunch_core::clients::devpod_home::DevpodHome +pub fn devlaunch_core::clients::devpod_home::DevpodHome::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::StructuralPartialEq for devlaunch_core::clients::devpod_home::DevpodHome pub mod devlaunch_core::clients::gh pub enum devlaunch_core::clients::gh::GhEvent pub devlaunch_core::clients::gh::GhEvent::CouldNotRun(devlaunch_core::clients::gh::GhUnavailable) diff --git a/rust/devlaunch-core/src/clients/devpod_home.rs b/rust/devlaunch-core/src/clients/devpod_home.rs index 15d5c4e4..e1022448 100644 --- a/rust/devlaunch-core/src/clients/devpod_home.rs +++ b/rust/devlaunch-core/src/clients/devpod_home.rs @@ -17,7 +17,7 @@ //! //! # Reading it is the point; one write is the exception //! -//! Everything here but [`DevpodHome::repoint`] reads. That write is argued for at +//! Everything here but `DevpodHome::repoint` reads. That write is argued for at //! the method, and it is what makes this an adapter rather than a path helper: the //! module owns devpod's file format on the way out as well as the way in, so the //! flows above it never open one. @@ -33,7 +33,7 @@ use std::ffi::OsString; use std::path::{Path, PathBuf}; -use crate::flows::repo_manager::system_words; +use crate::osext::system_words; /// The directory devpod keeps its own records in. /// @@ -56,7 +56,7 @@ impl DevpodHome { /// /// Honours `DEVPOD_HOME` for the same reason the rest of dl does: it is what /// scopes devpod, and the test suite sets it. The thin half of the pair — it - /// reads the process environment and nothing else, so [`Self::located`] is + /// reads the process environment and nothing else, so `Self::located` is /// where the cases are pinned. pub fn locate() -> Option { Self::located(std::env::var_os("DEVPOD_HOME"), crate::osext::home_dir) diff --git a/rust/devlaunch-core/src/flows/repo_manager.rs b/rust/devlaunch-core/src/flows/repo_manager.rs index 6416ffec..67aa7945 100644 --- a/rust/devlaunch-core/src/flows/repo_manager.rs +++ b/rust/devlaunch-core/src/flows/repo_manager.rs @@ -45,6 +45,7 @@ use crate::domain::model::{BaseRepository, RecordedDefaultBranch, Timestamp}; use crate::domain::workspace_id::{NamePart, UnsafeName, validate_ref_name}; use crate::domain::workspace_state::NonEmpty; use crate::notices::Notices; +use crate::osext::system_words; use crate::timing; /// The bare repository's directory name, inside the repo directory. @@ -551,25 +552,6 @@ pub(crate) fn present(path: &Path) -> bool { ) } -/// The system's own words for a failure, as Python's `OSError.strerror` gives -/// them. -/// -/// [`std::io::Error`]'s `Display` is `"{strerror} (os error {errno})"` for an OS -/// error, and the errno is already carried by the arm that holds this string — -/// where it is carried at all. So the suffix is dropped: the reason is printed to -/// a person beside the path it is about, and `Permission denied` is the whole of -/// what they need from it. -pub(crate) fn system_words(error: &std::io::Error) -> String { - let text = error.to_string(); - match error.raw_os_error() { - Some(errno) => text - .strip_suffix(&format!(" (os error {errno})")) - .unwrap_or(&text) - .to_owned(), - None => text, - } -} - /// One path a removal could not remove, and what the system said about it. /// /// The reason is carried rather than reconstructed at the print site because the diff --git a/rust/devlaunch-core/src/lib.rs b/rust/devlaunch-core/src/lib.rs index 21a01770..99d10bb9 100644 --- a/rust/devlaunch-core/src/lib.rs +++ b/rust/devlaunch-core/src/lib.rs @@ -5,8 +5,9 @@ //! //! 1. **runner** — the one seam to the outside: a spawn-spec in, an outcome //! out. The only place a process is started. -//! 2. **tool clients** — `devpod`, `git`, `gh`, `ssh`: typed argv builders -//! and output parsers over the runner. +//! 2. **tool clients** — `devpod`, `devpod_home`, `git`, `gh`, `ssh`: typed +//! argv builders and output parsers over the runner, plus `devpod_home` for +//! devpod-the-filesystem where `devpod` is devpod-the-command. //! 3. **domain** — `workspace_id`, `spec`, `model`, `metadata`, `config`, //! `xdg`, `locks`: the data model, written once. //! 4. **flows** — `launch`, `lifecycle`, `listing`, `provision`, diff --git a/rust/devlaunch-core/src/osext.rs b/rust/devlaunch-core/src/osext.rs index c3addc27..907bc011 100644 --- a/rust/devlaunch-core/src/osext.rs +++ b/rust/devlaunch-core/src/osext.rs @@ -23,6 +23,9 @@ //! the `/tmp` family as the fallback, the way `tempfile.gettempdir()` does. A //! non-existent `TMPDIR` otherwise makes the token-staging file fail to create //! and silently costs the workspace its gh login. +//! - [`system_words`] reads a failure the way `OSError.strerror` gives it. +//! `std::io::Error`'s `Display` appends `" (os error {errno})"`, which is the +//! errno repeated at a person who is being shown the path it happened to. use std::ffi::OsString; use std::path::PathBuf; @@ -126,6 +129,31 @@ fn usable(dir: &std::path::Path) -> bool { .is_ok() } +/// The system's own words for a failure, as Python's `OSError.strerror` gives +/// them. +/// +/// [`std::io::Error`]'s `Display` is `"{strerror} (os error {errno})"` for an OS +/// error, and the errno is already carried by the arm that holds this string — +/// where it is carried at all. So the suffix is dropped: the reason is printed to +/// a person beside the path it is about, and `Permission denied` is the whole of +/// what they need from it. +/// +/// It lives here rather than beside its first caller because it is a reading of +/// the host, like everything else in this module, and because a leaf is the only +/// place both a flow (`flows::repo_manager`) and a client +/// (`clients::devpod_home`) may name: the crate's layers run strictly downward, +/// and a client never names a flow. +pub fn system_words(error: &std::io::Error) -> String { + let text = error.to_string(); + match error.raw_os_error() { + Some(errno) => text + .strip_suffix(&format!(" (os error {errno})")) + .unwrap_or(&text) + .to_owned(), + None => text, + } +} + #[cfg(test)] mod tests { use super::*;