Add native extra_scripts flag runtime bridge#42
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces PlatformIO Changes
Sequence Diagram(s)sequenceDiagram
participant BuildCtx as Build Context
participant ScriptRT as Script Runtime
participant PyHarness as Python Harness
participant PIOScripts as PlatformIO Scripts
participant Compiler as Compiler
participant Linker as Linker
BuildCtx->>ScriptRT: resolve_extra_script_overlay(env_name)
ScriptRT->>ScriptRT: Load extra_scripts from config
ScriptRT->>PyHarness: Write harness + input.json to temp dir
ScriptRT->>PyHarness: Execute with project_dir as cwd
PyHarness->>PyHarness: Create MockEnv with mutation tracking
PyHarness->>PIOScripts: exec(pre_scripts) - PRE phase
PIOScripts->>PyHarness: Append/Prepend/Replace mutations
PyHarness->>PyHarness: Capture mutations into scopes
PyHarness->>PIOScripts: exec(post_scripts) - POST phase
PIOScripts->>PyHarness: Append/Prepend/Replace mutations
PyHarness->>ScriptRT: Return JSON with env state + unsupported list
ScriptRT->>ScriptRT: Parse result, convert to BuildOverlay
ScriptRT->>BuildCtx: Return LanguageExtraFlags + LinkExtraFlags
BuildCtx->>Compiler: compile_sources with LanguageExtraFlags
Compiler->>Compiler: Resolve per-source flags via for_source()
Compiler->>Compiler: Apply language-specific flags (C/CXX/ASM)
Compiler-->>BuildCtx: Compiled objects
BuildCtx->>Linker: link with LinkExtraArgs
Linker->>Linker: Append extra.flags to command
Linker->>Linker: Append extra.libs to libraries
Linker-->>BuildCtx: Linked output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fbuild-build/src/linker.rs (1)
80-96:⚠️ Potential issue | 🟠 MajorInclude overlay link inputs in the relink cache key.
Line 90 only compares
firmware.elfagainst object mtimes. Ifextra_scriptschangesextra.flagsorextra.libs, this branch will still reuse the old ELF, so new-L/-l/linker flags do not take effect until some object changes. At minimum, disable the fast path whenextrais non-empty; ideally persist a fingerprint ofextraalongside the ELF.Suggested guard
- let can_skip = elf_is_up_to_date( + let can_skip = extra.flags.is_empty() + && extra.libs.is_empty() + && elf_is_up_to_date( &candidate_elf, sketch_objects.iter().chain(core_objects.iter()), - ); + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/src/linker.rs` around lines 80 - 96, The fast-path in link_all currently only checks object mtimes via elf_is_up_to_date and ignores LinkExtraArgs (extra.flags/extra.libs), so changes to linker flags/libs are not picked up; update link_all to include extra in the relink cache key by either (a) refusing the fast path when extra is non-empty (i.e., treat can_skip as false if extra.flags or extra.libs is non-empty) or (b) compute a stable fingerprint (hash) of the LinkExtraArgs (flags, libs, scripts, etc.), persist that fingerprint alongside candidate_elf, and compare it before reusing firmware.elf; reference functions/values: link_all, elf_is_up_to_date, LinkExtraArgs (extra.flags/extra.libs), candidate_elf, and convert_firmware when implementing the persistent fingerprint approach.
🧹 Nitpick comments (1)
crates/fbuild-config/src/ini_parser.rs (1)
659-673: Consider reusingparse_list_valuesfromparse_lib_depsto avoid drift.You now have two near-identical parsing paths for multiline/comma-separated values.
♻️ Suggested simplification
fn parse_lib_deps(deps_str: &str) -> Vec<String> { - let mut result = Vec::new(); - - for line in deps_str.lines() { - for dep in line.split(',') { - let trimmed = dep.trim(); - if !trimmed.is_empty() { - result.push(trimmed.to_string()); - } - } - } - - result + parse_list_values(deps_str) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-config/src/ini_parser.rs` around lines 659 - 673, The file contains a duplicate multiline/comma-separated parsing path that duplicates logic from parse_lib_deps; remove the duplicate by reusing the existing parse_list_values implementation (or extract the shared logic into a single helper used by both) and update callers (e.g., parse_lib_deps and any other function currently duplicating the logic) to call that single function; ensure the helper signature and trimming/empty-item behavior remain identical so behavior doesn't change and only one implementation (parse_list_values or the new shared helper) remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fbuild-build/src/pipeline.rs`:
- Around line 478-498: The current src_overlay construction includes
ctx.project_compile_overlay and then that same src_overlay is reused for
compile_local_libraries(), leaking project-only (projenv) flags into local lib/*
builds; create a separate overlay for library compilation that omits
ctx.project_compile_overlay. Concretely, keep the existing user_overlay and the
src_overlay (which includes ctx.project_compile_overlay) for project-source
compilation, but build a new LanguageExtraFlags (e.g., lib_overlay or
local_lib_overlay) that combines user_overlay and the src-specific flags minus
ctx.project_compile_overlay, and pass that new overlay into
compile_local_libraries() instead of src_overlay; update any references to
src_overlay in the compile_local_libraries() call to use the new overlay so
projenv mutations do not affect local libraries.
In `@crates/fbuild-build/src/script_runtime_harness.py`:
- Around line 184-190: The resolve_script_entry function incorrectly treats any
":" in raw as a prefix separator, which breaks Windows absolute paths (e.g.,
"C:\..."); change the logic in resolve_script_entry so it only interprets a
prefix if raw actually starts with "pre:" or "post:" (e.g., check
raw.startswith(("pre:", "post:"))), otherwise treat the entire raw as a path and
return "post" with os.path.abspath(env.subst(raw)); keep raising RuntimeFailure
for unsupported prefixes but only after confirming the string truly begins with
a prefix.
In `@crates/fbuild-build/src/script_runtime.rs`:
- Around line 258-276: In find_python(), ensure we prefer "py -3" on Windows and
validate the interpreter is Python >= 3.6 by parsing the output of running
candidate --version (or stderr/stdout as appropriate) and rejecting candidates
that report Python < 3.6; update the Windows candidates slice to try &["py",
"-3"] before &["python"], and modify the loop that runs
command.arg("--version").output() to capture and parse the version string
(extract major/minor numbers from output like "Python X.Y.Z") and only return
the candidate vector when major == 3 and minor >= 6 (or major > 3). Also ensure
the returned Vec<String> still contains the original candidate tokens (e.g.,
"py","-3") when accepted.
---
Outside diff comments:
In `@crates/fbuild-build/src/linker.rs`:
- Around line 80-96: The fast-path in link_all currently only checks object
mtimes via elf_is_up_to_date and ignores LinkExtraArgs (extra.flags/extra.libs),
so changes to linker flags/libs are not picked up; update link_all to include
extra in the relink cache key by either (a) refusing the fast path when extra is
non-empty (i.e., treat can_skip as false if extra.flags or extra.libs is
non-empty) or (b) compute a stable fingerprint (hash) of the LinkExtraArgs
(flags, libs, scripts, etc.), persist that fingerprint alongside candidate_elf,
and compare it before reusing firmware.elf; reference functions/values:
link_all, elf_is_up_to_date, LinkExtraArgs (extra.flags/extra.libs),
candidate_elf, and convert_firmware when implementing the persistent fingerprint
approach.
---
Nitpick comments:
In `@crates/fbuild-config/src/ini_parser.rs`:
- Around line 659-673: The file contains a duplicate multiline/comma-separated
parsing path that duplicates logic from parse_lib_deps; remove the duplicate by
reusing the existing parse_list_values implementation (or extract the shared
logic into a single helper used by both) and update callers (e.g.,
parse_lib_deps and any other function currently duplicating the logic) to call
that single function; ensure the helper signature and trimming/empty-item
behavior remain identical so behavior doesn't change and only one implementation
(parse_list_values or the new shared helper) remains.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ea86fe6-6deb-4a2a-ab2d-a7853d4f472f
📒 Files selected for processing (21)
crates/fbuild-build/Cargo.tomlcrates/fbuild-build/src/avr/avr_linker.rscrates/fbuild-build/src/ch32v/ch32v_linker.rscrates/fbuild-build/src/compile_database.rscrates/fbuild-build/src/esp32/esp32_linker.rscrates/fbuild-build/src/esp32/orchestrator.rscrates/fbuild-build/src/esp8266/esp8266_linker.rscrates/fbuild-build/src/flag_overlay.rscrates/fbuild-build/src/generic_arm/arm_linker.rscrates/fbuild-build/src/lib.rscrates/fbuild-build/src/linker.rscrates/fbuild-build/src/nrf52/nrf52_linker.rscrates/fbuild-build/src/parallel.rscrates/fbuild-build/src/pipeline.rscrates/fbuild-build/src/renesas/renesas_linker.rscrates/fbuild-build/src/sam/sam_linker.rscrates/fbuild-build/src/script_runtime.rscrates/fbuild-build/src/script_runtime_harness.pycrates/fbuild-build/src/silabs/silabs_linker.rscrates/fbuild-build/src/teensy/teensy_linker.rscrates/fbuild-config/src/ini_parser.rs
Comment on lines
+478
to
+498
| let user_overlay = LanguageExtraFlags { | ||
| common: ctx | ||
| .user_flags | ||
| .iter() | ||
| .cloned() | ||
| .chain(ctx.global_compile_overlay.common.iter().cloned()) | ||
| .collect(), | ||
| c: ctx.global_compile_overlay.c.clone(), | ||
| cxx: ctx.global_compile_overlay.cxx.clone(), | ||
| asm: ctx.global_compile_overlay.asm.clone(), | ||
| }; | ||
| let src_overlay = LanguageExtraFlags::combined(&[ | ||
| &user_overlay, | ||
| &LanguageExtraFlags { | ||
| common: ctx.src_flags.clone(), | ||
| c: Vec::new(), | ||
| cxx: Vec::new(), | ||
| asm: Vec::new(), | ||
| }, | ||
| &ctx.project_compile_overlay, | ||
| ]); |
There was a problem hiding this comment.
Keep projenv overlays off local lib/* sources.
Lines 489-498 fold ctx.project_compile_overlay into src_overlay, and Lines 569-576 reuse that same overlay for compile_local_libraries(). That leaks projenv mutations into local libraries even though this overlay is documented as project-source-only. Please build a separate library overlay that excludes ctx.project_compile_overlay.
Suggested split
let src_overlay = LanguageExtraFlags::combined(&[
&user_overlay,
&LanguageExtraFlags {
common: ctx.src_flags.clone(),
c: Vec::new(),
cxx: Vec::new(),
asm: Vec::new(),
},
&ctx.project_compile_overlay,
]);
+ let local_lib_overlay = LanguageExtraFlags::combined(&[
+ &user_overlay,
+ &LanguageExtraFlags {
+ common: ctx.src_flags.clone(),
+ c: Vec::new(),
+ cxx: Vec::new(),
+ asm: Vec::new(),
+ },
+ ]);
@@
let library_objects = compile_local_libraries(
compiler,
¶ms.project_dir,
&ctx.build_dir,
- &src_overlay,
+ &local_lib_overlay,
jobs,
&build_log_mutex,
)?;Also applies to: 568-576
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-build/src/pipeline.rs` around lines 478 - 498, The current
src_overlay construction includes ctx.project_compile_overlay and then that same
src_overlay is reused for compile_local_libraries(), leaking project-only
(projenv) flags into local lib/* builds; create a separate overlay for library
compilation that omits ctx.project_compile_overlay. Concretely, keep the
existing user_overlay and the src_overlay (which includes
ctx.project_compile_overlay) for project-source compilation, but build a new
LanguageExtraFlags (e.g., lib_overlay or local_lib_overlay) that combines
user_overlay and the src-specific flags minus ctx.project_compile_overlay, and
pass that new overlay into compile_local_libraries() instead of src_overlay;
update any references to src_overlay in the compile_local_libraries() call to
use the new overlay so projenv mutations do not affect local libraries.
Comment on lines
+184
to
+190
| def resolve_script_entry(env, raw): | ||
| if ":" in raw: | ||
| prefix, path = raw.split(":", 1) | ||
| if prefix not in ("pre", "post"): | ||
| raise RuntimeFailure(f"unsupported extra_scripts prefix '{prefix}'") | ||
| return prefix, os.path.abspath(env.subst(path)) | ||
| return "post", os.path.abspath(env.subst(raw)) |
There was a problem hiding this comment.
Don't parse Windows drive letters as extra_scripts prefixes.
Line 185 treats any : as a pre:/post: separator. An entry like C:\scripts\flags.py or C:/scripts/flags.py becomes prefix C and fails before execution, so absolute script paths are broken on Windows. Only strip a prefix when the string actually starts with pre: or post:.
Suggested fix
def resolve_script_entry(env, raw):
- if ":" in raw:
- prefix, path = raw.split(":", 1)
- if prefix not in ("pre", "post"):
- raise RuntimeFailure(f"unsupported extra_scripts prefix '{prefix}'")
- return prefix, os.path.abspath(env.subst(path))
+ if raw.startswith("pre:"):
+ return "pre", os.path.abspath(env.subst(raw[4:]))
+ if raw.startswith("post:"):
+ return "post", os.path.abspath(env.subst(raw[5:]))
return "post", os.path.abspath(env.subst(raw))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-build/src/script_runtime_harness.py` around lines 184 - 190,
The resolve_script_entry function incorrectly treats any ":" in raw as a prefix
separator, which breaks Windows absolute paths (e.g., "C:\..."); change the
logic in resolve_script_entry so it only interprets a prefix if raw actually
starts with "pre:" or "post:" (e.g., check raw.startswith(("pre:", "post:"))),
otherwise treat the entire raw as a path and return "post" with
os.path.abspath(env.subst(raw)); keep raising RuntimeFailure for unsupported
prefixes but only after confirming the string truly begins with a prefix.
Comment on lines
+258
to
+276
| fn find_python() -> Option<Vec<String>> { | ||
| let candidates: &[&[&str]] = if cfg!(windows) { | ||
| &[&["python"], &["py", "-3"]] | ||
| } else { | ||
| &[&["python3"], &["python"]] | ||
| }; | ||
|
|
||
| for candidate in candidates { | ||
| let mut command = Command::new(candidate[0]); | ||
| if candidate.len() > 1 { | ||
| command.args(&candidate[1..]); | ||
| } | ||
| if let Ok(output) = command.arg("--version").output() { | ||
| if output.status.success() { | ||
| return Some(candidate.iter().map(|s| (*s).to_string()).collect()); | ||
| } | ||
| } | ||
| } | ||
| None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "script_runtime" crates/fbuild-build/src/ --type fRepository: FastLED/fbuild
Length of output: 149
🏁 Script executed:
cat -n crates/fbuild-build/src/script_runtime.rs | head -50Repository: FastLED/fbuild
Length of output: 1960
🏁 Script executed:
wc -l crates/fbuild-build/src/script_runtime.rsRepository: FastLED/fbuild
Length of output: 103
🏁 Script executed:
cat -n crates/fbuild-build/src/script_runtime.rs | sed -n '250,290p'Repository: FastLED/fbuild
Length of output: 1475
🏁 Script executed:
test -f crates/fbuild-build/src/script_runtime_harness.py && cat crates/fbuild-build/src/script_runtime_harness.py || echo "File not found"Repository: FastLED/fbuild
Length of output: 8640
🏁 Script executed:
rg "find_python" crates/fbuild-build/src/ -A 3 -B 3Repository: FastLED/fbuild
Length of output: 1061
Add Python 3.6+ version check to the interpreter selection.
The harness file (script_runtime_harness.py) uses f-strings and other Python 3.6+ syntax. The current find_python() only validates that --version succeeds, which allows a Python 2 interpreter to be selected on Windows (where python is tried before py -3). Add an explicit version check to reject incompatible interpreters.
Additionally, reorder Windows candidates to prefer py -3 (which explicitly requests Python 3) before attempting a bare python call.
Suggested fix
fn find_python() -> Option<Vec<String>> {
let candidates: &[&[&str]] = if cfg!(windows) {
- &[&["python"], &["py", "-3"]]
+ &[&["py", "-3"], &["python"]]
} else {
&[&["python3"], &["python"]]
};
for candidate in candidates {
let mut command = Command::new(candidate[0]);
if candidate.len() > 1 {
command.args(&candidate[1..]);
}
- if let Ok(output) = command.arg("--version").output() {
+ if let Ok(output) = command
+ .arg("-c")
+ .arg("import sys; raise SystemExit(0 if sys.version_info >= (3, 6) else 1)")
+ .output()
+ {
if output.status.success() {
return Some(candidate.iter().map(|s| (*s).to_string()).collect());
}
}
}📝 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 find_python() -> Option<Vec<String>> { | |
| let candidates: &[&[&str]] = if cfg!(windows) { | |
| &[&["python"], &["py", "-3"]] | |
| } else { | |
| &[&["python3"], &["python"]] | |
| }; | |
| for candidate in candidates { | |
| let mut command = Command::new(candidate[0]); | |
| if candidate.len() > 1 { | |
| command.args(&candidate[1..]); | |
| } | |
| if let Ok(output) = command.arg("--version").output() { | |
| if output.status.success() { | |
| return Some(candidate.iter().map(|s| (*s).to_string()).collect()); | |
| } | |
| } | |
| } | |
| None | |
| fn find_python() -> Option<Vec<String>> { | |
| let candidates: &[&[&str]] = if cfg!(windows) { | |
| &[&["py", "-3"], &["python"]] | |
| } else { | |
| &[&["python3"], &["python"]] | |
| }; | |
| for candidate in candidates { | |
| let mut command = Command::new(candidate[0]); | |
| if candidate.len() > 1 { | |
| command.args(&candidate[1..]); | |
| } | |
| if let Ok(output) = command | |
| .arg("-c") | |
| .arg("import sys; raise SystemExit(0 if sys.version_info >= (3, 6) else 1)") | |
| .output() | |
| { | |
| if output.status.success() { | |
| return Some(candidate.iter().map(|s| (*s).to_string()).collect()); | |
| } | |
| } | |
| } | |
| None | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-build/src/script_runtime.rs` around lines 258 - 276, In
find_python(), ensure we prefer "py -3" on Windows and validate the interpreter
is Python >= 3.6 by parsing the output of running candidate --version (or
stderr/stdout as appropriate) and rejecting candidates that report Python < 3.6;
update the Windows candidates slice to try &["py", "-3"] before &["python"], and
modify the loop that runs command.arg("--version").output() to capture and parse
the version string (extract major/minor numbers from output like "Python X.Y.Z")
and only return the candidate vector when major == 3 and minor >= 6 (or major >
3). Also ensure the returned Vec<String> still contains the original candidate
tokens (e.g., "py","-3") when accepted.
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
extra_scriptsruntime bridge for common PlatformIO/SCons flag mutationsenv/projenvscope mutations into fbuild compile and link phases--platformioSupported runtime surface
pre:andpost:entries fromextra_scriptsImport("env")in pre/post scripts andImport("projenv")in post scriptsAppend,AppendUnique,Prepend,ReplaceCPPDEFINES,CPPPATH,CCFLAGS,CFLAGS,CXXFLAGS,ASFLAGS,LINKFLAGS,LIBPATH,LIBSStringifyMacro,subst,GetProjectOptionBehavior
Validation
rustup run stable-x86_64-pc-windows-msvc cargo fmt --allCARGO_TARGET_DIR=target-msvc rustup run stable-x86_64-pc-windows-msvc cargo check -qCARGO_TARGET_DIR=target-msvc rustup run stable-x86_64-pc-windows-msvc cargo check -q -p fbuild-build --testsCARGO_TARGET_DIR=target-msvc rustup run stable-x86_64-pc-windows-msvc cargo check -q -p fbuild-config --testsNotes
cargo testexecution is blocked in this environment by a GNU/MSVC test-linker mismatch, so test binaries were typechecked withcargo check --testsinsteadCloses #41
Refs #34
Refs #38
Refs #39
Refs #40
Summary by CodeRabbit
Release Notes
New Features
extra_scriptsconfiguration, allowing Python scripts to dynamically modify compiler flags, linker arguments, and library paths during the build process.Chores