fix(esp32): strip Windows UNC prefix + collect .S/.s assembly sources (FastLED #2507)#273
Merged
Merged
Conversation
…LED #2507)
Two bugs together prevented FastLED's AutoResearch sketch from building
for esp32p4 via fbuild on Windows.
## Bug 1 — \?\ UNC prefix mangled into //?/ in response files
`crates/fbuild-build/src/zccache.rs::canonicalize_existing_path` used
`Path::canonicalize`, which on Windows returns paths prefixed with
`\?\` (extended-length namespace marker). That prefix only works with
backslash separators, but the response-file writer
(`fbuild-core::response_file::replace_path_backslashes`) rewrites all
backslashes to forward slashes so gcc doesn't interpret them as escape
sequences. The result was `-I//?/C:/…` on every include directory.
Some headers happened to resolve (gcc seems to tolerate the prefix on
some lookups) but chip-specific SoC headers like `soc/soc_caps.h` did
not. On esp32p4 this surfaced as:
cores/esp32/USBMSC.h:17:10: fatal error: soc/soc_caps.h: No such file
17 | #include "soc/soc_caps.h"
Fix: strip the `\?\` prefix in `canonicalize_existing_path` (cfg(windows)
only). `strip_unc_prefix` removes both `\?\` and the defensively-handled
`//?/` form, leaving `C:\…` which survives the backslash→slash rewrite
as the standard `C:/…` form gcc accepts. `path_arg_for_compile_cwd`
also strips the prefix from its `cwd` argument so `strip_prefix` matches
when callers pass an un-stripped canonical path.
Trade-off: long-path support (>260 chars) is lost for stripped paths,
but the cache root is `<home>/.fbuild` which is well under the limit.
Two new unit tests cover the strip helper and the existing
`compile_cwd_from_output` test was updated to reflect the new contract.
## Bug 2 — .S/.s assembly files silently dropped from library archives
`crates/fbuild-packages/src/library/library_info.rs::is_source_file`
only matched `.c/.cpp/.cc/.cxx`, so hand-written assembly stubs were
never compiled into the library archive. On esp32p4 the FastLED RX
ISR path (`fl::GpioIsrRxMcpwm::begin` →
`src/platforms/esp/32/drivers/gpio_isr_rx/fast_isr.S` →
`gpio_fast_edge_isr`) failed at link with:
ld.exe: libFastLED.a(platforms+_f948.o): in function
`fl::GpioIsrRxMcpwm::begin(fl::RxConfig const&)':
undefined reference to `gpio_fast_edge_isr'
Fix: extend `is_source_file` to include `S` and `s` extensions so the
project-as-library and per-library scanners pick them up.
## Verification
- `cargo test --release -p fbuild-build --lib zccache::` — 7/7 pass.
- End-to-end on the failing FastLED workload:
`bash compile esp32p4 --examples AutoResearch --no-filter` now
succeeds, producing a 1.7 MB firmware.bin in ~100 s.
📝 WalkthroughWalkthroughWindows zccache paths are normalized by stripping UNC extended-length prefixes before computing cache keys, ensuring consistency across different canonicalization behaviors. Additionally, assembly source files ( ChangesPath normalization and source file discovery
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/fbuild-packages/src/library/library_info.rs (2)
180-192: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd test coverage for assembly file recognition.
The change adds
.Sand.ssupport to fix real link failures, buttest_source_files_basiconly verifies C/C++ files. Given the importance of this fix (preventing undefined reference errors for hand-written ISRs), a test that explicitly creates and verifies assembly files are discovered would prevent regressions.🧪 Suggested test
#[test] fn test_source_files_includes_assembly() { let tmp = tempfile::TempDir::new().unwrap(); let src = tmp.path().join("src"); std::fs::create_dir_all(&src).unwrap(); std::fs::write(src.join("main.cpp"), "").unwrap(); std::fs::write(src.join("isr.S"), "").unwrap(); std::fs::write(src.join("lowlevel.s"), "").unwrap(); let lib = InstalledLibrary::new(tmp.path(), "test"); let files = lib.get_source_files(); assert_eq!(files.len(), 3); assert!(files.iter().any(|f| f.file_name().unwrap() == "isr.S")); assert!(files.iter().any(|f| f.file_name().unwrap() == "lowlevel.s")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-packages/src/library/library_info.rs` around lines 180 - 192, Add a new unit test (e.g., test_source_files_includes_assembly) next to test_source_files_basic that creates a temp src dir, writes files including "isr.S" and "lowlevel.s" alongside a C/C++ file, constructs InstalledLibrary::new(..., "test"), calls InstalledLibrary::get_source_files(), and asserts the returned list length and that it contains entries with file_name() equal to "isr.S" and "lowlevel.s" to ensure assembly (.S and .s) files are discovered.
69-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the doc comment to reflect assembly file support.
The comment claims this method returns only
.c, .cpp, .cc, .cxxfiles, but after the change at line 132, it also returns.Sand.sassembly files.📝 Proposed fix
- /// Get all source files (.c, .cpp, .cc, .cxx) in the library. + /// Get all source files (.c, .cpp, .cc, .cxx, .S, .s) in the library.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-packages/src/library/library_info.rs` at line 69, Update the doc comment above the method that "Get all source files (.c, .cpp, .cc, .cxx) in the library." to include assembly file extensions (.S and .s) so it matches the implementation change at line 132; edit the triple-slash Rust doc comment for the library source files getter to list ".c, .cpp, .cc, .cxx, .S, .s" (or equivalent phrasing) and ensure the comment remains concise and accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fbuild-build/src/zccache.rs`:
- Around line 447-451: The test function
strip_unc_prefix_removes_extended_length_marker has an assert_eq! that fails
rustfmt; run cargo fmt (or rustfmt) and reformat this test so the assert_eq! is
split across multiple lines per rustfmt rules (compare left and right operands
on separate lines), ensuring the call to strip_unc_prefix and the expected
PathBuf are formatted accordingly; locate the assert_eq! in
strip_unc_prefix_removes_extended_length_marker and apply cargo fmt to fix the
formatting.
---
Outside diff comments:
In `@crates/fbuild-packages/src/library/library_info.rs`:
- Around line 180-192: Add a new unit test (e.g.,
test_source_files_includes_assembly) next to test_source_files_basic that
creates a temp src dir, writes files including "isr.S" and "lowlevel.s"
alongside a C/C++ file, constructs InstalledLibrary::new(..., "test"), calls
InstalledLibrary::get_source_files(), and asserts the returned list length and
that it contains entries with file_name() equal to "isr.S" and "lowlevel.s" to
ensure assembly (.S and .s) files are discovered.
- Line 69: Update the doc comment above the method that "Get all source files
(.c, .cpp, .cc, .cxx) in the library." to include assembly file extensions (.S
and .s) so it matches the implementation change at line 132; edit the
triple-slash Rust doc comment for the library source files getter to list ".c,
.cpp, .cc, .cxx, .S, .s" (or equivalent phrasing) and ensure the comment remains
concise and accurate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64abf862-1264-48f7-a131-07f69b6c5ca2
📒 Files selected for processing (2)
crates/fbuild-build/src/zccache.rscrates/fbuild-packages/src/library/library_info.rs
Comment on lines
+447
to
+451
| fn strip_unc_prefix_removes_extended_length_marker() { | ||
| let raw = std::path::PathBuf::from(r"\\?\C:\Users\test\.fbuild\cache"); | ||
| let stripped = strip_unc_prefix(raw); | ||
| assert_eq!(stripped, std::path::PathBuf::from(r"C:\Users\test\.fbuild\cache")); | ||
| } |
There was a problem hiding this comment.
Fix rustfmt formatting violation.
The pipeline fails because cargo fmt --check expects the assert_eq! on line 450 to be formatted across multiple lines. Run cargo fmt to apply the correct formatting.
🎨 Suggested formatting fix
- assert_eq!(stripped, std::path::PathBuf::from(r"C:\Users\test\.fbuild\cache"));
+ assert_eq!(
+ stripped,
+ std::path::PathBuf::from(r"C:\Users\test\.fbuild\cache")
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn strip_unc_prefix_removes_extended_length_marker() { | |
| let raw = std::path::PathBuf::from(r"\\?\C:\Users\test\.fbuild\cache"); | |
| let stripped = strip_unc_prefix(raw); | |
| assert_eq!(stripped, std::path::PathBuf::from(r"C:\Users\test\.fbuild\cache")); | |
| } | |
| fn strip_unc_prefix_removes_extended_length_marker() { | |
| let raw = std::path::PathBuf::from(r"\\?\C:\Users\test\.fbuild\cache"); | |
| let stripped = strip_unc_prefix(raw); | |
| assert_eq!( | |
| stripped, | |
| std::path::PathBuf::from(r"C:\Users\test\.fbuild\cache") | |
| ); | |
| } |
🧰 Tools
🪛 GitHub Actions: Formatting / 0_Formatting.txt
[error] 447-451: cargo fmt --check failed (rustfmt). Formatting diff detected in test strip_unc_prefix_removes_extended_length_marker: assert_eq! formatting expected multiline layout.
🪛 GitHub Actions: Formatting / Formatting
[error] 447-452: cargo fmt --all -- --check failed due to formatting differences in test assertion. The expected formatting did not match the checked-in code (Diff shows assert_eq formatting changed to multi-line).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-build/src/zccache.rs` around lines 447 - 451, The test function
strip_unc_prefix_removes_extended_length_marker has an assert_eq! that fails
rustfmt; run cargo fmt (or rustfmt) and reformat this test so the assert_eq! is
split across multiple lines per rustfmt rules (compare left and right operands
on separate lines), ensuring the call to strip_unc_prefix and the expected
PathBuf are formatted accordingly; locate the assert_eq! in
strip_unc_prefix_removes_extended_length_marker and apply cargo fmt to fix the
formatting.
This was referenced May 27, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two bugs in the Windows compile path prevent FastLED's AutoResearch sketch from building for esp32p4 via fbuild. Both fixed here.
Cross-ref: FastLED/FastLED#2507 (meta issue), FastLED/FastLED#2493 (the PARLIO perf-trace work this unblocks).
Bug 1 —
\?\UNC prefix mangled into//?/in response filesPath::canonicalizeon Windows returns paths prefixed with the extended-length namespace marker\?\. That prefix only works with backslash separators, butfbuild-core::response_file::replace_path_backslashesrewrites all backslashes to forward slashes so gcc doesn't interpret them as escape sequences. The result was-I//?/C:/…on every include directory.Some headers happened to resolve (gcc tolerates the prefix on some lookups) but chip-specific SoC headers like
soc/soc_caps.hdid not:Fix: strip the
\?\prefix incanonicalize_existing_path(cfg(windows) only). New helperstrip_unc_prefixremoves both\?\and the defensively-handled//?/form, leavingC:\…which survives the backslash→slash rewrite as the standardC:/…form gcc accepts.path_arg_for_compile_cwdalso strips from itscwdargument sostrip_prefixmatches when callers pass an un-stripped canonical path.Trade-off: long-path support (>260 chars) is lost for stripped paths, but the cache root is
<home>/.fbuildwhich is well under the Windows limit in practice.Bug 2 —
.S/.sassembly files silently dropped from library archiveslibrary_info::is_source_fileonly matched.c/.cpp/.cc/.cxx, so hand-written assembly stubs were never compiled into library archives. On esp32p4 the FastLED RX ISR path (fl::GpioIsrRxMcpwm::begin→src/platforms/esp/32/drivers/gpio_isr_rx/fast_isr.S→gpio_fast_edge_isr) failed at link:Fix: extend
is_source_fileto includeSandsextensions.Tests
cargo test --release -p fbuild-build --lib zccache::— 7/7 pass (5 existing + 2 new for the strip helper).compile_cwd_from_output_canonicalizes_existing_workspacetest was updated to reflect the new contract (callsstrip_unc_prefixon itsexpectedvalue).End-to-end verification
Before:
bash compile esp32p4 --examples AutoResearch --no-filterfailed during framework lib compile withsoc/soc_caps.h: No such file.After: succeeds — 1.7 MB firmware.bin produced in ~100 s.
Test plan
cargo test --release -p fbuild-build --lib zccache::🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features