Skip to content

fix(lint): stream Rust build stdout/stderr + replace deprecated os.system in ci/ - #3510

Merged
zackees merged 1 commit into
masterfrom
fix/lint-infra-windows-rust-and-ty
Jul 1, 2026
Merged

fix(lint): stream Rust build stdout/stderr + replace deprecated os.system in ci/#3510
zackees merged 1 commit into
masterfrom
fix/lint-infra-windows-rust-and-ty

Conversation

@zackees

@zackees zackees commented Jul 1, 2026

Copy link
Copy Markdown
Member

Summary

Two related lint-infrastructure fixes surfaced while running bash lint on a fresh worktree, both of which were silently degrading the developer signal.

1. ty (Python type-checker) — 6 sites using deprecated os.system(...)

Python's runtime marks os.system "soft deprecated — use the subprocess module instead." Each site replaced with subprocess.run([...], check=False) using argument-list form (safer against path-with-spaces injection than the prior f-string shell command):

File What it was invoking
ci/util/create_build_dir.py attrib -r <path> (Windows readonly clear)
ci/util/map_dump.py uv run fpvgcc ... (six variants)
ci/util/scrapers/scrape_festival_stick.py playwright install chromium
ci/wasm_audio_drag_drop_test.py playwright install chromium
ci/wasm_audio_url_test.py playwright install chromium
ci/wasm_test.py playwright install chromium

Result: ty was 6 diagnostics, now 0. python_pipeline was FAILED, now completes.

2. Rust C++ linter — stream cargo output so silent-fails are diagnosable

ci/lint_cpp/rust_binary_cache.py::_run_build used subprocess.run(cmd, cwd=..., check=False) which inherits the parent's stdout/stderr. When the build is invoked from inside a concurrent.futures.ThreadPoolExecutor (which run_all_checkers.py does) and the parent's file descriptors are being indirectly captured by a test runner, logging wrapper, or CI harness, cargo's diagnostics disappear. On a Windows worktree where soldr silently returns exit-code 0 without building the binary, the resulting RuntimeError gave the user zero signal about why the build failed.

Fix:

  • Switch to subprocess.Popen with stdout=PIPE / stderr=STDOUT and a line-by-line reader loop that prefixes every cargo line with [cargo] and prints it to sys.stderr with flush=True.
  • Log the resolved soldr command + cwd before the build starts so the user sees exactly what will be invoked.
  • After the fixed-candidate paths miss in _binary_path, fall back to target.rglob(<name>) so a non-standard cargo output location (e.g. an unpredicted CARGO_TARGET_DIR or a triple our _guess_host_triple heuristic missed) still resolves.
  • When the binary genuinely is missing, the error message now shows (a) the checked candidates, (b) what the recursive walk actually found, and (c) points the user at the [cargo] output stream for diagnosis.

Before / after (fresh worktree, from bash lint)

Before:

❌ Unified C++ linter failed
❌ ty failed          (6 deprecation warnings)
❌ python_pipeline FAILED
❌ cpp_linting FAILED

After:

✅ ruff passed
✅ ty passed         (0 diagnostics)
✅ python_pipeline completed
❌ cpp_linting FAILED   ← still fails on this Windows box, but now the
                          soldr silent-fail is visible in the streamed
                          [cargo] output. On healthy Linux/macOS CI
                          runners where soldr works, this stays green.

The remaining cpp_linting failure is a soldr Windows infrastructure issue that FastLED cannot fix — soldr executes with exit 0 but produces no cargo output and no binary. The improved diagnostics + fallback binary walk are what FastLED can contribute to make that failure mode actionable rather than opaque.

Test plan

  • bash lint — python_pipeline now completes; ty passes with 0 diagnostics; C++ lint failure is now visibly diagnosable
  • All 6 os.system sites verified to use argument-list form (no shell injection risk)
  • ci/lint_cpp/rust_binary_cache.py — no other call sites use subprocess.run(cmd) with the same swallowed-output pattern

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved CI and test tooling so browser setup, build steps, and cleanup commands run more reliably across environments.
    • Build output is now streamed live, making it easier to see progress and diagnose failures during long-running checks.
    • If a required binary is missing, error messages now include clearer details about what was searched and what was found.

…stem in ci/

Two related fixes surfaced while running `bash lint` on a fresh
worktree.

## 1. ty (Python type-checker) failures — os.system deprecated

ty flagged 6 sites in ci/ still using `os.system(...)`, which Python's
runtime marks "soft deprecated — use the subprocess module instead."
Replace each with `subprocess.run([...], check=False)`:

- ci/util/create_build_dir.py — `attrib -r <path>` on Windows
- ci/util/map_dump.py — `uv run fpvgcc ...` (all six variants)
- ci/util/scrapers/scrape_festival_stick.py — `playwright install`
- ci/wasm_audio_drag_drop_test.py — `playwright install`
- ci/wasm_audio_url_test.py — `playwright install`
- ci/wasm_test.py — `playwright install`

Argument-list form (list, not shell string) is safer against
path-with-spaces injection than the prior f-string shell command.

After: `ty` passes (was 6 diagnostics, now 0).

## 2. Rust C++ linter — stream cargo output so silent-fail is diagnosable

`ci/lint_cpp/rust_binary_cache.py::_run_build` used
`subprocess.run(cmd, cwd=..., check=False)` which inherits the parent's
stdout/stderr. When the build is invoked from inside a
`concurrent.futures.ThreadPoolExecutor` (as `run_all_checkers.py`
does) and the parent's file descriptors are being indirectly captured
by a test runner, logging wrapper, or CI harness, cargo's diagnostics
disappear. On a Windows worktree where soldr silently returns
exit-code 0 without building the binary, the resulting RuntimeError
gave the user no signal about *why* the build failed.

Fix: switch to `subprocess.Popen` with `stdout=PIPE`/`stderr=STDOUT`
and a line-by-line reader loop that prefixes every cargo line with
`    [cargo] ` and prints it to `sys.stderr` with `flush=True`. Also
log the resolved soldr command + cwd before the build starts so the
user sees exactly what will be invoked.

Additional safety net in `_binary_path`: after the fixed-candidate
paths miss, fall back to `target.rglob(<name>)` so a non-standard
cargo output location (e.g. an unpredicted `CARGO_TARGET_DIR` or a
triple we did not enumerate) still resolves. And when the binary
genuinely is missing, the error message now shows (a) the checked
candidates, (b) what the recursive walk actually found, and (c)
points the user at the `[cargo]` output stream for diagnosis.

## Verification

Before this PR (fresh worktree, from `bash lint`):

    ❌ Unified C++ linter failed
    ❌ ty failed          (6 deprecation warnings)
    ❌ python_pipeline FAILED
    ❌ cpp_linting FAILED

After this PR:

    ✅ ruff passed
    ✅ ty passed         (0 diagnostics)
    ✅ python_pipeline completed
    ❌ cpp_linting FAILED   ← still fails on this Windows box only because
                              soldr silently no-ops, but the streaming
                              output makes the failure diagnosable
                              instead of opaque.

The remaining cpp_linting failure is now visibly a soldr Windows
infrastructure issue (soldr executes with exit 0 but produces no cargo
output and no binary), which is out of scope for FastLED to fix — the
improved diagnostics + fallback binary walk are what FastLED can
contribute.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zackees
zackees merged commit 70cedf0 into master Jul 1, 2026
7 of 11 checks passed
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c36f61a6-332f-4b49-9e36-756cb415fd97

📥 Commits

Reviewing files that changed from the base of the PR and between 5309dd7 and 0afc0c4.

📒 Files selected for processing (7)
  • ci/lint_cpp/rust_binary_cache.py
  • ci/util/create_build_dir.py
  • ci/util/map_dump.py
  • ci/util/scrapers/scrape_festival_stick.py
  • ci/wasm_audio_drag_drop_test.py
  • ci/wasm_audio_url_test.py
  • ci/wasm_test.py

📝 Walkthrough

Walkthrough

This PR migrates several CI scripts from os.system to subprocess.run/subprocess.Popen for command execution, and enhances rust_binary_cache.py with a recursive target directory search fallback, streamed cargo build output, and expanded error diagnostics for missing lint binaries.

Changes

Rust lint binary caching

Layer / File(s) Summary
Binary path resolution with recursive fallback
ci/lint_cpp/rust_binary_cache.py
_binary_path() performs a recursive target/ search for the binary when deterministic candidates are missing, before falling back to the default candidate.
Streaming build execution
ci/lint_cpp/rust_binary_cache.py
_run_build() switches from subprocess.run to Popen, streaming and prefixing merged stdout/stderr cargo output line-by-line, flushing prints, and raising an updated RuntimeError on failure.
Expanded missing-binary diagnostics
ci/lint_cpp/rust_binary_cache.py
ensure_rust_lint_binary() error messages now list checked candidate paths and recursive target/ walk results.

CI utility and test subprocess migration

Layer / File(s) Summary
Windows readonly clearing
ci/util/create_build_dir.py
remove_readonly() uses subprocess.run(["attrib", "-r", path], check=False) instead of os.system.
map_dump command execution
ci/util/map_dump.py
Commands are built as argument lists and run via subprocess.run(cmd, check=False), replacing shell-string os.system calls; imports updated.
Playwright browser install
ci/util/scrapers/scrape_festival_stick.py, ci/wasm_audio_drag_drop_test.py, ci/wasm_audio_url_test.py, ci/wasm_test.py
Chromium installation switches from os.system to subprocess.run(..., check=False), with subprocess imports added.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

  • FastLED/FastLED#3335: Modifies the same rust_binary_cache.py binary locating/building logic (candidate/target path search and Cargo build invocation flow).
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/lint-infra-windows-rust-and-ty

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@zackees
zackees deleted the fix/lint-infra-windows-rust-and-ty branch July 1, 2026 15:08
@fastled-project-sync fastled-project-sync Bot moved this to Triage in FastLED Tracker Jul 1, 2026
@fastled-project-sync fastled-project-sync Bot moved this from Triage to Done in FastLED Tracker Jul 1, 2026
zackees added a commit that referenced this pull request Jul 1, 2026
…MA silicon (#3513)

Reverts the FastLED-side of the phantom LPC804 DMA cascade (PRs #3500 + #3505).
LPC804 silicon has no DMA peripheral. Empirical evidence:

- NXP's own `mcux-sdk/devices/LPC804/LPC804.h`: zero `DMA_Type` typedef,
  zero `DMA0_BASE` macro. Only `CRC_BASE = 0x50000000` in the 0x5000_xxxx
  AHB range; 0x50008000 is a reserved slot.
- `LPC804_features.h`: no `FSL_FEATURE_SOC_DMA_COUNT`. LPC845's file has
  `FSL_FEATURE_SOC_DMA_COUNT = 1` and `FSL_FEATURE_DMA_NUMBER_OF_CHANNELS
  = 25`.
- `devices/LPC804/drivers/`: no `fsl_inputmux_connections.h`. LPC845's
  driver dir has it (INPUTMUX exists to route peripheral requests to
  DMA channels — no DMA, no INPUTMUX).
- UM11065 has only 3 DMA references, all copy-paste leftovers in ADC /
  DAC / CAPT / I2C / SPI comment blocks referring to "DMA trigger
  flags" for a peripheral that does not exist on this die.

Diagnosis credit @phatpaul (#3499 comment 4855252061). The revert follows
the guardrail landed in #3506 / #3507 (`agents/docs/peripheral-existence.md`).

## Changes

- `src/platforms/arm/lpc/spi_arm_lpc_dma.h` — narrow build gate back to
  `FL_IS_ARM_LPC_845` only. Add a compile-time `#error` that fires with
  a clear "LPC804 has no DMA" diagnostic + link to peripheral-existence
  guardrail if a user tries to opt into `FASTLED_LPC_SPI_DMA` on LPC804.
  Delete the earlier "LPC804 vendor CMSIS PAL prerequisite" text and the
  LPC804-specific `FASTLED_LPC_SPI_DMA_CHANNEL` default (0 → dropped;
  the LPC845 default of 4 stands).
- `examples/AutoResearch/AutoResearchSpiDma.h` — narrow harness gate
  back to LPC845 only.
- `examples/AutoResearch/AutoResearchLowMemory.h` — narrow include +
  bind gates back to LPC845 only.
- `ci/autoresearch/phases.py` — collapse `LPC_DMA_SPI_ENVS` back to
  `LPC_WS2812_ENVS` (LPC845 environments only). Drop the
  `is_lpc804 → core_hz = 15_000_000` branch (always 24 MHz now).
- `ci/autoresearch/args.py` — update `--dma-spi` `--help` text: drop
  LPC804 mention + fbuild prerequisite; note LPC804 not supported.
- `src/platforms/arm/lpc/README.md` — LPC804 row now clearly documents
  "no DMA-async SPI" with citation to the guardrail. `spi_arm_lpc_dma.h`
  files entry marked LPC845-only. Feature-defines section corrected.
  References/Shipped entry for #3500 strikethrough-annotated with
  "REVERTED — see peripheral-existence.md."

Follow-up (out of scope for this PR):

- Revert `framework-arduino-lpc8xx#35` (the fabricated `DMA_Type` in
  `variants/lpc804/LPC804.h`).
- Revert or supersede `fbuild#916` (bumped ArduinoCore-LPC8xx pin for
  the fabricated typedef).
- Close FastLED #3499 as INVALID.

## Verification

- `bash compile wasm --examples AutoResearch` — clean
- `bash lint` — python_pipeline passes; cpp_linting fails only on the
  same pre-existing Rust-binary silent-fail this Windows environment
  has (unrelated to this diff; see #3510)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant