builder: espidf: fix flag/arg pairing across compileCommandFragments - #1756
Open
94xhn wants to merge 1 commit into
Open
builder: espidf: fix flag/arg pairing across compileCommandFragments#175694xhn wants to merge 1 commit into
94xhn wants to merge 1 commit into
Conversation
prepare_build_envs() paired a flag like -include or -isystem with its argument by grabbing the *entire next* compileCommandFragments entry's fragment string, e.g. build_flag + " " + compile_commands[i + 1]["fragment"]. That only happens to work if the argument is the only thing in that next fragment; if CMake split -include and its path into two fragments where the second fragment contains the path plus more flags (eg esp_lvgl_adapter's target_compile_options(... PUBLIC -include "path")), the whole remainder of that fragment gets swallowed as one bogus argument, and unrelated flags after it are lost. Flatten all fragments into a single token stream first, so a flag needing an argument is paired with just the next token regardless of which fragment it originally came from. Also re-quote that argument if it contains a space before handing the joined string to ParseFlags(), since ParseFlags() re-splits string arguments with shlex.split() and would otherwise break an unquoted path apart again. Fixes platformio#1730 Signed-off-by: yi chen <94xhn1@gmail.com>
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
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.
Description
prepare_build_envs()paired a flag like-includeor-isystemwith its argument by grabbing the entire nextcompileCommandFragmentsentry's fragment string (build_flag + " " + compile_commands[i + 1]["fragment"]). That only works if the argument is the only thing in that next fragment. If CMake splits-includeand its path into two fragments where the second fragment contains the path plus more flags — as reported in #1730 withesp_lvgl_adapter'starget_compile_options(... PUBLIC -include "path")— the whole remainder of that fragment gets swallowed as one bogus argument, and unrelated flags after it are silently lost. The bare path also ends up added toLIBSas a stray positional argument instead of being paired with-includeat all, matching the exact symptom described in #1730 (xtensa-esp-elf-gcc: fatal error: cannot specify '-o' with '-c' with multiple files).This flattens all fragments into a single token stream first, so a flag needing an argument is paired with just the next token regardless of which fragment it originally came from. It also re-quotes that argument if it contains a space before handing the joined string to
ParseFlags(), sinceParseFlags()re-splits string arguments withshlex.split()and would otherwise break an unquoted path (e.g. underProgram Fileson Windows) apart again.Fixes #1730
Verification
No unit tests cover this function, and reproducing the full
esp_lvgl_adapterbuild locally would need the whole ESP-IDF toolchain, so I verified this directly against the actualSCons.Environment.ParseFlags()implementation this file calls (viatool-sconsfrom a local PlatformIO install), reproducing the exact shape of the bug report:Before this fix:
-includeand its path are lost —CCFLAGSgets a bogus flag built from-includeplus the entire raw text of the next fragment, and depending on shape either drops-Wall/-O2or dumps the path intoLIBSas a positional argument (this second part is the literal cause of the "cannot specify '-o' with '-c' with multiple files" error in #1730, since PlatformIO then passes that stray path to the compiler as if it were another source file).After this fix:
CCFLAGScorrectly gets("-include", <File config.h>), and-Wall/-O2are preserved as separate flags — confirmed by calling the project's actualParseFlags()on the resulting strings, not just by reasoning about it.Also checked the space-in-path path specifically (
-isystem "C:/Program Files/foo"), confirming it survives as oneCCFLAGSentry rather than getting re-split byParseFlags()'s internalshlex.split().Generative AI
I used generative AI tools when creating this PR, but a human has checked the code and is responsible for the code and the description above.