fix(compile): force / in path args so cc1 spec-file pass works on Windows - #890
Merged
Merged
Conversation
…indows Follow-up to #885 (`fix(compile): pin TMP/TEMP for compiler subprocess on Windows`). With TMP/TEMP env now propagated correctly, gcc's driver finally manages to create its internal spec file — and as soon as it does, the next failure mode surfaces: cc1plus.exe: fatal error: srcmain.cpp: No such file or directory The driver writes args into the spec file using GCC's quoting rules, where `\` is an escape character. So `src\main.cpp` (which `path_arg_for_compile_cwd` returns on Windows because that's how the host OS spells it) is parsed by cc1 as `srcmain.cpp` and fails to open. Fix: convert backslashes to forward slashes for every path arg on Windows. Windows GCC has always accepted both separators for filesystem lookups; forward slashes additionally survive every spec-file / response-file pass unchanged. POSIX hosts pass through unmodified. Repro pre-fix (after #885): bash compile esp32dev --examples Blink → cc1plus.exe: fatal error: srcmain.cpp: No such file or directory Post-fix: clean Blink build in 117 s on Windows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 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 |
5 tasks
zackees
added a commit
to FastLED/FastLED
that referenced
this pull request
Jun 30, 2026
…ESP32 (#3446) (#3447) ## chip-id detection — three-strategy fallback chain (#3446) Previously the autoresearch port detection probed each port with a single 3.0 s esptool `chip-id` attempt using the default DTR/RTS auto- reset. On a CP210x ESP32-WROOM devkit that budget consistently undershoots — the realistic worst case is ~7 s (slow CP210x driver init + ~4 s of esptool SYNC retries + bootloader handshake), so healthy boards were misclassified as "device may not be in bootloader mode" and the entire bring-up gave up before any test could start. New behaviour: - Per-strategy timeout default is 7 s (covers the worst case while keeping a 5-port-all-empty sweep under ~10 s total). - Primary strategy is still `--before default-reset`. - ONLY when the primary attempt actually times out do we escalate to the fallback chain (`--before usb-reset` then `--before no-reset`). "Port open OK but no chip-id produced" stays a fast-path failure — empty ports don't drag the total budget out. - The caller in `_resolve_port` drops its explicit `timeout=3.0` override so it picks up the new default and the fallback chain. Local repro: `bash autoresearch esp32dev` with a CP210x ESP32-wroom on COM11 now resolves to `(esp32) ESP32` on the first attempt instead of `detection failed (esptool timed out after 3.0s)`. ## LegacyClocklessProxy — gate pin 8 on classic ESP32 `SK6812<8, RGB>` instantiation fires FastLED's `_ESPPIN<8, 256, false>::validpin() == false` static_assert because GPIO8 on the classic ESP32 (esp32dev / WROOM) is reserved for SPI flash (D2/HD). Newer ESP32 variants (S2/S3/C2/C3/C5/C6/H2/P4) repurpose GPIO8 and accept it as a valid output pin, and every non- ESP family treats pin 8 as a normal digital — so the gate is the narrow classic-ESP32 exclusion, not a broad change. Mirrors the existing `AUTORESEARCH_LEGACY_SUPPORTS_PIN_22` Teensy gate. Without this the sketch failed at compile time for esp32dev and autoresearch never got past the build phase. ## pyproject.toml — fbuild 2.3.15 pin note + version chase The pin stays at `fbuild==2.3.14` for now (2.3.15 isn't on PyPI yet as of this commit) but the comment history is updated to reflect the Windows compile env + path-separator fixes that landed in FastLED/fbuild#885 and FastLED/fbuild#890. Bump the pin to 2.3.15 in a follow-up once the release workflow publishes. Closes (pending CI): #3446 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jul 1, 2026
This was referenced Aug 23, 2026
zackees
added a commit
that referenced
this pull request
Aug 23, 2026
…#1372) * ci(dylint): add a Windows leg so Windows-only code is actually linted Closes #1359. `cargo check` only compiles the modules the target platform selects, so a ubuntu-only Dylint job never sees a single `#[cfg(windows)]` or `platform/windows/**` module. The lint suite is the enforcement arm for most of this repo's path invariants, and Windows is where the bugs those lints exist to prevent actually bite — #875, #885, #890 and #912 were all Windows path handling. The gate was pointed away from the platform that needs it. Not hypothetical. A full sweep on a Windows host against main reports: error: use fbuild_core::path::NormalizedPath::display_slash() instead of hand-rolled backslash-to-slash rewrite --> crates\fbuild-core\src\platform\windows\fs.rs:21:17 ## Fixing the violation, not excusing it `ban_manual_slash_normalize` allowlisted `fbuild-core/src/path.rs` as "the primitive itself". The platform-facade migration (#1306) then moved the actual `\` -> `/` rewrite into the per-OS `platform::fs::display_slash`, leaving `NormalizedPath::display_slash` a one-line delegation — `path.rs` no longer performs the transformation at all. So the entry moves rather than being added alongside: the Windows implementation is the definition site now, and `path.rs` is dropped from this lint's allowlist. Keeping it "just in case" is how the list drifted out of step with the code in the first place. `path.rs` keeps its entries on the *other* lints, which it still earns. Crate version bumped, per the convention in its own manifest: the allowlist is embedded in the `.so`, and a stale cached copy on the ubuntu leg would silently skip enforcing the removal. ## The workflow change A matrix with per-leg pinned `name`, not a derived one. `Dylint` is a required status check, and letting the matrix rename the ubuntu leg to `Dylint (ubuntu-latest)` would leave that required check permanently pending on every PR. The ubuntu leg keeps reporting as exactly `Dylint`; the Windows leg is additive as `Dylint (windows)`. `fail-fast: false`, so a Windows violation cannot mask a Linux one. `defaults.run.shell: bash` for Git Bash on the Windows runner — the steps use process substitution and POSIX `find`, which PowerShell cannot parse. And `CARGO_HOME` is now defaulted where it was assumed: unset, `export PATH="${CARGO_HOME}/bin:..."` would silently prepend a bare `/bin`. ## Verified on Windows, which is the new leg Every step of the job was run locally on a Windows host: - full `dylint --all -- --workspace --all-targets` sweep: **0 violations** - the observed-comparison that closes the job (`enforce_platform_boundary.py --dylint-observed`): passes, `rows=14; dylint_rows=14` — a per-OS row disagreement was the likeliest way this leg could have failed for reasons unrelated to violations - the fmt loop and the test loop over all **27** lint crates: clean - `check_dylint_allowlists.py`, `render_workflows.py --check`, and `check_workflow_concurrency.py`: pass ## Not included: a macOS leg Same one-line matrix addition, deliberately deferred. `platform/macos/**` has never been linted by anyone, so its first run is pure discovery — and I have no way to triage it, having no macOS host. Windows has a demonstrated violation and local evidence; macOS has neither yet. Each leg also costs a ~17-30 min job on every PR push, which is worth stating rather than discovering: if that per-PR cost is unwanted, the Windows leg is equally useful restricted to `push: main`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci(dylint): scope the lint crates' own tests to the ubuntu leg The new Windows leg went red on its first run, and the finding is worth recording because it is not what the leg exists to catch. The lints themselves are clean on Windows — the workspace sweep passes. What failed is the "Test Dylint libraries" step, which runs each lint crate's own compiletest ui fixtures: error: could not load library `.../target/dylint-tests/debug/ban_manual_slash_normalize@nightly-2026-04-16.dll`: LoadLibraryExW failed Dylint 6.0.1 looks for its test library under `<target>/debug` while soldr sets `CARGO_BUILD_TARGET`, so cargo writes to `<target>/<host>/debug`. Each lint's `fn ui` already clears that variable — that is what the comment in every one of them is about — but on the runner the load still fails. A dylint/soldr/compiletest interaction, not an fbuild defect. Rather than work around it, the step is scoped to ubuntu, and the scoping is principled rather than a dodge: those are the lint crates' *own* tests, asserting each lint fires on its fixture. That behavior is platform- independent and already covered once. What the Windows leg uniquely provides is compiling Windows-gated *workspace* source so the lints can see it — which is the entire point of #1359, and which passes. The same reasoning scopes the fmt loop and the three Python validators, none of which learn anything from a second OS. Side benefit: the Windows leg gets ~10 minutes shorter, which matters for a job that runs on every PR push. The Windows leg now runs exactly the steps verified locally on a Windows host: the full sweep (0 violations) and the observed-comparison (`rows=14; dylint_rows=14`). Refs #1359 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci(dylint): pin the workflow token to contents: read CodeRabbit: the workflow had no `permissions` block, so it inherited the repository/organization default, which can include write access. Every step in this job reads — checkout, a shallow `git fetch origin main` for the shrink-only allowlist diff, and the lint runs themselves. Nothing writes, so nothing should be able to. Worth noting this is not yet the repo-wide convention: 8 of 106 workflows set `permissions` today. Hardening the rest is a worthwhile sweep but belongs in its own change rather than riding along here. Refs #1359 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Follow-up to #885 (
fix(compile): pin TMP/TEMP for compiler subprocess on Windows). With TMP/TEMP env now propagated correctly, gcc's driver finally manages to create its internal spec file — and as soon as it does, the next failure mode surfaces:The driver writes args into the spec file using GCC's quoting rules, where
\is an escape character. Sosrc\main.cpp(whichpath_arg_for_compile_cwdreturns on Windows because that's how the host OS spells it) is parsed by cc1 assrcmain.cppand fails to open.Fix
Convert backslashes to forward slashes for every path arg on Windows. Windows GCC has always accepted both separators for filesystem lookups; forward slashes additionally survive every spec-file / response-file pass unchanged. POSIX hosts pass through unmodified.
Repro
Pre-fix (after #885 lands):
Post-fix: clean Blink build in 117s on Windows (esp32dev, FastLED).
Test plan
esp32dev) Blink compile succeeds end-to-end on Windows 10.cfg!(windows)gate).🤖 Generated with Claude Code