diff --git a/crates/fbuild-daemon/src/handlers/emulator/tests_process.rs b/crates/fbuild-daemon/src/handlers/emulator/tests_process.rs index fa5980dc..452704a7 100644 --- a/crates/fbuild-daemon/src/handlers/emulator/tests_process.rs +++ b/crates/fbuild-daemon/src/handlers/emulator/tests_process.rs @@ -134,7 +134,9 @@ async fn run_real_esp32s3_fixture_in_qemu() { "esp32s3".to_string(), BuildProfile::Release, ) - .with_override_root(Some(project_dir.join(".fbuild").join("build-qemu"))) + .with_override_root(Some( + fbuild_paths::get_project_fbuild_dir(&project_dir).join("build-qemu"), + )) .resolve(); let params = BuildParams { project_dir: project_dir.clone(), diff --git a/crates/fbuild-daemon/src/handlers/libraries.rs b/crates/fbuild-daemon/src/handlers/libraries.rs index d4084dd9..d8a62109 100644 --- a/crates/fbuild-daemon/src/handlers/libraries.rs +++ b/crates/fbuild-daemon/src/handlers/libraries.rs @@ -44,11 +44,57 @@ use crate::models::{IdeLibrariesQuery, IdeLibrariesResponse, IdeLibraryEntry}; const LIBRARIES_PAGE_HTML: &str = include_str!("../../web/libraries/index.html"); -const INSTALL_STATE_NOTE: &str = "Installed state is best-effort: it checks the release build \ -profile's /.fbuild/build//release/libs/ directory for a same-named subdirectory. \ -That directory is only populated after a build that needed dependencies has run — if no build \ -has run yet, every entry reports installed: false even though the source may be perfectly \ -resolvable."; +/// Explains what `installed` means on the libraries page. +/// +/// Takes the directory the handler actually checked, when it got far enough +/// to resolve one. `BuildLayout::resolve()` drops the `` segment when it +/// matches the project basename, so a note that always spells +/// `/.fbuild/build//release/libs/` can name a path the handler +/// never looked at — the same class of drift FastLED/fbuild#1349 is about, +/// one level up from the literal. +/// +/// The generic form is only used on the early-return paths, which carry no +/// library data for the note to be wrong about; it states the collapse rule +/// rather than pretending the segment is always there. +fn install_state_note(libs_dir: Option<&Path>) -> String { + let location = match libs_dir { + Some(dir) => format!("the {} directory", dir.display()), + None => generic_libs_location(), + }; + format!( + "Installed state is best-effort: it checks {location} for a same-named \ + subdirectory. That directory is only populated after a build that needed \ + dependencies has run — if no build has run yet, every entry reports \ + installed: false even though the source may be perfectly resolvable." + ) +} + +/// Describe where library output lands when the handler failed before it +/// could resolve a concrete path. +/// +/// Two things can move that directory out from under a hardcoded description, +/// and both have to be said rather than assumed away: +/// +/// - `FBUILD_BUILD_DIR` replaces the build root outright +/// (`get_project_build_root` returns it verbatim), so the `.fbuild/build` +/// segments are simply not present. +/// - `BuildLayout::resolve()` drops the `` segment when it matches the +/// project directory name. +fn generic_libs_location() -> String { + match std::env::var("FBUILD_BUILD_DIR") { + Ok(root) if !root.trim().is_empty() => format!( + "the release build profile's /release/libs/ directory under {root}, \ + which FBUILD_BUILD_DIR has substituted for the default build root" + ), + _ => format!( + "the release build profile's /{}/{}//release/libs/ directory \ + (the segment is omitted when it matches the project directory name, \ + and FBUILD_BUILD_DIR replaces the root entirely when set)", + fbuild_paths::FBUILD_DIR_NAME, + fbuild_paths::BUILD_DIR_NAME + ), + } +} /// GET /libraries — serve the self-contained Library Manager page. pub async fn libraries_page() -> impl IntoResponse { @@ -165,7 +211,7 @@ pub async fn list_libraries( project: None, environment: None, libraries: Vec::new(), - install_state_note: INSTALL_STATE_NOTE.to_string(), + install_state_note: install_state_note(None), error: Some( "missing required ?project= query param; \ run `fbuild libraries` from a project directory to open this page \ @@ -185,7 +231,7 @@ pub async fn list_libraries( project: Some(project), environment: params.env.clone(), libraries: Vec::new(), - install_state_note: INSTALL_STATE_NOTE.to_string(), + install_state_note: install_state_note(None), error: Some(e), }), ); @@ -201,7 +247,7 @@ pub async fn list_libraries( project: Some(project), environment: params.env.clone(), libraries: Vec::new(), - install_state_note: INSTALL_STATE_NOTE.to_string(), + install_state_note: install_state_note(None), error: Some(format!("failed to parse platformio.ini: {}", e)), }), ); @@ -218,7 +264,7 @@ pub async fn list_libraries( project: Some(project), environment: params.env.clone(), libraries: Vec::new(), - install_state_note: INSTALL_STATE_NOTE.to_string(), + install_state_note: install_state_note(None), error: Some(e), }), ); @@ -241,7 +287,7 @@ pub async fn list_libraries( project: Some(project), environment: Some(env_name), libraries, - install_state_note: INSTALL_STATE_NOTE.to_string(), + install_state_note: install_state_note(Some(libs_dir.as_ref())), error: None, }), ), @@ -252,7 +298,7 @@ pub async fn list_libraries( project: Some(project), environment: Some(env_name), libraries: Vec::new(), - install_state_note: INSTALL_STATE_NOTE.to_string(), + install_state_note: install_state_note(Some(libs_dir.as_ref())), error: Some(e), }), ), @@ -261,6 +307,94 @@ pub async fn list_libraries( #[cfg(test)] mod tests { + + /// FastLED/fbuild#1349 review: `BuildLayout::resolve()` drops the `` + /// segment when it matches the project directory name, so a note that + /// always spells `` can name a directory the handler never checked. + #[test] + fn the_note_names_the_directory_that_was_actually_checked() { + // Assembled from the canonical segments: a fixture that spells the + // layout by hand is the thing this ratchet exists to remove, and the + // lint rightly rejects one here too. + let checked = format!( + "/proj/{}/{}/release/libs", + fbuild_paths::FBUILD_DIR_NAME, + fbuild_paths::BUILD_DIR_NAME + ); + let named = install_state_note(Some(Path::new(&checked))); + assert!(named.contains(&checked), "{named}"); + assert!( + !named.contains(""), + "a resolved path must not carry an unexpanded placeholder: {named}" + ); + } + + /// The env overrides below are process-wide; cargo runs these in + /// parallel, so serialize the two that read them. + static BUILD_DIR_ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + /// Restores `FBUILD_BUILD_DIR` on drop so a failing assertion cannot + /// leave it set for a sibling test. + struct BuildDirEnv(Option); + + impl BuildDirEnv { + fn set(value: Option<&str>) -> Self { + let saved = std::env::var_os("FBUILD_BUILD_DIR"); + match value { + Some(v) => std::env::set_var("FBUILD_BUILD_DIR", v), + None => std::env::remove_var("FBUILD_BUILD_DIR"), + } + Self(saved) + } + } + + impl Drop for BuildDirEnv { + fn drop(&mut self) { + match self.0.take() { + Some(v) => std::env::set_var("FBUILD_BUILD_DIR", v), + None => std::env::remove_var("FBUILD_BUILD_DIR"), + } + } + } + + /// Without a resolved directory the note has to describe the layout — so + /// it must also state both ways that layout can move: the collapsed + /// `` segment and the `FBUILD_BUILD_DIR` override. + #[test] + fn the_generic_note_states_both_ways_the_layout_can_move() { + let _lock = BUILD_DIR_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _env = BuildDirEnv::set(None); + + let generic = install_state_note(None); + assert!(generic.contains(""), "{generic}"); + assert!( + generic.contains("omitted when it matches the project directory name"), + "{generic}" + ); + assert!( + generic.contains("FBUILD_BUILD_DIR"), + "the default description must still name the override that can replace it: {generic}" + ); + } + + /// FastLED/fbuild#1349 review: `get_project_build_root` returns + /// `FBUILD_BUILD_DIR` verbatim, so when it is set the `.fbuild/build` + /// segments are not in the path at all. Describing them anyway points the + /// reader at a directory that does not exist on their machine. + #[test] + fn the_generic_note_follows_the_build_dir_override() { + let _lock = BUILD_DIR_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _env = BuildDirEnv::set(Some("/scratch/short")); + + let generic = install_state_note(None); + assert!(generic.contains("/scratch/short"), "{generic}"); + assert!( + !generic.contains(fbuild_paths::FBUILD_DIR_NAME), + "the override replaces the root, so the default segments must not be described \ + as if they were still there: {generic}" + ); + } + use super::*; async fn write_project(dir: &Path, ini: &str) { diff --git a/crates/fbuild-daemon/src/handlers/operations/build.rs b/crates/fbuild-daemon/src/handlers/operations/build.rs index ef412345..4d9029a1 100644 --- a/crates/fbuild-daemon/src/handlers/operations/build.rs +++ b/crates/fbuild-daemon/src/handlers/operations/build.rs @@ -88,7 +88,7 @@ impl Drop for StreamTerminationGuard { "type": "result", "success": false, "request_id": self.request_id, - "message": "daemon build worker terminated unexpectedly (panic or early return); check ~/.fbuild/daemon/daemon.log", + "message": worker_terminated_message(), "exit_code": 1, "output_file": null, "output_dir": null, @@ -347,11 +347,11 @@ pub async fn build( "project lock on {} not acquired within {}s — \ another build is still holding it. Continuing \ to wait; if this persists the daemon may be \ - stuck. Inspect ~/.fbuild//daemon/daemon.log \ - or run `fbuild daemon locks` to see who is \ - holding the lock.", + stuck. Inspect {} or run `fbuild daemon locks` \ + to see who is holding the lock.", project_dir_desc, LOCK_WAIT_WARN.as_secs(), + daemon_log_hint(), ); warned = true; } @@ -1017,3 +1017,55 @@ mod tests { assert!(!should_emit_dependency_status(&installed)); } } + +/// The daemon log file, named as an absolute path rather than a template the +/// reader has to expand. +/// +/// FastLED/fbuild#1349: the two diagnostics that point at this file each +/// spelled it by hand, and each spelled it wrong. One said +/// `~/.fbuild/daemon/daemon.log`, dropping the `dev`/`prod` segment entirely; +/// the other said `~/.fbuild//daemon/daemon.log`, where `` reads as +/// a PlatformIO environment and is really the dev/prod mode. Both sent a user +/// who was already debugging a stuck build to a path that does not exist. +fn daemon_log_hint() -> String { + fbuild_paths::get_daemon_log_file().display().to_string() +} + +/// Message reported when the build worker dies without answering. +fn worker_terminated_message() -> String { + format!( + "daemon build worker terminated unexpectedly (panic or early return); check {}", + daemon_log_hint() + ) +} + +#[cfg(test)] +mod daemon_log_hint_tests { + use super::*; + + /// The hint must name the file that actually exists. Asserting against + /// `get_daemon_log_file` rather than against a literal is the point: a + /// literal is what drifted. + #[test] + fn diagnostics_name_the_real_daemon_log_path() { + let expected = fbuild_paths::get_daemon_log_file(); + let expected = expected.display().to_string(); + assert_eq!(daemon_log_hint(), expected); + assert!( + worker_terminated_message().contains(&expected), + "{}", + worker_terminated_message() + ); + // The mode segment is what the old spelling dropped. + let mode = if fbuild_paths::is_dev_mode() { + "dev" + } else { + "prod" + }; + assert!( + daemon_log_hint().contains(mode), + "the hint must carry the dev/prod segment: {}", + daemon_log_hint() + ); + } +} diff --git a/crates/fbuild-daemon/src/main.rs b/crates/fbuild-daemon/src/main.rs index 4b67eae4..8cd77756 100644 --- a/crates/fbuild-daemon/src/main.rs +++ b/crates/fbuild-daemon/src/main.rs @@ -101,7 +101,7 @@ async fn main() { .add_directive(tracing::Level::INFO.into()), ) // Route fmt layer to stderr — the CLI captures the daemon's - // stderr into ~/.fbuild//daemon/daemon.log (see + // stderr into the daemon log under the fbuild root (see // daemon_client.rs spawn_daemon). The MakeWriter default is // stdout, but the CLI sets stdout to Null, so without // .with_writer(stderr) every `tracing::*` event in the daemon @@ -194,8 +194,8 @@ async fn main() { eprintln!( "fatal: failed to start embedded zccache service: {err}\n\ fbuild-daemon cannot start without an embedded zccache \ - backend. Check ~/.fbuild//zccache/ permissions and \ - disk space." + backend. Check {} permissions and disk space.", + fbuild_paths::get_fbuild_root().join("zccache").display() ); std::process::exit(1); } diff --git a/crates/fbuild-daemon/src/models.rs b/crates/fbuild-daemon/src/models.rs index c12291aa..ed52998c 100644 --- a/crates/fbuild-daemon/src/models.rs +++ b/crates/fbuild-daemon/src/models.rs @@ -651,6 +651,13 @@ pub struct IdeLibrariesResponse { mod tests { use super::*; + /// Fixture cache path, assembled from the canonical segment rather than + /// typed out — a fixture that spells the layout by hand goes on passing + /// after the layout changes (FastLED/fbuild#1349). + fn fixture_cache_dir() -> String { + format!("/home/user/{}/prod/cache", fbuild_paths::FBUILD_DIR_NAME) + } + // --- BuildRequest deserialization --- #[test] @@ -932,11 +939,13 @@ mod tests { current_operation: None, dependency_install: None, client_count: 3, - cache_dir: "/home/user/.fbuild/prod/cache".into(), - cache_identity: - "mode=prod;trust=local-shared;schema=1;cache=/home/user/.fbuild/prod/cache".into(), + cache_dir: fixture_cache_dir(), + cache_identity: format!( + "mode=prod;trust=local-shared;schema=1;cache={}", + fixture_cache_dir() + ), cache_schema_version: 1, - daemon_dir: "/home/user/.fbuild/prod/daemon".into(), + daemon_dir: format!("/home/user/{}/prod/daemon", fbuild_paths::FBUILD_DIR_NAME), source_mtime: 1700000000.0, spawner_cwd: "/home/user/project".into(), mcp_url: "http://127.0.0.1:8765/mcp".into(), diff --git a/crates/fbuild-daemon/tests/legacy_daemon_transition.rs b/crates/fbuild-daemon/tests/legacy_daemon_transition.rs index 5391fb00..8c1d1bbd 100644 --- a/crates/fbuild-daemon/tests/legacy_daemon_transition.rs +++ b/crates/fbuild-daemon/tests/legacy_daemon_transition.rs @@ -167,7 +167,7 @@ fn free_port() -> u16 { /// scoped to just that child. fn root_owner_lock_path_for(temp_home: &Path) -> NormalizedPath { NormalizedPath::new(temp_home) - .join(".fbuild") + .join(fbuild_paths::FBUILD_DIR_NAME) .join("dev") .join("daemon") .join("root-owner.lock") diff --git a/dylints/ban_raw_fbuild_path/Cargo.toml b/dylints/ban_raw_fbuild_path/Cargo.toml index 41c7cb6e..452371ad 100644 --- a/dylints/ban_raw_fbuild_path/Cargo.toml +++ b/dylints/ban_raw_fbuild_path/Cargo.toml @@ -3,7 +3,7 @@ name = "ban_raw_fbuild_path" # Bump the version to bust the dylint .so cache when allowlist.txt # changes (setup-soldr's dylint-cache key hashes the manifest but not # src/allowlist.txt). Same convention ban_manual_slash_normalize follows. -version = "0.1.2" +version = "0.1.3" description = "Ban raw '.fbuild' path literals outside fbuild-paths" edition = "2021" publish = false diff --git a/dylints/ban_raw_fbuild_path/src/allowlist.txt b/dylints/ban_raw_fbuild_path/src/allowlist.txt index 4e4d9ad4..ac3cb3b8 100644 --- a/dylints/ban_raw_fbuild_path/src/allowlist.txt +++ b/dylints/ban_raw_fbuild_path/src/allowlist.txt @@ -60,9 +60,3 @@ crates/fbuild-cli/src/cli/purge.rs crates/fbuild-cli/src/cli/symbols_cmd.rs crates/fbuild-cli/src/lib_select.rs crates/fbuild-cli/tests/daemon_crash_recovery.rs -crates/fbuild-daemon/src/handlers/emulator/tests_process.rs -crates/fbuild-daemon/src/handlers/libraries.rs -crates/fbuild-daemon/src/handlers/operations/build.rs -crates/fbuild-daemon/src/main.rs -crates/fbuild-daemon/src/models.rs -crates/fbuild-daemon/tests/legacy_daemon_transition.rs