Fix incorrect SDK provisioning commands in integration-tests instructions#34992
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34992Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34992" |
There was a problem hiding this comment.
Pull request overview
Updates the integration-test setup instructions to use the correct Arcade build script syntax for provisioning the repo-local SDK/workloads, avoiding ineffective --target=... arguments.
Changes:
- Replace the incorrect
./build.sh --target=dotnetand./build.sh --target=dotnet-local-workloadscommands with the supported./build.sh -restorecommand. - Clarify (in the comment) that this is the recommended Arcade-based provisioning path.
|
Should this go to main, then it gets merged to net10.0 and net11.0 branches? |
davidnguyen-tech
dismissed
rmarinho’s stale review
The base branch was changed.
|
/rebase |
…ions The instructions used `./build.sh --target=dotnet` which is invalid — unrecognized args are silently passed through to MSBuild as properties and do nothing useful. The Cake targets `dotnet` and `dotnet-local-workloads` exist but must be invoked via `dotnet cake --target=X`, not `./build.sh --target=X`. Replace with the recommended Arcade command `./build.sh -restore` which provisions the SDK and installs workloads via the `InitInternalTooling` target in eng/Tools.props. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The integration test skill (Run-IntegrationTests.ps1) uses `./build.sh -restore -pack` because integration tests need locally-packed MAUI NuGet packages installed into the SDK, not just the SDK with published workloads. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
davidnguyen-tech
force-pushed
the
fix/build-instructions
branch
from
a354991 to
fa372a5
Compare
|
@rmarinho Yes sounds reasonable, thank you :) |
|
/review -b feature/refactor-copilot-yml |
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 1 findings
See inline comments for details.
MauiBot
added
s/agent-review-incomplete
s/agent-reviewed
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Could you please check the ai's suggestions?
|
/review -b feature/refactor-copilot-yml |
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 2 findings
See inline comments for details.
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Could you review the 2 suggestions?
- Add 'dotnet tool restore' as Step 0 so 'dotnet cake' works on a fresh checkout (the Cake tool comes from .config/dotnet-tools.json). - Close the bash code fence after the workload command so the Verification section and its own fenced block render correctly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review -b feature/refactor-copilot-yml |
🤖 AI Summary
📊 Review Session —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34992 | Replace invalid ./build.sh --target=... instructions with repo-local tool restore plus dotnet cake --target=dotnet and dotnet cake --target=dotnet-local-workloads. |
⏳ PENDING (Gate skipped: no tests detected) | .github/instructions/integration-tests.instructions.md |
Original PR |
🔬 Code Review — Deep Analysis
Code Review — PR #34992
Independent Assessment
What this changes: Updates the manual integration-test prerequisite instructions to restore repo-local tools, then provision the local SDK and MAUI workloads with dotnet cake --target=dotnet and dotnet cake --target=dotnet-local-workloads instead of ./build.sh --target=....
Inferred motivation: The old commands mixed the Arcade shell wrapper with Cake target syntax, so a fresh local setup could fail or silently not provision what the instructions claim.
Reconciliation with PR Narrative
Author claims: The PR says the integration-test instructions used incorrect SDK provisioning commands and replaces them with dotnet cake; it also notes the change is documentation-only.
Agreement/disagreement: This matches the code. The added dotnet tool restore also addresses prior reviewer feedback and aligns with .github/skills/run-integration-tests/SKILL.md and Run-IntegrationTests.ps1, which restore local tools before invoking Cake.
Findings
No findings.
Devil's Advocate
I checked the full changed file, nearby run-integration-test skill docs/scripts, repo-local tool manifest, and recent file history. The main possible concern is that dotnet cake requires a global .NET SDK before the local .dotnet/ SDK exists, but the surrounding skill prerequisites already require a .NET SDK from global.json, and existing repo guidance uses this same sequence. CI is not fully complete: the GitHub check list shows Build Analysis still in progress, with no failing check runs reported.
Verdict: LGTM
Confidence: high
Summary: The documentation now matches the integration-test runner's actual auto-provisioning sequence and fixes the previously incorrect command form. No code or documentation correctness issues found; wait for CI completion before merge.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix-1 | Use pwsh ./build.ps1 --target=... wrapper commands after dotnet tool restore. |
✅ PASS | 1 file | Valid command shape, but not better than the PR because it adds wrapper indirection while still routing to Cake. |
| 2 | try-fix-2 | Use a higher-level ./dotnet-local.sh bootstrap flow. |
❌ FAIL | 0 files | Rejected before applying: dotnet-local.sh is not present in this checkout. |
| 3 | try-fix-3 | Prefer Run-IntegrationTests.ps1 -Category "Build" -AutoProvision as the primary setup path and retain Cake commands as fallback. |
✅ PASS | 1 file | Best alternative: aligns docs with the integration-test runner and reduces setup drift. |
| 4 | try-fix-4 | Document the lower-level .dotnet/dotnet build src/DotNet/DotNet.csproj workload-install commands instead of dotnet-local-workloads. |
✅ PASS | 1 file | Valid but too verbose and brittle for normal setup docs; useful only as troubleshooting detail. |
| PR | PR #34992 | Restore repo-local tools, then run dotnet cake --target=dotnet and dotnet cake --target=dotnet-local-workloads. |
1 file | Original PR. Code review found no issues; no tests were detected by the gate. |
Cross-Pollination
| Model/Reviewer | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Candidate 1: repo PowerShell build wrapper. |
| maui-expert-reviewer | 2 | Yes | Candidate 2: repo-local bootstrap script; failed because script is absent. |
| maui-expert-reviewer | 3 | Yes | Candidate 3: integration-test runner -AutoProvision; passed and is the strongest alternative. |
| maui-expert-reviewer | 4 | Yes | Candidate 4: lower-level workload install commands; passed but is less maintainable. |
Exhausted: Yes
Selected Fix: Candidate #3 — It passed docs-only validation and is the only alternative that is both repository-valid and arguably better than the PR for maintainability, because it points users to the same integration-test runner workflow they are already instructed to use and keeps the direct Cake commands as fallback. The PR's current fix remains correct and simpler.
📋 Report — Final Recommendation
Comparative Report — PR #34992
Candidates Compared
| Rank | Candidate | Test Result | Outcome | Rationale |
|---|---|---|---|---|
| 1 | pr-plus-reviewer |
Gate skipped; expert review found no issues | Winner | Same fix as the PR after expert review. It is the shortest correct documentation update and matches the integration-test runner's actual auto-provisioning sequence without adding extra workflow indirection. |
| 2 | pr |
Gate skipped; prior review found no issues | Equivalent to winner | The raw PR fix is correct. It ranks just below pr-plus-reviewer only because the required expert-review pass explicitly revalidated it and found no further changes. |
| 3 | try-fix-3 |
PASS | Strong alternative | Documents Run-IntegrationTests.ps1 -Category "Build" -AutoProvision as the primary setup path and retains Cake commands as fallback. This is valid and maintainable, but it expands a manual-prerequisites section and is not necessary to fix the incorrect commands. |
| 4 | try-fix-1 |
PASS | Valid but less direct | Uses pwsh ./build.ps1 --target=.... This remains repository-valid but adds wrapper indirection while still relying on restored repo tools and Cake. |
| 5 | try-fix-4 |
PASS | Valid but too low-level | Documents internal .dotnet/dotnet build src/DotNet/DotNet.csproj workload-install mechanics. It is more brittle and more likely to drift than the Cake target that owns this behavior. |
| 6 | try-fix-2 |
FAIL | Rejected | Documents dotnet-local.sh, which is not present in this checkout. Because it failed validation, it ranks below all candidates that passed or were review-clean. |
Analysis
The submitted PR fix addresses the root documentation bug: ./build.sh --target=... is not the correct first-time SDK/workload provisioning command shape for these instructions. Adding dotnet tool restore is also necessary because direct dotnet cake depends on repo-local tools from .config/dotnet-tools.json.
try-fix-3 is the strongest alternative because Run-IntegrationTests.ps1 -AutoProvision directly wraps the same setup sequence and is already the preferred integration-test runner. However, the changed section is specifically a manual prerequisite fallback for missing local SDK/workloads; the PR's direct Cake commands are concise, accurate, and aligned with the run-integration-tests skill documentation and script error message.
No expert-review feedback required code or documentation changes, so pr-plus-reviewer is identical to pr. It wins because it preserves the PR's minimal, correct fix after independent expert validation.
Winner
pr-plus-reviewer — the PR fix plus expert reviewer feedback applied. Since the reviewer found no actionable issues, this is the submitted PR fix unchanged.
Description
The
.github/instructions/integration-tests.instructions.mdfile contained incorrect build commands:These mix up Arcade build script syntax (
./build.sh) with Cake target syntax (--target=X). Unrecognized arguments are silently passed through to MSBuild as properties viaeng/common/build.sh(line 200-201) and do nothing useful.Fix
Replace with
dotnet cakeImpact
Documentation-only change. No functional code changes.
AI now uses the right commands.