Skip to content

chore(paths): route fbuild-deploy tool paths through fbuild-paths - #1383

Merged
zackees merged 1 commit into
mainfrom
chore/1349-ratchet-fbuild-path-batch1
Aug 23, 2026
Merged

chore(paths): route fbuild-deploy tool paths through fbuild-paths#1383
zackees merged 1 commit into
mainfrom
chore/1349-ratchet-fbuild-path-batch1

Conversation

@zackees

@zackees zackees commented Aug 23, 2026

Copy link
Copy Markdown
Member

First ratchet batch for #1349. Allowlist 48 → 43; all five fbuild-deploy
entries are gone.

The drift the ratchet was for

Five tool resolvers — lpc21isp, probe-rs, wchisp, wlink, and the LPC-Link2
debugger assets — each hand-rolled ~/.fbuild/{dev|prod}/tools. All five
disagreed with fbuild-paths about what dev mode is:

test with FBUILD_DEV_MODE=0
the five copies var_os("FBUILD_DEV_MODE").is_some() dev
fbuild_paths::is_dev_mode() var(..) == "1" prod

So an explicit opt-out sent every managed tool to the dev tree while every
other fbuild path stayed in prod. A tool installed exactly where fbuild's own
hint said to install it would then go on being not-found — and the hint
hardcoded prod, so under FBUILD_DEV_MODE=1 it named a directory the
resolver was not searching either.

The new test in lpc.rs fails on the old code with precisely that split:

assertion `left == right` failed: FBUILD_DEV_MODE="0" must resolve the same tools dir as fbuild-paths
  left: Some("C:\Users\niteris\.fbuild\dev\tools")
 right: Some("C:\Users\niteris\.fbuild\prod\tools")

This is the failure mode #1349 predicted, found by removing the duplication
rather than by hitting it in the field. It is a behavior change: with
FBUILD_DEV_MODE set to anything other than 1, managed tools now resolve
under prod, consistent with the rest of fbuild.

New in fbuild-paths

The call sites re-derived the path because the accessor they needed did not
exist — get_fbuild_root() panics when the home directory cannot be
determined, and these resolvers return Option/Result on purpose so a
home-less environment stays diagnosable rather than becoming a crash.

  • try_get_fbuild_root / try_get_tools_dir — the Option-returning forms
  • get_tools_dir / TOOLS_DIR_NAME — the managed-tools location itself
  • tools_dir_label — the ~/.fbuild/{dev|prod}/tools spelling for
    diagnostics, so an install hint names the tree fbuild will actually search

Verification

The lint is live on these files, not merely silent: with the allowlist entries
removed, a canary ".fbuild/build" literal reintroduced into wlink.rs fails
the build —

error: raw '.fbuild' path literal: use fbuild_paths (FBUILD_DIR_NAME, …)
error: could not compile `fbuild-deploy` (lib) due to 1 previous error

— and passes with the canary removed. soldr cargo test -p fbuild-deploy: 286
passed, 0 failed. Lint crate version bumped so the cached .so is rebuilt
against the shrunk allowlist.

Also hoists the module's existing env-var mutex over the two tests that now
read a FBUILD_DEV_MODE-derived path, with a drop-guard restore, so a failing
assertion cannot leave a flipped value behind for a sibling test.

Five tool resolvers in `fbuild-deploy` each hand-rolled
`~/.fbuild/{dev|prod}/tools`, and all five disagreed with `fbuild-paths`
about what dev mode is: they tested `var_os("FBUILD_DEV_MODE").is_some()`,
while `fbuild_paths::is_dev_mode()` requires the literal `1`. So
`FBUILD_DEV_MODE=0` — an explicit opt *out* — sent lpc21isp, probe-rs,
wchisp, wlink and the LPC-Link2 debugger assets to the dev tree while every
other fbuild path stayed in prod. A tool installed where fbuild said to
install it would then go on being not-found. The new test in `lpc.rs` fails
on the old code with exactly that split.

That is the drift #1349 predicted, found by removing the
duplication rather than by hitting it in the field.

`fbuild-paths` grows the accessors the call sites needed and did not have:

  - `try_get_fbuild_root` / `try_get_tools_dir` — the `Option`-returning
    forms. The resolvers report a missing home directory rather than
    panicking, and that is why they re-derived the path instead of calling
    `get_fbuild_root`, which panics.
  - `get_tools_dir` / `TOOLS_DIR_NAME` — the managed-tools location itself.
  - `tools_dir_label` — the `~/.fbuild/{dev|prod}/tools` spelling for
    diagnostics, so an install hint names the tree fbuild will actually
    search instead of hardcoding `prod`.

Allowlist: 48 -> 43. Lint crate version bumped so the cached `.so` is
rebuilt against the shrunk list.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@zackees, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 30 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: cb1dac00-3744-4526-a021-1ff14e7fe70c

📥 Commits

Reviewing files that changed from the base of the PR and between b6f7eb9 and 7ab8c11.

📒 Files selected for processing (8)
  • crates/fbuild-deploy/src/lpc.rs
  • crates/fbuild-deploy/src/lpc_debugger_reflash.rs
  • crates/fbuild-deploy/src/probe_rs.rs
  • crates/fbuild-deploy/src/wchisp.rs
  • crates/fbuild-deploy/src/wlink.rs
  • crates/fbuild-paths/src/lib.rs
  • dylints/ban_raw_fbuild_path/Cargo.toml
  • dylints/ban_raw_fbuild_path/src/allowlist.txt

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 merged commit 4d328d0 into main Aug 23, 2026
98 checks passed
@zackees
zackees deleted the chore/1349-ratchet-fbuild-path-batch1 branch August 23, 2026 18:12
@fastled-project-sync fastled-project-sync Bot moved this to Triage in FastLED Tracker Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

1 participant