[Apple mobile] Stage the Helix R2R app payload outside the crossgen2 input directory - #132489
[Apple mobile] Stage the Helix R2R app payload outside the crossgen2 input directory#132489davidnguyen-tech wants to merge 15 commits into
Conversation
The Helix ReadyToRun step copied the crossgen2 output over the publish directory, but that directory is also the crossgen2 input. When the AOT build runs twice in the same work item - a Helix work item retry re-runs the whole command in the same directory - the second run compiles the already compiled assemblies again, and --strip-il-bodies drops the IL of an image that no longer has the original method bodies. The app then dies at startup with "A method body required at runtime was stripped from the ReadyToRun image". Stage the app payload in obj/r2r-publish instead: copy the pristine publish tree there, overlay the crossgen2 output on top of it, and point AppleBuildDir and the Apple*ToBundle items at the staging directory. AppleAppBuilder bundles every top level entry of its AppDir, so redirecting the item lists alone is not enough, the AppDir itself has to move out of the pristine input directory. Also fail with a clear error if the publish directory already contains R2R output, and remove obj/R2R before compiling so that a partially written attempt cannot look up to date to the next one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR addresses a Helix retry hazard in Apple mobile CoreCLR test app ReadyToRun (R2R) compilation by preventing crossgen2 outputs from being written back into the publish/ directory (which is also the crossgen2 input). Instead, it stages a pristine app payload into an intermediate directory and overlays R2R outputs there, ensuring retries recompile from original IL inputs.
Changes:
- Introduces an
obj/r2r-publish/staging directory: copies the originalpublish/payload into it, then overlays crossgen2 outputs, and builds the app bundle from the staged payload. - Ensures retry safety by recreating the staging directory on every run and deleting
obj/R2Rbefore recompilation. - Adds guardrails: warnings when R2R outputs are unexpectedly empty and a fail-fast error if stale
*.r2r.dyliboutputs are detected inpublish/.
A payload can legitimately arrive with ReadyToRun output in it: a test project that sets PublishReadyToRun itself keeps that setting on the build machine, where AppleBuild.props only suppresses the default, so the composite image is published and then shipped inside the work item. Failing the build on the mere presence of a *.r2r.dylib would break the first attempt of such a work item, and the guard is not what keeps the crossgen2 inputs pristine, the staging directory is. Keep the diagnostic, drop the hard failure, and say what the consequence is. Also make the comment on the obj/R2R cleanup match what the SDK actually tracks: the composite image is an output of _CreateR2RImages, the per assembly files written next to it are not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Measuring the actual payload showed the check has no signal. The build machine does not strip IL when it publishes ReadyToRun for these projects, because the in tree crossgen2 targets never pass the composite strip argument: an agent published payload has 145956 method bodies and zero stripped ones, and only the Helix compilation produces stripped bodies. So the only thing that can still put a .r2r.dylib in the publish directory is a build machine that pre compiled the app, where compiling again is fine, and the message claimed the opposite. The staging directory is what keeps the crossgen2 inputs pristine. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Apple Helix ReadyToRun proxy build had no test surface, so the retry bug it just got fixed for could come back unnoticed. Cover it from Wasm.Build.Tests, the only in-tree suite that already runs MSBuild driven build tests on a CI build machine. The test ships the real ProxyProjectForAOTOnHelix.proj into the test payload and drives it with `dotnet msbuild`, so the production target logic is what is under test rather than a copy of it. It lays out a Helix shaped work item, stands in for crossgen2 with a stub props file, and runs the R2R targets twice in the same directory with leftovers from a killed attempt planted in between. Exact manifests assert that the publish directory stays byte for byte identical across both runs, that the staging directory is rebuilt from scratch, and that AppleBuildDir and the Apple*ToBundle item lists resolve inside the staging directory. Wire src/mono/msbuild/apple/* into the wasmbuildtests path set so a change to the proxy project actually runs the test on PR CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/mono/wasm/Wasm.Build.Tests/AppleHelixR2RTests.cs:356
- ReplaceDirectoryContent uses TryDeleteDirectory, which swallows IOException/UnauthorizedAccessException. Since this method is intended to ensure the crossgen output directory is clean between attempts, suppressing delete failures can leave stale files behind and make the test nondeterministic (or validate the wrong behavior). It’s better to delete the directory deterministically (or fail) and then recreate/populate it.
private static void ReplaceDirectoryContent(string directory, (string RelativePath, string Content)[] files)
{
TryDeleteDirectory(directory);
foreach ((string relativePath, string content) in files)
WriteFile(Path.Combine(directory, ToNativePath(relativePath)), content);
The run script builds SDK_FOR_WORKLOAD_TESTING_PATH from $(dirname $0), so it can be a relative path, and the child msbuild runs with a different working directory. Resolve the host to a full path the way BuildEnvironment already does, and point DOTNET_ROOT, DOTNET_INSTALL_DIR and PATH at it for the same reason that file gives: the repo build environment sets them to its own dotnet. Narrow the path trigger to src/mono/msbuild/apple/data/*. The test only ever loads the proxy project from data/, and the AppleBuild props and targets next to it are shielded by the stub Directory.Build files, so triggering the whole browser wasm test matrix on a change to those would buy no coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous commit prepended the resolved host directory to PATH by interpolating the parent value, which yields a trailing separator when the parent has no PATH at all. An empty PATH entry means the working directory on unix, and the child runs with the work item publish directory as its working directory, so keep the separator out in that case. Also say in the path trigger comment which files under apple/data the test actually covers: it replaces the Directory.Build files with stubs, so a change to those runs the test without exercising them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/mono/wasm/Wasm.Build.Tests/AppleHelixR2RTests.cs:378
- ReplaceDirectoryContent deletes the directory via TryDeleteDirectory, which silently swallows IOException/UnauthorizedAccessException. In this method that behavior can hide real test setup failures and leave stale files behind (e.g., if deletion fails on Windows due to file locks), potentially making the test flaky or misleading. Prefer a deterministic delete that fails the test when the directory cannot be cleared; keep TryDeleteDirectory only for best-effort cleanup in the finally path.
private static void ReplaceDirectoryContent(string directory, (string RelativePath, string Content)[] files)
{
TryDeleteDirectory(directory);
foreach ((string relativePath, string content) in files)
WriteFile(Path.Combine(directory, ToNativePath(relativePath)), content);
|
/azp run runtime-extra-platforms |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 780282fa-426e-4ebe-96ab-14cd314cce54
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 780282fa-426e-4ebe-96ab-14cd314cce54
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/mono/msbuild/apple/tests/Apple.Build.Tests/AppleHelixR2RTests.cs:180
- RunAttempt has an unused parameter
publishDir. This makes the helper signature misleading and may also trip analyzers if unused-parameter diagnostics are enabled. Consider removing the parameter and updating the two call sites accordingly.
private void RunAttempt(
string workItemRoot,
string publishDir,
string crossgenOutputDir,
string r2rIntermediateDir,
string appBundleSourceDir,
(string RelativePath, string Content)[] r2rOutput)
{
src/mono/msbuild/apple/data/ProxyProjectForAOTOnHelix.proj:188
- These steps assume
$(_R2RAppBundleSourceDir)is set. Currently the target’s Condition is onlyPublishReadyToRunContainerFormat == macho, so invoking the target without the full R2R/CoreCLR predicates would leave_R2RAppBundleSourceDirempty and make RemoveDir/Copy behavior ambiguous. Add an explicit guard so the target fails fast if_R2RAppBundleSourceDirisn’t set.
<!-- Recreated on every run so that a Helix retry in the same work item cannot inherit stale dylibs
or app builder output from a previous attempt. -->
<RemoveDir Directories="$(_R2RAppBundleSourceDir)" />
<MakeDir Directories="$(_R2RAppBundleSourceDir)" />
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 780282fa-426e-4ebe-96ab-14cd314cce54
|
/azp run runtime-extra-platforms |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @akoeplinger, @matouskozak, @simonrozsival |
| standard artifacts/bin/<project>/<config>/<tfm> layout. src/mono/wasm/Wasm.Build.Tests skips the | ||
| src/mono conventions the same way, only it redirects to the src/libraries ones instead of the | ||
| repo defaults. --> | ||
| <Import Project="..\..\..\..\..\Directory.Build.props" /> |
Fixes #131922
Problem
Apple mobile CoreCLR test apps are ReadyToRun-compiled on the Helix machine from assemblies in
publish/. The build process then copied Crossgen2 output back into that same directory.Helix retries reuse the work-item directory. A retry therefore fed attempt 1's already-compiled, IL-stripped assemblies back into Crossgen2. The resulting app crashed before test discovery:
This is retry input corruption in the build orchestration, not a Crossgen2 compiler bug.
Fix
Keep
publish/as the stable Crossgen2 input for every attempt.For CoreCLR Mach-O R2R, the proxy now:
obj/R2R/before compiling;obj/r2r-app-bundle-source/frompublish/plus the current R2R output;AppleBuildDirand the bundle item lists at that directory.Crossgen2 and AppleAppBuilder output can no longer become input to a later retry, and partial output from a killed attempt is removed.
Regression coverage
The new
Apple.Build.Testshost test runs the real proxy targets twice in the same temporary work-item directory. It verifies that:publish/keeps the same files and contents;The test runs through the
apple.buildtestssubset on macOS x64 when Apple build files change and on non-PR rolling builds. It does not require an Apple SDK or device.Validation
./build.sh -s apple.buildtests -c Release -test: 1 passed, 0 failed.System.Net.Security.Unit.Teststhree times in one tvOS Helix work-item directory in build 1558829. All three attempts rebuilt R2R, launched the app, reached 128 tests, and had no stripped-IL orAPP_CRASHfailure. The work item remained red because of a separate certificate-context test failure.Note
This description and change were prepared with GitHub Copilot assistance and reviewed by the submitting developer.