-
Notifications
You must be signed in to change notification settings - Fork 4.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable NativeAOT runtime tests on MacCatalyst #102882
base: main
Are you sure you want to change the base?
Enable NativeAOT runtime tests on MacCatalyst #102882
Conversation
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this.
Overall it looks good, but I made a few comments and have questions before approving this.
- template: /eng/pipelines/common/templates/runtimes/build-runtime-tests-and-send-to-helix.yml | ||
parameters: | ||
creator: dotnet-bot | ||
testBuildArgs: tree nativeaot/SmokeTests /p:BuildNativeAOTRuntimePack=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to set /p:BuildNativeAOTRuntimePack=true
here, I thought the property is used only when building the runtime bits, not the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't recall the initial reason for including it, and it is likely no longer needed since we now use a single build command. I believe it was related to the initial multi-command build process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this property jobs reports:
eng/liveBuilds.targets(135,5): error : The 'coreclr' subset must be built before building this project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking it out. I think we should investigate/refactor this (as a follow-up).
@@ -77,3 +77,40 @@ jobs: | |||
creator: dotnet-bot | |||
interpreter: true | |||
testRunNamePrefixSuffix: Mono_$(_BuildConfig) | |||
|
|||
# | |||
# Build the whole product using Native AOT and run runtime tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be running sandboxed tests as well, as that is matching more what the end user would experience (sandbox entitlement is required by the App Store).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are failures for the Sandboxed version in the CI, I believe DevTeamProvisioning
needs to be set to adhoc
(but not 100% sure).
@@ -484,6 +484,35 @@ $__Command $HARNESS_RUNNER apple just-run %5c | |||
-v | |||
CLRTestExitCode=$? | |||
|
|||
# Exit code of xharness is zero when tests finished successfully |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need special-case maccatalyst
now or to be precise why just-run
is not permitted?
How did it work before running maccatalyst
tests with Mono?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The xharness doesn't support the just-run
command for MacCatalyst: https://github.com/dotnet/xharness/blob/6ec0d581b6f420a225a13784911624eda64b2653/src/Microsoft.DotNet.XHarness.CLI/CommandArguments/Apple/AppleJustRunCommandArguments.cs#L51. I couldn't find the reason for this in the commit history. However, I believe this could be related to the installation process, which differs for MacCatalyst compared to other Apple mobile platforms.
The Mono tests on MacCatalyst include the AppleTestRunner. To support this for Native AOT, we need to implement #91871.
/cc: @premun Do you know why apple [un]install
and apple just-run
commands are not supported for MacCatalyst?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly, run
accepts path to the app which it installs and runs (for all platforms). For maccatalyst it technically just calls open [app]
.
The just-run
command accepts app name and just opens the app on the device without installation. maccatalyst needs to have the app bundle path because technically we don't install anything anywhere. So it could technically get the path instead of the app bundle name but I guess it just didn't make sense because of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation, I am good with the changes.
<Copy SourceFiles="@(RecursiveCopyHack)" DestinationFolder="$(FinalPath)/%(RecursiveDir)" /> | ||
<!-- MacCatalyst requires Contents/Info.plist and Contents/MacOS/ApplicationBinary --> | ||
<Copy Condition="'$(TargetOS)' == 'maccatalyst' AND '%(BundleContent.Filename)%(BundleContent.Extension)' == 'Info.plist'" SourceFiles="@(BundleContent)" DestinationFolder="$(FinalPath)/Contents/%(RecursiveDir)" /> | ||
<Copy Condition="'$(TargetOS)' == 'maccatalyst' AND '%(BundleContent.Filename)%(BundleContent.Extension)' != 'Info.plist'" SourceFiles="@(BundleContent)" DestinationFolder="$(FinalPath)/Contents/MacOS/%(RecursiveDir)" /> | ||
<Copy Condition="'$(TargetOS)' != 'maccatalyst'" SourceFiles="@(BundleContent)" DestinationFolder="$(FinalPath)/%(RecursiveDir)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't Contents
dir already contained in the BundleContent
? (I am not sure why we need to special-case maccatalyst)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be already there, but I couldn't recursively copy content in msbuild from the build directory to the bundle. The RecursiveDir
metadata for BundleContent
is empty here. Please share any ideas on how to avoid "manual" copy.
It is in place to verify E2E tests, but I think we need to resolve this before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RecursiveDir metadata for BundleContent is empty here.
Yes, that is what I was confused about.
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
Description
This PR enables a subset of runtime tests to run Native AOT on MacCatalyst. It updates the runtime pipeline to support executing these tests on MacCatalyst.
Changes
This PR updates the CLRTest.Execute.Bash.targets file to set the
apple run
command for MacCatalyst. The commandapple just-run
used on Apple mobile is not permitted, andapple test
requires the a test runner. Additionally, it is necessary to locateInfo.plist
in theContents/
directory and the binary inContents/MacOS/
within the bundle.Verification
The pipelines
runtime-extra-platforms
andruntime-maccatalyst
should include Native AOT on MacCatalyst.Additional Notes
We should review the runtime pipeline and consider consolidating it with the libraries pipeline.
Fixes #89437