Skip to content

fix(config): consume INI shell-quoting layer in parse_flags - #949

Merged
zackees merged 1 commit into
mainfrom
fix/947-ini-flag-dequote
Jul 2, 2026
Merged

fix(config): consume INI shell-quoting layer in parse_flags#949
zackees merged 1 commit into
mainfrom
fix/947-ini-flag-dequote

Conversation

@zackees

@zackeeszackees commented Jul 2, 2026

Copy link
Copy Markdown
Member

Summary

PlatformIO feeds build_flags/build_src_flags through Python shlex, so the standard string-valued define idiom -DNAME="\"Demo\"" must be dequoted at INI parse time. parse_flags previously preserved the quoting layer verbatim; downstream, prepare_flags_for_exec's \"" collapse (correct for the internal -DNAME=\"...\" convention used by board/SDK defines) then turned the macro value into ""Demo"", and every use site failed with unable to find string literal operator 'operator""Demo'.

Fix in parse_flags (single-pass tokenizer): quote delimiters group-and-strip, and \" outside single quotes is an escaped literal ". Backslashes not escaping a double quote stay literal, so Windows paths are untouched — the one deliberate divergence from POSIX shlex, now documented. All platforms that build user flags through the shared pipeline benefit (esp32/esp8266/rp2040/stm32).

Validation

  • New test_parse_flags_dequotes_shell_layer covering: outer-quoted escaped form, bare escaped form, quoted values with spaces, quoted Windows paths, single-quoted literals.
  • Full fbuild-config suite: 180 passed. fbuild-build --lib: 714 passed (Linux container, soldr cargo).
  • Real workload: NightDriverStrip env:demo (build_src_flags -DPROJECT_NAME="\"Demo\"") previously failed every PROJECT_NAME use site ~11.5 min into a cold build in the meta: profiling-driven build performance burndown (Docker Linux, NightDriverStrip, cold vs hot cache) #942 Docker harness; with this fix the full compile and link succeed (compile-sketch=100.8s, firmware.elf produced — remaining harness failure is just esptool missing from the container image).

Closes#947
Part of #942

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved flag parsing so quoted values and escaped quotes are handled more consistently.
    • Fixed tokenization for inputs with spaces, Windows-style paths, and fully literal quoted sections.
    • Added a regression test to protect against double-consuming quote handling.
  • Documentation

    • Clarified how quoted flag values are interpreted during parsing, including how escaped double quotes are treated.

@coderabbitai

coderabbitaiBot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9c1d6cea-fe22-4a50-841f-3fef0ff7dd99

📥 Commits

Reviewing files that changed from the base of the PR and between a46aff0 and e1e55f8.

📒 Files selected for processing (2)
  • crates/fbuild-config/src/ini_parser/tests.rs
  • crates/fbuild-config/src/ini_parser/values.rs

📝 Walkthrough

Walkthrough

The parse_flags tokenizer in values.rs is rewritten to unescape backslash-escaped double quotes (\"") and to treat quote characters as delimiters instead of emitting them into output tokens. Documentation is updated accordingly, and a new regression test is added.

Changes

parse_flags quoting fix

Layer / File(s)Summary
Tokenizer rewrite and documentation
crates/fbuild-config/src/ini_parser/values.rs
parse_flags now uses a peekable iterator to convert \" (outside single quotes) into a literal " and excludes quote delimiters from output tokens; doc comment describes the new shell-quoting consumption semantics and its divergence from POSIX shlex.
Regression test for dequoting behavior
crates/fbuild-config/src/ini_parser/tests.rs
New test verifies parse_flags dequotes correctly for outer/bare escaped quoting, space-containing quoted values, Windows path quoting, and single-quoted literals.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

Not applicable — the change is a single-function tokenizer fix without multi-component interaction flow.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Linked Issues check⚠️ WarningThe parser fix and regression test are present, but the requested orchestrator audit and #942 build confirmation are not evidenced here.Add the esp8266/rp2040/stm32 shell_split audit or follow-up issues, and include evidence that NightDriverStrip env:demo now compiles past the prior failure.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title is concise and accurately summarizes the core parse_flags shell-quoting fix.
Out of Scope Changes check✅ PassedThe changes stay focused on parse_flags behavior and its regression tests, with no unrelated scope visible.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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/947-ini-flag-dequote

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.

PlatformIO feeds build_flags through Python shlex, so the standard
string-valued define idiom -DNAME="\"Demo\"" must be dequoted at
INI parse time: quote delimiters group-and-strip, and \" (outside
single quotes) is an escaped literal quote. Previously the outer quotes
survived into the token and prepare_flags_for_exec's \"->" collapse
turned the macro value into ""Demo"", failing every use site with
'unable to find string literal operator'. Backslashes not escaping a
double quote stay literal so Windows paths are untouched.
Reproduced with NightDriverStrip env:demo (build_src_flags
-DPROJECT_NAME="\"Demo\"") in the #942 profiling harness.
Closes#947
Part of #942
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@zackees
zackeesforce-pushed the fix/947-ini-flag-dequote branch from 54e44c3 to e1e55f8CompareJuly 2, 2026 21:16
@zackees

Copy link
Copy Markdown
MemberAuthor

@coderabbitai review

@coderabbitai

coderabbitaiBot commented Jul 2, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@zackees
zackees merged commit 41d7129 into mainJul 2, 2026
90 of 92 checks passed
@zackees
zackees deleted the fix/947-ini-flag-dequote branch July 2, 2026 21:47
Sign up for freeto 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.

fix(build): escaped quotes in build_flags defines are mangled before reaching gcc (ESP32 sketch compile)

1 participant

@zackees