Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions crates/fbuild-build-arm/src/teensy/teensy_linker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
);
Expand Down
20 changes: 17 additions & 3 deletions crates/fbuild-core/src/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
/// (`<project>/.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 `<workspace>/.fbuild/...`, so running
Expand All @@ -323,7 +334,7 @@ pub fn compile_cwd_from_output(output: &Path) -> Option<PathBuf> {
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())
Expand Down Expand Up @@ -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"))
Expand All @@ -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!(
Expand Down
61 changes: 54 additions & 7 deletions crates/fbuild-core/src/response_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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<std::ffi::OsString>);
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")
Expand Down
11 changes: 5 additions & 6 deletions crates/fbuild-library-select/tests/teensy41_ldf_diag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,11 @@ fn home_dir() -> Option<PathBuf> {
}

fn find_teensy_libraries() -> Option<PathBuf> {
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();
Expand Down
2 changes: 1 addition & 1 deletion crates/fbuild-library/src/library/library_compiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
14 changes: 10 additions & 4 deletions crates/fbuild-paths/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
///
Expand Down
2 changes: 1 addition & 1 deletion dylints/ban_raw_fbuild_path/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 7 additions & 5 deletions dylints/ban_raw_fbuild_path/src/allowlist.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Loading