Replace eval with arrays in native build scripts

[ApparentlyPlus]

Contributes to #73327.

build_native assembled the gen-buildsys.sh invocation as a string and ran it through eval, so the command was parsed twice and any path containing a space was split on the second pass. This replaces the string with a bash array passed straight to the script, so quoting is preserved in one parse.

Verified locally with a full ./build.sh -c Release on linux-x64 and macos-arm64. At a path with a space, the build now gets past CMake configure and dies later, at the unquoted linker --version-script, which is the next set of fixes.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[ApparentlyPlus]

@teo-tsirpanis This should be area-Infrastructure again, the bot mislabeled it.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

[ApparentlyPlus]

Test are green besides the known failures

[akoeplinger]

Holistic Review

Motivation: Preserving CMake argument boundaries addresses a real problem with values containing spaces.

Approach: Converting argument handling to arrays is appropriate, but upstream callers still combine multiple CMake options into a single string. Those callers need to migrate alongside the downstream handling.

Summary: ⚠️ Needs Changes. The change introduces a compatibility regression for existing multi-option --cmakeargs usage.

Detailed Findings

❌ Argument forwarding — Multiple CMake options become one setting

Merge-blocking: eng/native/build-commons.sh:390–393

Each -cmakeargs payload is now preserved as one argument, but eng/build.sh:476 still concatenates options—including repeated --cmakeargs arguments—into one string. The runtime, host, and native-library MSBuild projects forward that string as one quoted payload.

Consequently, subsequent -D options become part of the first setting’s value. Isolated probes using the PR’s parser and generator, followed by real CMake execution, confirmed:

Combined argument: FIRST=[1 -DSECOND=2] SECOND=[]
Separate arguments: FIRST=[1] SECOND=[2]

This breaks existing usage, including the two-option ARM cross-build example in docs/workflow/building/coreclr/cross-building.md:117. Repeating --cmakeargs at the top level does not avoid the problem.

Requested change: Update upstream argument producers so each CMake option reaches the array separately while preserving spaces within values. Cover repeated top-level options, existing multi-option payloads, and values containing spaces; blindly splitting on whitespace would reintroduce the original problem.

Reviewed head ff32d25. Verification included source tracing and isolated shell/CMake probes; no repository builds or test suites were run.

Note

This review was generated by GitHub Copilot.

[ApparentlyPlus]

I'm a bit busy today, I'll check this one out first thing tomorrow

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[ApparentlyPlus]

Good catch, I reproduced it. -cmakeargs "-DA=1 -DB=2" gave A=1 -DB=2 with B unset. Repeated --cmakeargs too, since eng/build.sh:476 merges them first.

$(CMakeArgs) only carries user input, so I split it into items in the three consumers that feed build_native:

<_CMakeArgs Include="$([MSBuild]::Unescape($(CMakeArgs.Trim().Replace(' ', ';'))))" />

runtime.proj, corehost.proj, build-native.proj. mono.proj is unaffected, it calls cmake through a shell string. This also fixes eng/Subsets.props, which appends -DCLR_CROSS_COMPONENTS_BUILD=1 and broke with no user input at all.

./build.sh -c Release with --cmakeargs "-DPROBE_A=1" --cmakeargs "-DPROBE_B=2 -DPROBE_C=3" builds on linux-x64 and macos-arm64, at a normal path and at one with a space (provided the next set of fixes for the latter). All three probes land separately in CMakeCache.txt, with the spaced paths intact in the same cache.

Important

The split happens above the scripts, so build.sh --cmakeargs "-DA=1 -DB=2" still works. Calling build-runtime.sh/build-native.sh directly doesn't go through MSBuild, so their -cmakeargs now takes one option literally and multiples need the flag repeated, as runtime.proj and the docs do. A payload can't be both literal for spaced paths and word-split for multiple options.

[ApparentlyPlus]

Tests are green and wider this time because I touched the native build harness. Worth another look from someone or copilot regardless, because it's very tricky to debug everything by hand here.

[ApparentlyPlus]

@akoeplinger Just a nudge; no need to rush on this if you've got other priorities!

[Copilot]

Copilot review overview

🟡 Changes recommended

The MSBuild whitespace splitting corrupts individual CMake values containing spaces.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3

Open (3)

• Splitting CMake arguments on spaces breaks quoted values · New
• Space splitting breaks CMake arguments with embedded spaces · New
• Whitespace replacement corrupts CMake definitions with spaces · New

[akoeplinger]

@ApparentlyPlus I asked copilot to come up with a potential fix, but I'm not sure I like everything in it. Feel free to use if you want: akoeplinger@03f8c9e

[ApparentlyPlus]

Don't worry, I'll take a look and see what's best here. Thanks!

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[ApparentlyPlus]

Okay, Copilot's solution is actually much better than what I came up with, so I tested it and cherry picked it.

Mine split $(CMakeArgs) on every space in MSBuild, so --cmakeargs '-DFOO=/a b/c' got torn in half, and Copilot rightfully flagged it. Forwarding after -- and escaping the fragment through MSBuild keeps the user's own quoting intact, so the quotes decide the boundaries cleanly, instead of me guessing. It also handles both multiple options and values with spaces, which mine couldn't do at once.

I verified it on linux-x64 and macos-arm64 with --cmakeargs "-DPROBE_A=1" --cmakeargs "-DPROBE_B=2 -DPROBE_C=3" --cmakeargs '-DPROBE_S="/a b/c"', normal + space paths. Eveyrhting lands correctly in CMakeCache.txt in every combination, and dropping the -DCLR_DOTNET_HOST_PATH single quotes is fine since CMake strips them anyway.

Copilot also added test_cmake_args.py, but I dropped that. It was a helper test script, and it caught some failures when I broke an escaping line to check that it bites, but nothing in CI runs it and it's a new directory, which would pollute the codebase unnecessarily. I kept the readme changes tho.

[ApparentlyPlus]

@akoeplinger Interesting finding while I was debugging the Copilot suggestion - this exact eval pattern isn't confined to the native build scripts (yay me!).

TL;DR: the same eval problem exists in other build paths (test, managed), and the same "flat string instead of array" shape shows up in cross building. I propose we don't fold either in here, but rather in a follow up.

Specifically:

1. There's still two* eval $nextCommand left in src/tests/build.sh, at 123 and 466. They're the managed test build and the test run, not the native build, so they're outside what the title claims.

2. Two more in the same family, neither an eval but the same shape: src/tests/build.sh:382 is an unquoted source, the same bug as eng/build.sh:169 that started build.sh fails when run in a directory that contains spaces #73327, and gen-buildsys.sh:75,80 builds cmake_extra_defines as a flat string with $scriptroot in it, which only bites on cross builds and I'm not sure I can test locally.

Note

*One of them isn't a straight eval removal though. __CommonMSBuildArgs packs seven MSBuild args into a single string that gets added as one array element, and __msbuildLog and friends carry escaped \" quotes. Both only work because eval re-parses them. Pull the eval without converting those first and you get exactly the regression Copilot caught here. Just gotta be careful.

None of it regresses this PR, it's just that the scope got slightly bigger again. Truly a gift that keeps on giving :')

I'd say merge this one once it's green, then a follow up for the test and cross build ones, then the quoting sweep I described in the issue. That would cover native, managed and test builds on unix. I'll refine the backlog in the issue thread too, better to break it down than battle the abyss here. We're close though!