Skip to content
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

[ios][tests] Run functional tests with Mono and Native AOT on Helix #87773

Merged
merged 106 commits into from
Aug 11, 2023

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Jun 19, 2023

Description

Initial work can be found at https://github.com/steveisok/runtime/tree/ios-sample-lib-targets.

This PR adds support for Native AOT compilation on Helix. It improves test coverage for Mono and Native AOT by running functional tests on Helix. The proxy project is updated with Native AOT props and apple build targets are updated to support Native AOT compilation. Additionally, it simplifies the sample app by utilizing the shared Apple targets.

Fixes #85294

@kotlarmilos kotlarmilos added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it area-Infrastructure-mono os-ios Apple iOS labels Jun 19, 2023
@kotlarmilos kotlarmilos added this to the 8.0.0 milestone Jun 19, 2023
@kotlarmilos kotlarmilos self-assigned this Jun 19, 2023
@ghost
Copy link

ghost commented Jun 19, 2023

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

Work in progress.

This PR aims to add the Mono and Native AOT iOS samples as functional tests on a device to the Helix. Currently, it includes some temporary workarounds that will be resolved once the CI reports green.

initial work can be found at https://github.com/steveisok/runtime/tree/ios-sample-lib-targets.

Description

TBD

Author: kotlarmilos
Assignees: kotlarmilos
Labels:

NO-MERGE, NO-REVIEW, area-Infrastructure-mono, os-ios

Milestone: 8.0.0

@steveisok steveisok self-requested a review June 19, 2023 17:45
@steveisok
Copy link
Member

These are the artifacts we need to replace when UseNativeAOTRuntime is true. That should give us enough to aot on helix with.

  1. Replace this with the native aot in tree runtime pack. The build still produces one, but w/ only the system.native bits

<HelixCorrelationPayload Include="$(MicrosoftNetCoreAppRuntimePackDir)" Destination="build/microsoft.netcore.app.runtime.$(TargetOS)-$(TargetArchitecture.ToLower())" />

  1. Replace MonoAotCrossDir with the ILC equivalent. This item probably should be renamed to something more generic.

<HelixCorrelationPayload Include="$(MonoAotCrossDir)" Destination="build/cross" />

  1. Replace MonoTargetsTaskDir with something that points to the NativeAOT BuildIntegration targets

<HelixCorrelationPayload Include="$(MonoTargetsTasksDir)" Destination="build/MonoTargetsTasks" />

@kotlarmilos
Copy link
Member Author

kotlarmilos commented Jun 20, 2023

  1. Replace this with the native aot in tree runtime pack. The build still produces one, but w/ only the system.native bits

It should be the same path, right?

2. Replace MonoAotCrossDir with the ILC equivalent. This item probably should be renamed to something more generic.

How does the AppleBuild.targets find the compiler at build/cross on Helix? I want to make the name more generic, but not sure if it is predefined.


Update: When adding new artifacts to the Helix, update the HelixCorrelationPayload property in sendtohelix-mobile.targets with the mappings from the AppleBuild.LocalBuild.props.

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos kotlarmilos requested a review from ivanpovazan August 8, 2023 12:12
Copy link
Member

@ivanpovazan ivanpovazan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the two nits, this looks good.
However, since it is a quite large PR I would suggest someone else also to take a look before merging.

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@steveisok steveisok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for sticking with it.

src/libraries/sendtohelix-mobile.targets Outdated Show resolved Hide resolved
<_AOTBuildCommand>$(_AOTBuildCommand) /p:XHARNESS_EXECUTION_DIR=&quot;$XHARNESS_EXECUTION_DIR&quot; /p:RunAOTCompilation=$(RunAOTCompilation) /p:TargetOS=$(TargetOS) /p:TargetArchitecture=$(TargetArchitecture) /p:MonoForceInterpreter=$(MonoForceInterpreter) /p:MonoEnableLLVM=true /p:DevTeamProvisioning=$(DevTeamProvisioning) /p:UsePortableRuntimePack=true /p:Configuration=$(Configuration)</_AOTBuildCommand>
<_AOTBuildCommand>$(_AOTBuildCommand) /p:XHARNESS_EXECUTION_DIR=&quot;$XHARNESS_EXECUTION_DIR&quot; /p:RunAOTCompilation=$(RunAOTCompilation) /p:UseNativeAOTRuntime=$(UseNativeAOTRuntime) /p:TargetOS=$(TargetOS) /p:TargetArchitecture=$(TargetArchitecture) /p:MonoForceInterpreter=$(MonoForceInterpreter) /p:DevTeamProvisioning=$(DevTeamProvisioning) /p:UsePortableRuntimePack=true /p:Configuration=$(Configuration)</_AOTBuildCommand>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the other functional tests project files, most of them have MonoForceInterpreter set, but some have TreatAsLocalProperty="MonoForceInterpreter" some don't, so I believe the TreatAsLocalProperty was added as a workaround to prevent overwriting what a project specifies through the command line - like building on Helix.

To conclude, I feel like this needs to be refactored (maybe not in this PR), with either removing any notion of MonoForceInterpreter from the functional test's project file (and test the change), or leave it how it was to prevent any unwanted regression.

@steveisok what do you think?

I would suggest keeping it and slowly unwinding in a follow up.

# iOS/tvOS devices
# Build the whole product using Native AOT and run libraries tests
#
- template: /eng/pipelines/common/platform-matrix.yml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have the capacity to run these on every PR. Even when we kick off runtime-ioslike manually, there may be a capacity issue running both mono and nativeaot together.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, my concern was all libraries tests (when we get to it) and I see it's running smoke tests only.

@agocke
Copy link
Member

agocke commented Sep 7, 2023

Hey, I just noticed this is running libraries tests using Native AOT in the runtime pipeline. We've previously found that this was too expensive to run on every PR. Can we move these legs out of the runtime pipeline and into runtime-extra-platforms or similar?

@filipnavara
Copy link
Member

This concern was already previously voiced in a comment. The conclusion was that it runs only small subset of the tests (smoke tests). Are you seeing a bigger subset now? If it becomes an issue, is it possible to trigger it on specific path changes automatically?

@agocke
Copy link
Member

agocke commented Sep 7, 2023

Ah, nevermind, I see now this is only smoke tests. OK, lemme look in more detail at average run times. If it's not too expensive, we can continue running smoke tests.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants