Skip to content

fix(setup): locate fbuild binary via cargo --message-format=json#458

Merged
zackees merged 1 commit into
mainfrom
fix/setup-py-target-triple
Jun 7, 2026
Merged

fix(setup): locate fbuild binary via cargo --message-format=json#458
zackees merged 1 commit into
mainfrom
fix/setup-py-target-triple

Conversation

@zackees

@zackees zackees commented Jun 7, 2026

Copy link
Copy Markdown
Member

Summary

Drives cargo with --message-format=json-render-diagnostics and pulls the binary path out of the structured artifact stream — the same mechanism cargo install uses. Works regardless of how cargo lays out its target dir.

Relation to #461

PR #461 already shipped a fix for the same bug (filesystem probe of target/release/ + every target/<triple>/release/ subdir, max-mtime tiebreak). This PR is rebased on top of that merge and replaces the filesystem probe with the JSON-stream approach. Both fix the original "binary not at target\release\fbuild.exe" failure. This PR is materially more robust against:

  • Custom CARGO_TARGET_DIR — env-driven override, doesn't sit anywhere under the repo's target/ tree at all.
  • Custom profiles (--profile dist, etc.) — output dir becomes target/<triple>/<profile>/, which the mtime-tiebreak filesystem probe in fix(setup.py): locate cargo binary under target/<host-triple>/release/ #461 can't see.
  • Cargo's own conventions — if cargo ever rearranges its output layout (it has, twice in the last decade), the JSON stream still reports the right path.

The filesystem probe survives as a fallback for the cached-Fresh edge case where cargo doesn't emit compiler-artifact messages on stdout. So this PR is strictly a superset of #461's behaviour.

What changed

  • Runs cargo build --release -p fbuild-cli --message-format=json-render-diagnostics.
  • Captures stdout (JSON stream); stderr passes through so soldr's session summary stays visible.
  • Walks the artifact lines, picks the most recent compiler-artifact with target.name == "fbuild" and a non-null executable field.
  • Falls back to probing canonical layouts when the JSON stream produces nothing.
  • Error message when both fail lists every candidate path and points users at the issue tracker with the verbose-build command attached.

Test plan

  • Verified locally on Windows: uv pip install succeeds where the original target/release/<bin> check failed.
  • Binary lands at ci/bin/fbuild.exe as expected by the wheel packaging step.
  • fbuild --version from the newly-installed venv prints fbuild 2.2.19.
  • CI on this branch will exercise the cross-platform paths.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

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

More reviews will be available in 13 minutes and 16 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7002546c-ce6a-4eaa-a4a0-089ad7413623

📥 Commits

Reviewing files that changed from the base of the PR and between 3464d1c and 3368f01.

📒 Files selected for processing (1)
  • setup.py
📝 Walkthrough

Walkthrough

The PR strengthens setup.py's binary discovery logic by parsing Cargo's JSON diagnostic output to locate the compiled fbuild executable, handling per-host and per-layout target layouts. A filesystem search fallback is added if no JSON artifact is found, and the discovered binary is staged into ci/bin/.

Changes

Fbuild Binary Discovery

Layer / File(s) Summary
Documentation and imports
setup.py
Module docstring expanded to document Cargo's per-host/per-layout binary locations and JSON-based discovery strategy; json and Optional imports added and TARGET_DIR removed.
Binary discovery implementation
setup.py
_find_fbuild_executable_from_json() parses Cargo JSON output for the compiler-artifact entry; _find_fbuild_executable_by_search() searches target/ directories as a fallback; _build_fbuild_cli() runs soldr cargo build, orchestrates discovery, and stages the found binary.
Build process integration
setup.py
BuildWithCargo.run() now invokes _build_fbuild_cli() to build, discover, and stage the executable into ci/bin/ and fails with a clear error if the binary is not found.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

I nibble JSON lines in the night,
Sniffing artifacts till paths are right,
No brittle targets block my way,
I fetch the binary, tuck it away,
A rabbit's hop — build saved today. 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically describes the main change: improving the fbuild binary location discovery in setup.py by using cargo's JSON message format instead of assuming a fixed target directory layout.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/setup-py-target-triple

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 and usage tips.

@zackees
zackees force-pushed the fix/setup-py-target-triple branch from 3d1a743 to 3464d1c Compare June 7, 2026 04:24
@fastled-project-sync fastled-project-sync Bot moved this to Triage in FastLED Tracker Jun 7, 2026
`pip install .` failed on every Windows dev install where soldr sets
`CARGO_BUILD_TARGET=<host-triple>` (the default for this repo):

  ERROR: cargo build succeeded but binary not at
         C:\Users\niteris\dev\fbuild\target\release\fbuild.exe.

Cargo had written the binary to
`target/x86_64-pc-windows-msvc/release/fbuild.exe`, but `setup.py`
hard-coded `target/release/fbuild.exe` as the only place it would
look.

Switch to `cargo build --message-format=json-render-diagnostics`,
walk the structured artifact stream, and pull the real `executable`
path out of the compiler-artifact line. That's the same mechanism
`cargo install` uses and it works regardless of the target-dir
layout (host-triple, custom profile, custom CARGO_TARGET_DIR).

Falls back to searching `target/release/` and every
`target/<triple>/release/` subdir when cargo's JSON stream is empty
(e.g. a fully-cached `Fresh` run that skips emitting artifact
messages on stdout).

Verified locally on Windows: `uv run python -c "import fbuild"`
now succeeds during `uv pip install`, the binary lands in
`ci/bin/fbuild.exe`, and `fbuild --version` prints `fbuild 2.2.19`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zackees
zackees force-pushed the fix/setup-py-target-triple branch from 3464d1c to 3368f01 Compare June 7, 2026 05:11
@zackees
zackees merged commit 357629c into main Jun 7, 2026
82 of 86 checks passed
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