Fix quoting in eng/build.sh#14506
Conversation
I use a particular set of flags to build .NET VMR, which flows something
like this to the msbuild build scripts:
$ ./build.sh /p:OfficialBuilder="Red Hat Inc."
This regressed recently (probably
dotnet#14361). This change fixes the
issue.
For arrays which can be empty, use the ${var[@]+"${var[@]}"} syntax,
as discussed in dotnet/dotnet#797, to avoid
issues on macOS.
Assisted-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a regression in eng/build.sh argument handling so property values containing spaces (for example /p:OfficialBuilder="Red Hat Inc.") round-trip correctly into the underlying Arcade build invocation, and updates stage2 argument forwarding to use proper arrays.
Changes:
- Convert
eng/build.shfrom string-concatenated arguments to Bash arrays for correct quoting/escaping. - Change stage2 argument collection to support multiple stage2 args (now accumulated) and forward them safely.
- Update CI pipeline invocations to use the new stage2-argument passing style.
Show a summary per file
| File | Description |
|---|---|
| eng/build.sh | Uses Bash arrays for build args to preserve quoting; updates stage2 arg handling. |
| eng/build.ps1 | Changes stage2Arguments to a string array and forwards it directly to stage2. |
| azure-pipelines/quarantine.yml | Updates build invocation to use -stage2Argument for stage2-only args. |
| .vsts-dotnet-ci.yml | Updates stage2 invocation pattern (including splitting multi-arg stage2 use into multiple arguments). |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 2
| --stage2argument) | ||
| stage2Arguments+=( "$2" ) | ||
| # Supplying stage2Argument implies a multi-stage build. | ||
| stage2=true | ||
| shift 2 | ||
| ;; |
| [switch] $stage2, | ||
| [string] $stage2Arguments = "", | ||
| [string[]] $stage2Arguments = @(), | ||
| [Parameter(ValueFromRemainingArguments=$true)][String[]]$properties |
There was a problem hiding this comment.
@omajid we don't need to preserve the old plural name. Sorry my comment was misleading.
There was a problem hiding this comment.
It seems odd to me to have a variable named as a singular but is actually a plural (array). Is [string[]] $stage2Argument an accptable name?
There was a problem hiding this comment.
Do whatever makes most sense to you. I only mentioned the singular form as you started using it in commit 2 already in the YML. My only request is to keep the name aligned between the pwsh and the shell code :)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
I use a particular set of flags to build .NET VMR, which flows something like this to the msbuild build scripts:
This regressed recently (probably
#14361). This change fixes the issue.
For arrays which can be empty, use the ${var[@]+"${var[@]}"} syntax, as discussed in dotnet/dotnet#797, to avoid issues on macOS.
Assisted-By: Claude Opus 4.6 noreply@anthropic.com