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

[infra] Enable trimming on Helix for Apple mobile #96169

Closed
wants to merge 99 commits into from

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Dec 19, 2023

Description

This PR (fixes #91923) introduces the trimming of library tests on Helix for Apple mobile targets. With trimming enabled, the configuration more closely mirrors the experience of an end user when building Apple mobile applications.

This change results in a 20min (42%) reduction in CI job builds for iOS (a subset of tests) and a 22min (18%) reduction for tvOS (the complete coverage)

Initial work is available at #92719.

Test job run: https://dev.azure.com/dnceng-public/public/_build/results?buildId=622954&view=results

@kotlarmilos kotlarmilos added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-Infrastructure-mono os-ios Apple iOS labels Dec 19, 2023
@kotlarmilos kotlarmilos added this to the 9.0.0 milestone Dec 19, 2023
@kotlarmilos kotlarmilos self-assigned this Dec 19, 2023
@ghost
Copy link

ghost commented Dec 19, 2023

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

Issue Details

Description

This PR tests trimming on Helix for Apple mobile targets.

Initial work is available at #92719.

Author: kotlarmilos
Assignees: kotlarmilos
Labels:

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

Milestone: 9.0.0

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslikesimulator

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

testBuildArgs: /p:ArchiveTests=true /p:DevTeamProvisioning=- /p:RunAOTCompilation=true /p:MonoForceInterpreter=true /p:BuildTestsOnHelix=true
testRunNamePrefixSuffix: Mono_$(_BuildConfig)
extraHelixArguments: /p:NeedsToBuildAppsOnHelix=true
extraStepsTemplate: /eng/pipelines/common/templates/runtimes/build-runtime-tests-and-send-to-helix.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 moved from extraStepsTemplate/extraStepsParameters in #92375, is there a particular reason as to why we are moving back?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, reverted. I assume this was a merge conflict issue.

@@ -16,7 +16,7 @@
<!-- running aot-helix tests locally, so we can test with the same project file as CI -->
<_AOTBuildCommand Condition="'$(ContinuousIntegrationBuild)' != 'true'">$(_AOTBuildCommand) /p:RuntimeSrcDir=$(RepoRoot) /p:RuntimeConfig=$(Configuration)</_AOTBuildCommand>
<!-- The command below sets default properties for runtime and library tests -->
<_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:MonoEnableLLVM=true /p:DevTeamProvisioning=$(DevTeamProvisioning) /p:UsePortableRuntimePack=true /p:Configuration=$(Configuration)</_AOTBuildCommand>
<_AOTBuildCommand>$(_AOTBuildCommand) /p:XHARNESS_EXECUTION_DIR=&quot;$XHARNESS_EXECUTION_DIR&quot; /p:EnableAggressiveTrimming=$(EnableAggressiveTrimming) /p:UsePortableRuntimePack=$(UsePortableRuntimePack) /p:RunAOTCompilation=$(RunAOTCompilation) /p:UseNativeAOTRuntime=$(UseNativeAOTRuntime) /p:TargetOS=$(TargetOS) /p:TargetArchitecture=$(TargetArchitecture) /p:MonoForceInterpreter=$(MonoForceInterpreter) /p:MonoEnableLLVM=true /p:DevTeamProvisioning=$(DevTeamProvisioning) /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.

Which scenarios would we not want to use the portable runtime pack?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can revert this change. We were testing different options for trimming on helix and one of them was to trim on build machines.

Comment on lines 10 to 11
<OriginalPublishDir>$([System.IO.Path]::GetFullPath('$(TestRootDir)..\publish\'))</OriginalPublishDir>
<ExtraFilesPath>$(OriginalPublishDir)..\extraFiles\</ExtraFilesPath>
Copy link
Member

Choose a reason for hiding this comment

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

These are overwriting L6/L7

<Copy SourceFiles="@(_RuntimePackFiles)"
DestinationFolder="$(OriginalPublishDir)" />
</Target>

<Target Name="_TrimApp" DependsOnTargets="ILLink;CopyFilesToPublishDirectory">
<Delete Files="@(_RemovedManagedAssembly)" />
Copy link
Member

Choose a reason for hiding this comment

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

Should we be deleting these files?

Is the main intent to prevent them from being added to AppleAssembliesToBundle?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, let's try to use _RemovedManagedAssembly to filter out AppleAssembliesToBundle.

Comment on lines +117 to +118
<ReferencePath Include="@(AppleReferenceSharedPathFiles->'$(MicrosoftNetCoreAppRuntimePackLibDir)%(FileName)%(Extension)')" />
<ReferencePath Include="@(AppleReferenceExtraPathFiles->'$(ExtraFilesPath)%(FileName)%(Extension)')" />
Copy link
Member

Choose a reason for hiding this comment

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

What do these do?

Copy link
Member Author

@kotlarmilos kotlarmilos Apr 3, 2024

Choose a reason for hiding this comment

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

The ReferencePath items are used by ILLink targets.

<AppleBuildDependsOn Condition="'$(EnableAggressiveTrimming)' == 'true' and '$(UseNativeAOTRuntime)' != 'true'">ConfigureTrimming;_TrimApp;$(AppleBuildDependsOn)</AppleBuildDependsOn>
<AppleBuildDependsOn>_PublishRuntimePack;_PrepareForAppleBuildAppOnHelix;$(AppleBuildDependsOn);_AfterAppleBuildOnHelix</AppleBuildDependsOn>
<!-- Forced by ILLink targets -->
<SelfContained>true</SelfContained>
Copy link
Member

Choose a reason for hiding this comment

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

Are we always hitting ILLink targets now (if so, we can probably remove L29 which also sets SelfContained to true right?)? Or is this now required because of ConfigureTrimming being added (if so we should only set this for the same conditions that ConfigureTrimming has right?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I believe we can remove it.

<AppleBuildDependsOn>_PublishRuntimePack;_PrepareForAppleBuildAppOnHelix;$(AppleBuildDependsOn);_AfterAppleBuildOnHelix</AppleBuildDependsOn>
<!-- Forced by ILLink targets -->
<SelfContained>true</SelfContained>
<PublishDir>$(OriginalPublishDir)</PublishDir>
Copy link
Member

Choose a reason for hiding this comment

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

Is this also forced by ILLink targets? Or is there another reason to have this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is used by ILLink targets.


<Target Name="BundleTestAppleApp" DependsOnTargets="AppleBuild" />

<Target Name="_PrepareForAppleBuildAppOnHelix" DependsOnTargets="_PublishRuntimePack">
<Target Name="_PrepareForAppleBuildAppOnHelix">
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, does moving _PublishRuntimePack from this target's DependsOnTargets up to AppleBuildDependsOn change anything functionally?

If this target is the primary reason to have _PublishRuntimePack, I think it would be better to keep that dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't change the functionality. The primary reason for moving it to the AppleBuildDependsOn is to have a clear list of targets executed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it should be reverted/added as a DependsOnTargets in _PrepareForAppleBuildAppOnHelix?

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

Closing this PR in favor of #100669 which has rebased commits.

@kotlarmilos kotlarmilos closed this Apr 5, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mono] Update tests to be trim-compatible on ios/tvos platforms
4 participants