diff --git a/crates/fbuild-build-arm/src/teensy/teensy_linker.rs b/crates/fbuild-build-arm/src/teensy/teensy_linker.rs index 17b2d71f8..d890b8533 100644 --- a/crates/fbuild-build-arm/src/teensy/teensy_linker.rs +++ b/crates/fbuild-build-arm/src/teensy/teensy_linker.rs @@ -276,18 +276,27 @@ mod tests { #[test] fn link_runs_in_absolute_output_dir_not_inherited_cwd() { let (out, core_dir) = if fbuild_core::platform::host::is_windows() { - ("C:\\proj\\.fbuild\\build\\release", "C:\\pkgs\\teensy4") + ( + format!( + "C:\\proj\\{}\\build\\release", + fbuild_paths::FBUILD_DIR_NAME + ), + "C:\\pkgs\\teensy4".to_string(), + ) } else { - ("/proj/.fbuild/build/release", "/pkgs/teensy4") + ( + format!("/proj/{}/build/release", fbuild_paths::FBUILD_DIR_NAME), + "/pkgs/teensy4".to_string(), + ) }; - let scripts = LinkerScripts::single(PathBuf::from(core_dir), "imxrt1062_t41.ld"); - let objects = [PathBuf::from(out).join("sketch.o")]; + let scripts = LinkerScripts::single(PathBuf::from(&core_dir), "imxrt1062_t41.ld"); + let objects = [PathBuf::from(&out).join("sketch.o")]; assert_eq!( link_cwd_for( - Path::new(out), + Path::new(&out), objects.iter().chain(scripts.search_dirs.iter()) ), - Some(Path::new(out)), + Some(Path::new(&out)), "absolute output dir must become the link cwd so linker scratch \ files stay in the build tree" ); diff --git a/crates/fbuild-core/src/path.rs b/crates/fbuild-core/src/path.rs index 3dadaf84c..b1711c263 100644 --- a/crates/fbuild-core/src/path.rs +++ b/crates/fbuild-core/src/path.rs @@ -310,6 +310,17 @@ pub fn normalize_for_key(path: &Path) -> String { // different project directories — see agents/docs/path-conventions.md. // --------------------------------------------------------------------------- +/// The directory segment fbuild owns, both project-local +/// (`/.fbuild/`) and home-local (`~/.fbuild/{dev|prod}/`). +/// +/// Defined here rather than in `fbuild-paths` because `fbuild-paths` +/// depends on this crate, not the reverse, and both +/// [`compile_cwd_from_output`] below and `response_file` need it. +/// `fbuild_paths::FBUILD_DIR_NAME` re-exports this, so `fbuild-paths` +/// remains the name every other crate reaches for +/// (FastLED/fbuild#1349). +pub const FBUILD_DIR_NAME: &str = ".fbuild"; + /// Return the workspace root to use as the CWD for zccache compiles. /// /// fbuild object files live under `/.fbuild/...`, so running @@ -323,7 +334,7 @@ pub fn compile_cwd_from_output(output: &Path) -> Option { if dir .file_name() .and_then(|name| name.to_str()) - .is_some_and(|name| name.eq_ignore_ascii_case(".fbuild")) + .is_some_and(|name| name.eq_ignore_ascii_case(FBUILD_DIR_NAME)) { return dir.parent().map(|workspace| { canonicalize_lexical(workspace).unwrap_or_else(|| workspace.to_path_buf()) @@ -744,7 +755,10 @@ mod tests { #[test] fn compile_cwd_from_output_uses_workspace_before_fbuild() { - let output = Path::new("/work/project/.fbuild/build/env/release/src/main.o"); + let output = PathBuf::from(format!( + "/work/project/{FBUILD_DIR_NAME}/build/env/release/src/main.o" + )); + let output = output.as_path(); assert_eq!( compile_cwd_from_output(output).as_deref(), Some(Path::new("/work/project")) @@ -761,7 +775,7 @@ mod tests { fn compile_cwd_from_output_canonicalizes_existing_workspace() { let tmp = tempfile::TempDir::new().unwrap(); let workspace = tmp.path().join("project"); - let output = workspace.join(".fbuild/build/main.o"); + let output = workspace.join(format!("{FBUILD_DIR_NAME}/build/main.o")); std::fs::create_dir_all(output.parent().unwrap()).unwrap(); let expected = strip_unc_prefix(&workspace.canonicalize().unwrap()); assert_eq!( diff --git a/crates/fbuild-core/src/response_file.rs b/crates/fbuild-core/src/response_file.rs index 0efce5be6..13390ab0a 100644 --- a/crates/fbuild-core/src/response_file.rs +++ b/crates/fbuild-core/src/response_file.rs @@ -37,17 +37,23 @@ fn response_files_dir() -> PathBuf { fn response_files_root(home: &Path, dev_mode: bool) -> PathBuf { let mode = if dev_mode { "dev" } else { "prod" }; - home.join(".fbuild") + home.join(crate::path::FBUILD_DIR_NAME) .join(mode) .join("tmp") .join("response-files") } +/// Resolve the home directory through the neutral host facade. +/// +/// Deliberately not a local `HOME`-first lookup. On Windows the facade +/// prefers `%USERPROFILE%`, and preferring `HOME` instead put response files +/// under an MSYS path like `/c/Users/you` whenever fbuild ran from Git Bash — +/// which native Windows GCC cannot open, and is precisely the failure +/// [`windows_temp_dir`] exists to avoid (FastLED/fbuild#1349). fn home_dir() -> PathBuf { - std::env::var("HOME") - .or_else(|_| std::env::var("USERPROFILE")) - .map(PathBuf::from) - .unwrap_or_else(|_| std::env::temp_dir()) + crate::platform::host::home_dir() + .map(|home| home.into_path_buf()) + .unwrap_or_else(std::env::temp_dir) } fn is_dev_mode() -> bool { @@ -338,20 +344,61 @@ mod tests { assert_eq!(content, expected_content); } + /// FastLED/fbuild#1349: this module resolved the home directory itself, + /// preferring `HOME` over `USERPROFILE`. The neutral facade prefers + /// `USERPROFILE` on Windows, and that difference is exactly the case this + /// module exists for: under MSYS2 / Git Bash `HOME` is a POSIX path such + /// as `/c/Users/you`, which native Windows GCC cannot open. Preferring it + /// put response files somewhere no compiler could read — the failure + /// `windows_temp_dir` was written to avoid, reintroduced by resolving the + /// home directory a second way. + /// + /// The test harness does not inherit Git Bash's `HOME`, so the divergence + /// is invisible unless the test sets it. + #[test] + fn home_dir_prefers_the_native_profile_over_an_msys_home() { + static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + let _lock = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + if !crate::platform::host::is_windows() { + return; // `HOME` is the right answer everywhere else + } + + struct Restore(&'static str, Option); + impl Drop for Restore { + fn drop(&mut self) { + match self.1.take() { + Some(v) => std::env::set_var(self.0, v), + None => std::env::remove_var(self.0), + } + } + } + + let _home = Restore("HOME", std::env::var_os("HOME")); + let _profile = Restore("USERPROFILE", std::env::var_os("USERPROFILE")); + std::env::set_var("HOME", "/c/Users/msys"); + std::env::set_var("USERPROFILE", "C:\\Users\\native"); + + assert_eq!( + home_dir(), + PathBuf::from("C:\\Users\\native"), + "an MSYS-style HOME must not win: native GCC cannot open that path" + ); + } + #[test] fn test_response_files_root_uses_fbuild_owned_tmp_dir() { let home = Path::new("/home/user"); assert_eq!( response_files_root(home, false), - home.join(".fbuild") + home.join(crate::path::FBUILD_DIR_NAME) .join("prod") .join("tmp") .join("response-files") ); assert_eq!( response_files_root(home, true), - home.join(".fbuild") + home.join(crate::path::FBUILD_DIR_NAME) .join("dev") .join("tmp") .join("response-files") diff --git a/crates/fbuild-library-select/tests/teensy41_ldf_diag.rs b/crates/fbuild-library-select/tests/teensy41_ldf_diag.rs index a2b8eec24..738ac5d49 100644 --- a/crates/fbuild-library-select/tests/teensy41_ldf_diag.rs +++ b/crates/fbuild-library-select/tests/teensy41_ldf_diag.rs @@ -29,12 +29,11 @@ fn home_dir() -> Option { } fn find_teensy_libraries() -> Option { - let home = home_dir()?; - let root = home - .join(".fbuild") - .join("prod") - .join("cache") - .join("platforms"); + // FastLED/fbuild#1349: ask fbuild-paths where the cache is rather than + // rebuilding `~/.fbuild/prod/cache` here — the hardcoded `prod` made this + // diagnostic look in the wrong tree under `FBUILD_DEV_MODE=1`, and it + // ignored `FBUILD_CACHE_DIR` entirely. + let root = fbuild_paths::get_cache_root().join("platforms"); let entries = std::fs::read_dir(&root).ok()?; for entry in entries.flatten() { let name = entry.file_name().to_string_lossy().into_owned(); diff --git a/crates/fbuild-library/src/library/library_compiler.rs b/crates/fbuild-library/src/library/library_compiler.rs index 57b7b4a52..6f72397f5 100644 --- a/crates/fbuild-library/src/library/library_compiler.rs +++ b/crates/fbuild-library/src/library/library_compiler.rs @@ -737,7 +737,7 @@ fn object_hash_key(source: &Path, obj_dir: &Path) -> String { for ancestor in obj_dir.ancestors() { if ancestor .file_name() - .map(|n| n == ".fbuild") + .map(|n| n == fbuild_core::path::FBUILD_DIR_NAME) .unwrap_or(false) { if let Some(workspace) = ancestor.parent() { diff --git a/crates/fbuild-paths/src/lib.rs b/crates/fbuild-paths/src/lib.rs index c9d7fa974..aca93de08 100644 --- a/crates/fbuild-paths/src/lib.rs +++ b/crates/fbuild-paths/src/lib.rs @@ -13,10 +13,16 @@ pub mod running_process; /// The project-local and home-local fbuild directory segment: `.fbuild`. /// -/// This is the canonical spelling. Nothing outside this crate should write -/// the literal — see the `ban_raw_fbuild_path` Dylint -/// (FastLED/fbuild#1349). -pub const FBUILD_DIR_NAME: &str = ".fbuild"; +/// This is the canonical spelling. Nothing outside `fbuild-paths` / +/// `fbuild-core` should write the literal — see the `ban_raw_fbuild_path` +/// Dylint (FastLED/fbuild#1349). +/// +/// Re-exported from `fbuild-core` rather than defined here: `fbuild-core` +/// needs the segment too (`compile_cwd_from_output` walks for it, +/// `response_file` builds under it) and cannot depend on this crate, since +/// the dependency runs the other way. Keeping the name reachable at +/// `fbuild_paths::FBUILD_DIR_NAME` means no consumer has to know that. +pub use fbuild_core::path::FBUILD_DIR_NAME; /// The build-tree segment directly under [`FBUILD_DIR_NAME`]: `build`. /// diff --git a/dylints/ban_raw_fbuild_path/Cargo.toml b/dylints/ban_raw_fbuild_path/Cargo.toml index cdd9e7ec1..41c7cb6ee 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.1" +version = "0.1.2" 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 685d7f5b3..4e4d9ad47 100644 --- a/dylints/ban_raw_fbuild_path/src/allowlist.txt +++ b/dylints/ban_raw_fbuild_path/src/allowlist.txt @@ -18,11 +18,17 @@ # call site delegates to them. crates/fbuild-paths/src/lib.rs +# Where `FBUILD_DIR_NAME` is actually declared. It cannot live in +# fbuild-paths: fbuild-paths depends on fbuild-core, not the reverse, and +# fbuild-core needs the segment itself (`compile_cwd_from_output` walks for +# it, `response_file` builds under it). fbuild-paths re-exports the const, so +# `fbuild_paths::FBUILD_DIR_NAME` is still the name every other crate uses. +crates/fbuild-core/src/path.rs + # --- Baseline: legacy sites captured at landing (FastLED/fbuild#1349) --- # Each line below is a file that spells `.fbuild` by hand today. Removing # a line is the unit of progress on #1349; adding one is not allowed. -crates/fbuild-build-arm/src/teensy/teensy_linker.rs crates/fbuild-build-engine/src/build_fingerprint/fast_path.rs crates/fbuild-build-engine/src/build_info.rs crates/fbuild-build-engine/src/compile_database/tests/clang.rs @@ -54,13 +60,9 @@ 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-core/src/path.rs -crates/fbuild-core/src/response_file.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 -crates/fbuild-library-select/tests/teensy41_ldf_diag.rs -crates/fbuild-library/src/library/library_compiler.rs