Centralize eng/common build defaults in tools.ps1/tools.sh#17025
Draft
Copilot wants to merge 3 commits into
Draft
Centralize eng/common build defaults in tools.ps1/tools.sh#17025Copilot wants to merge 3 commits into
Copilot wants to merge 3 commits into
Conversation
Copilot
AI
changed the title
[WIP] Fix defaults for bool switch conditions in tools.ps1
Fix eng/common/tools.ps1 switch-parameter defaults
Jun 18, 2026
Make tools.ps1/tools.sh the single source of truth for build defaults (binaryLog, nodeReuse, configuration, verbosity, warnAsError, etc.) and remove the duplicated/masking defaults from build, msbuild, sdk-task, darc-init and dotnet-install. PowerShell importers opt in via $importerBoundParameters so tools.ps1 unbinds declared-but-unpassed parameters while preserving explicitly passed and assignment-set values. Also: default source-build configuration to Release in tools.sh; fold sdk-task restore into a single MSBuild /restore invocation with a -noRestore/--no-restore switch; add prepareMachine to sdk-task.sh; and fix sdk-task.sh's --noWarnAsError case label. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ViktorHofer
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Arcade’s shared eng/common tooling to ensure CI/environment-aware defaults (notably binaryLog and nodeReuse) are applied correctly when importing scripts declare [switch]/[bool] parameters, and it aligns several entrypoints/templates to rely on those centralized defaults.
Changes:
- Add an opt-in mechanism in
eng/common/tools.ps1to “unbind” declared-but-unpassed parameters so tools-owned defaults apply. - Centralize source-build configuration defaulting and CI node-reuse behavior in
eng/common/tools.sh/tools.ps1, removing redundant overrides from wrappers. - Update
sdk-taskscripts and pipeline templates/docs to reflect new default restore behavior (with-noRestore/--no-restore).
Show a summary per file
| File | Description |
|---|---|
| eng/publishing/v3/publish.yml | Drops explicit -restore for sdk-task.ps1 invocation (restore now defaults on). |
| eng/common/tools.sh | Moves source-build configuration defaulting and CI node-reuse gating into shared defaults. |
| eng/common/tools.ps1 | Adds opt-in parameter “unbinding” so tools-owned defaults aren’t masked by declared params; updates node-reuse default to honor MSBUILD_NODEREUSE_ENABLED. |
| eng/common/sdk-task.sh | Switches restore control to --no-restore and simplifies execution to a single MSBuild call with optional /restore. |
| eng/common/sdk-task.ps1 | Switches restore control to -noRestore and simplifies execution to a single MSBuild call with optional /restore; opts in to tools-managed defaults. |
| eng/common/msbuild.sh | Relies on tools.sh for defaults; removes redundant CI node-reuse override. |
| eng/common/msbuild.ps1 | Opts in to tools-managed defaults; removes redundant CI node-reuse override. |
| eng/common/dotnet-install.ps1 | Opts in to tools-managed defaults for verbosity/runtime feed parameters. |
| eng/common/darc-init.sh | Relies on tools.sh for verbosity default. |
| eng/common/darc-init.ps1 | Opts in to tools-managed defaults for verbosity. |
| eng/common/core-templates/post-build/post-build.yml | Drops explicit -restore for sdk-task.ps1 invocation. |
| eng/common/core-templates/job/publish-build-assets.yml | Drops explicit -restore for sdk-task.ps1 invocation. |
| eng/common/build.sh | Removes duplicated CI/config defaulting logic in favor of tools.sh. |
| eng/common/build.ps1 | Opts in to tools-managed defaults and removes duplicated CI overrides for binlog/node reuse. |
| Documentation/DependencyFlowOnboardingWithoutArcade.md | Updates example to remove explicit -restore/-msbuildEngine lines. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 15/15 changed files
- Comments generated: 4
Comment on lines
+15
to
+22
| if (Test-Path variable:importerBoundParameters) { | ||
| $importerDeclaredParameters = (Get-PSCallStack)[1].InvocationInfo.MyCommand.Parameters.Keys | ||
| foreach ($toolsManagedDefault in @('binaryLog', 'nodeReuse', 'warnAsError', 'warnNotAsError', 'verbosity', 'configuration', 'msbuildEngine', 'runtimeSourceFeed', 'runtimeSourceFeedKey')) { | ||
| if (($importerDeclaredParameters -contains $toolsManagedDefault) -and -not $importerBoundParameters.ContainsKey($toolsManagedDefault)) { | ||
| Remove-Variable -Name $toolsManagedDefault -ErrorAction SilentlyContinue | ||
| } | ||
| } | ||
| } |
Comment on lines
2
to
10
| Param( | ||
| [string] $configuration = 'Debug', | ||
| [string] $configuration, | ||
| [string] $task, | ||
| [string] $verbosity = 'minimal', | ||
| [string] $msbuildEngine = $null, | ||
| [switch] $restore, | ||
| [string] $verbosity, | ||
| [string] $msbuildEngine, | ||
| [switch] $noRestore, | ||
| [switch] $prepareMachine, | ||
| [switch][Alias('nobl')]$excludeCIBinaryLog, | ||
| [switch]$noWarnAsError, |
Comment on lines
37
to
46
| case $lowerI in | ||
| --task) | ||
| task=$2 | ||
| shift 2 | ||
| ;; | ||
| --restore) | ||
| restore=true | ||
| --no-restore) | ||
| restore=false | ||
| shift 1 | ||
| ;; | ||
| --verbosity) |
Comment on lines
82
to
84
| arguments: -task PublishBuildAssets | ||
| -restore | ||
| -msbuildEngine dotnet | ||
| /p:ManifestsPath='$(Build.StagingDirectory)/Download/AssetManifests' | ||
| /p:MaestroApiEndpoint=https://maestro.dot.net" |
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.
Summary
Makes
eng/common/tools.ps1andeng/common/tools.shthe single source of truth for build defaults (binaryLog,nodeReuse,configuration,verbosity,warnAsError,warnNotAsError,msbuildEngine,runtimeSourceFeed/Key,prepareMachine,restore). Previously these defaults were duplicated in the importing scripts (build,msbuild,sdk-task,darc-init,dotnet-install), and on the PowerShell side a declared[switch]/[bool]/[string]parameter is always defined, so it masked the CI/environment-aware defaults intools.ps1— e.g. a switch-based entry point invoked with-cigotbinaryLog=$falseinstead of$true, andnodeReusestayed$trueon CI.Approach
PowerShell: opt-in, declared-parameter-aware unbinding
An importer opts in by assigning its bound parameters before dot-sourcing:
tools.ps1then drops only the managed variables that the importer declares as a parameter but did not pass, so the CI/environment-aware defaults take effect. Values passed explicitly, or set by assignment (e.g.sdk-task.ps1computing$binaryLog/$warnAsError), are preserved because only declared-but-unpassed parameters are unbound. The block is skipped entirely for scripts that do not opt in, so external/out-of-repo consumers keep their current behavior.Opted in:
build.ps1,msbuild.ps1,sdk-task.ps1,dotnet-install.ps1,darc-init.ps1.Bash
No masking exists (
${var:-default}), so the duplicated default initializers were simply removed frombuild.sh,msbuild.sh,sdk-task.sh,darc-init.shand left totools.sh.Notable changes
tools.ps1/tools.sh— own all the defaults; folded theMSBUILD_NODEREUSE_ENABLEDCI override into thenodeReuse/node_reusedefault;tools.shnow defaultsconfigurationtoReleasewhensource_buildis true (only when not explicitly provided), otherwiseDebug.build.ps1/build.sh/msbuild.ps1/msbuild.sh— removed the duplicated defaults and the redundant post-import CInodeReuseoverride blocks. Net behavior:nodeReusenow followstools(no node reuse on CI unlessMSBUILD_NODEREUSE_ENABLED=1), while an explicitly passed-nodeReuse/--nodereuseis respected.sdk-task.ps1/sdk-task.sh—restorenow defaults to true and is folded into a singleMSBuild /restoreinvocation (the separateRestorepass is gone); replaced the-restoreflag with-noRestore/--no-restore; addedprepareMachine/--preparemachineto the bash script for parity; inlined the now single-useBuildfunction (Executeis the only target); fixed the bash--noWarnAsErrorcase label (was never matching the lowercased input).publish-build-assets.yml,post-build.yml,publishing/v3/publish.yml, onboarding doc) — dropped the now-redundant-restoreargument fromsdk-task.ps1invocations.Validation
All edited PowerShell files parse via the PS parser; all edited shell files pass
bash -n. The opt-in unbinding mechanism was verified on both PowerShell 5.1 and Core: explicitly passed values and assignment-set values are preserved, declared-but-unpassed managed parameters fall back to thetoolsdefaults.To double check: