[perf][wasm] fix wasm-perf lane, stage complete CoreCLR toolchain cohort - #5
Conversation
Source PR: dotnet#133203 Source head: bd1868a
✅ Shipwright · ApproveRecommendation: approve PR #5 · Tier
Findings (7)
Fireworks usage: 15,342 input · 856 output · 16,198 total tokens · $0.0039 · 13s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
|
|
||
| set -euo pipefail | ||
|
|
||
| source_dir="$1" |
There was a problem hiding this comment.
Shipwright · HIGH
The new stage-nuget-cohort.sh script has no usage/help text and relies on positional arguments with no validation of argument count.
Impact: The new stage-nuget-cohort.sh script has no usage/help text and relies on positional arguments with no validation of argument count. A new hire invoking it with the wrong number of arguments will get a confusing 'unbound variable' error from set -u rather than a clear message.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| mkdir -p $(Build.SourcesDirectory)/artifacts/packages/${{ parameters.configForBuild }}/Shipping && | ||
| cp $(Build.SourcesDirectory)/artifacts/coreclr-runtimepack/*.nupkg $(Build.SourcesDirectory)/artifacts/packages/${{ parameters.configForBuild }}/Shipping/ | ||
| displayName: "Stage CoreCLR runtime pack into local feed" | ||
| artifactName: BrowserWasmCoreCLRWorkloadPackages_$(_hostedOs) |
There was a problem hiding this comment.
Shipwright · HIGH
The inline script in perf-wasm-prepare-artifacts-steps.yml duplicates the logic for finding the WebAssembly SDK package and extracting its version, which is also partially duplicat
Impact: The inline script in perf-wasm-prepare-artifacts-steps.yml duplicates the logic for finding the WebAssembly SDK package and extracting its version, which is also partially duplicated in stage-nuget-cohort.sh. This makes the cohort versioning logic harder to maintain and understand across three locations.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| echo "Expected exactly one non-symbol $package_pattern in $source_dir; found $package_count." >&2 | ||
| find "$source_dir" -maxdepth 1 -type f \ | ||
| -name "$package_pattern" -not -name '*.symbols.nupkg' -print >&2 | ||
| exit 1 |
There was a problem hiding this comment.
Shipwright · HIGH
stage-nuget-cohort.sh derives cohort_version from the first package when expected_version is empty, but the version extraction uses package_prefix from the glob pattern.
Impact: stage-nuget-cohort.sh derives cohort_version from the first package when expected_version is empty, but the version extraction uses package_prefix from the glob pattern. If the pattern contains multiple wildcards or the basename has a suffix other than .nupkg, the derived version can be wrong or empty, causing the subsequent cohort match to fail or silently accept mismatched packages.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| -name "$package_pattern" -not -name '*.symbols.nupkg' -print | | ||
| wc -l | tr -d '[:space:]' | ||
| )" | ||
|
|
There was a problem hiding this comment.
Shipwright · HIGH
The script uses find ...
Impact: The script uses find ... -print -quit to select a package after counting. If a file is added or removed between the count and selection (TOCTOU), package can be empty or point to a different file, and the subsequent cp will fail or copy the wrong artifact. Pipeline artifacts are generally stable, but the script does not guard against this.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| shift 3 | ||
|
|
||
| mkdir -p "$destination_dir" | ||
|
|
There was a problem hiding this comment.
Shipwright · HIGH
The script uses unquoted variable expansions in find -name "$package_pattern" and cp "$package" "$destination_dir/".
Impact: The script uses unquoted variable expansions in find -name "$package_pattern" and cp "$package" "$destination_dir/". If a package pattern or path contains spaces or shell metacharacters, the command can break or execute unintended commands. Pipeline inputs are controlled, but the script is not robust against malicious or accidental special characters.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| @@ -0,0 +1,43 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| set -euo pipefail | |||
There was a problem hiding this comment.
Shipwright · HIGH
The inline script in perf-wasm-prepare-artifacts-steps.yml uses bash with set -euo pipefail but does not set -o pipefail in the stage-nuget-cohort.sh script itself.
Impact: The inline script in perf-wasm-prepare-artifacts-steps.yml uses bash with set -euo pipefail but does not set -o pipefail in the stage-nuget-cohort.sh script itself. If a command in a pipeline fails, the script may continue and copy an incorrect or missing package.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| Assert.Contains("Stopping after validating the R2R directory", output); | ||
| Match match = s_r2rDirectoryRegex.Match(output); | ||
| Assert.True(match.Success, output); |
There was a problem hiding this comment.
Shipwright · MEDIUM
The new test asserts the R2R directory casing matches the SDK output, but the expected path uses Path.Combine(GetObjDir(config), "R2R") + Path.DirectorySeparatorChar.
Impact: The new test asserts the R2R directory casing matches the SDK output, but the expected path uses Path.Combine(GetObjDir(config), "R2R") + Path.DirectorySeparatorChar. On case-sensitive filesystems this will fail if the actual output casing differs, and on Windows the assertion is case-insensitive so it cannot detect the casing regression the test is intended to catch. The test does not reliably verify the intended b…
Suggested fix: Fix the review finding before release.
1 participant
Summary
Stage the complete same-build CoreCLR browser-WASM package cohort consumed by the performance pipeline. This fixes two failures:
Microsoft.NETCore.App.Crossgen2.linux-x64version11.0.0-ciwas absent from the local feed.Microsoft.NET.ILLink.Taskspackage. In build 1581666, restore requested ILLink product version.109, the artifact did not contain it, NuGet floated to.118, and_RunILLinkthen failed because the payload only contained the.109runtime.The existing CoreCLR-only Crossgen2 staging switch becomes one toolchain/cohort staging switch that:
ILLink.Tasks.csprojwithout rebuilding or restoring, because the normalclr+libs+host+packsbuild compiles the project but does not pack it;This completes the runtime artifact consumed by dotnet/performance#5297 and repairs the prerequisite artifact build used by the existing Mono WASM performance lane.
Complete staged cohort
The CoreCLR performance artifact now carries the complete same-build publish inputs:
staging/dotnet-nonestaging/microsoft.netcore.app.runtime.browser-wasmMicrosoft.NETCore.App.RefMicrosoft.NET.Sdk.WebAssembly.PackMicrosoft.NETCore.App.Crossgen2.linux-x64Microsoft.NET.ILLink.TasksThe dependent Mono artifact job also receives the same-build CoreCLR browser runtime and Crossgen2 packages before installing its workload. WebAssembly SDK and ILLink are direct SDK tool restore dependencies, while Crossgen2 and the runtime/ref packs are selected through the SDK framework/tool-pack graph. ILLink's private dependencies are included in its tooling package, so no additional locally built SDK tool package is required. The performance-side override aligns
KnownWebAssemblySdkPack,KnownCrossgen2Pack, andKnownILLinkPackto the staged local package version.Validation
Microsoft.NETCore.App.Crossgen2.linux-x64version11.0.0-ciwas missing from the local package feed; this change publishes and downloads that exact package from the CoreCLR build job..118ILLink/runtime mismatch, the package graph above, and that the regular runtime build compiles but does not packILLink.Tasks.csproj.git diff --checkand an independent focused review of the final pipeline diff.Remaining end-to-end validation
Queue
runtime-wasm-perfpipeline 156 on Ubuntu 22.04 with repository resource overrides:{ "resources": { "repositories": { "self": { "refName": "refs/heads/lewing-stage-wasm-r2r-tools" }, "performance": { "refName": "refs/heads/lewing-wasm-coreclr-r2r-perf" } } } }The Mono artifact job must install the workload from the local feed without a missing Crossgen2 package. The CoreCLR R2R lane must restore without NU1603/fallback, complete ILLink, emit non-empty per-app R2R images through Crossgen2, stage WebCIL, and start the benchmark under V8. Existing failed builds cannot provide that proof because their published artifacts predate the complete cohort staged by this change.
Note
This pull request description was generated with the assistance of GitHub Copilot.
Source merge-base:
7e807becb8c6b1917dac0565c674ade7947a8becSource head:
bd1868ac31217c90a52eb67003b8fe886e237e3a