Skip to content

[wasm] Standup EnableAgressiveTrimming for library tests #46367

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.

Sign up for GitHub

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

Merged
merged 48 commits into from
Jan 4, 2021
Merged

[wasm] Standup EnableAgressiveTrimming for library tests #46367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
57ef639
Trim wasm library tests
Dec 15, 2020
3436a5f
Add SystemBuffers HelloTest and only test that
Dec 15, 2020
9220864
Import linker from nuget to override sdk linker
Dec 16, 2020
3703995
Remove redundant assembly inclusions
Dec 16, 2020
4ba287b
Add WasmTestRunner referenced assemblies
Dec 16, 2020
fcb5c37
Add properties to reduce library size
Dec 17, 2020
0410010
Updates
Dec 17, 2020
043a0e2
Merge branch 'mdhwang/trim_library_tests' of github.com:mdh1418/runti…
Dec 17, 2020
f33c81b
Configure trimming for library tests
Dec 17, 2020
4f88882
Merge branch 'mdhwang/trim_library_tests' of github.com:mdh1418/runti…
Dec 17, 2020
5f2a3ba
Link assemblies except for test assembly
Dec 18, 2020
2c17b60
Use another path to WasmTestRunner.dll
Dec 18, 2020
036b34c
Fix typo
Dec 18, 2020
6667907
Merge branch 'mdhwang/trim_library_tests' of github.com:mdh1418/runti…
Dec 18, 2020
6969b5a
Setup correctly rooted assemblies
marek-safar Dec 20, 2020
79dc240
Don't copy WasmTestRunner.dll from wrong patch when using linker
marek-safar Dec 20, 2020
d6dfd82
Don't inflate publish output with packages which are not used for tes…
marek-safar Dec 20, 2020
9ef8529
Fix xunit framework files copying to publish location to not overwrite
marek-safar Dec 21, 2020
076fc93
Add linker descriptor for xunit code which linker cannot track
marek-safar Dec 21, 2020
79274dd
Add tested assembly to the test payload closure
marek-safar Dec 21, 2020
c46f92b
One more method to preserve
marek-safar Dec 21, 2020
b1130cb
Merge branch 'mdhwang/trim_library_tests' of github.com:mdh1418/runti…
Dec 21, 2020
72e600d
Preserving some xunit types to make sure we can actually discover the…
Dec 21, 2020
54efb29
Merge remote-tracking branch 'upstream/master' into mdhwang/trim_libr…
Dec 21, 2020
4fa8c51
Put Wasm.targets import in the right spot
Dec 21, 2020
4f52ee3
Add a mode where we can run the regular system.buffers tests
Dec 22, 2020
4fa5e03
Dont link out Microsoft.DotNet.RemoteExecutor
Dec 22, 2020
948ad2b
Move Microsoft.DotNet.RemoteExecutor inclusion to xml
Dec 22, 2020
7b0d350
Consolidate ConfigureTrimming itemgroups
Dec 23, 2020
1b447f0
Build PrepareForWasmBuildApp before WasmBuildApp
Dec 23, 2020
c0f503a
Remove manual import of WasmTestRunner assemblies
Dec 23, 2020
fd8e34c
Remove temporary HelloTest from System.Buffers
Dec 23, 2020
15f7f0d
Added flag to not aggressively link by default
Dec 23, 2020
5ada1d5
Merge branch 'mdhwang/trim_library_tests' of github.com:mdh1418/runti…
Dec 23, 2020
a9259ec
Revert import and build order changes
Dec 23, 2020
a97fb56
Condition trimming related changes on EnableAggressiveTrimming
Dec 23, 2020
316f98e
Revert "Don't copy WasmTestRunner.dll from wrong patch when using lin…
Dec 23, 2020
4900907
Add comment to note temporary RemoteExecutor assembly inclusion
Dec 28, 2020
00022b5
Move props from general tests to mobile tests
Dec 28, 2020
03bb472
Revert "Condition trimming related changes on EnableAggressiveTrimming"
Dec 29, 2020
2d35f3a
Broaden trimming options for all mobile targets
Dec 30, 2020
ce113f2
Enable PublishTrimmed regardless of OS when aggressively trimming
Jan 4, 2021
28e694f
Move trimming specific property to ConfigureTrimming
Jan 4, 2021
07568bb
Import NuGet linker in mobile tests by default
Jan 4, 2021
3262912
Add file description
Jan 4, 2021
7da7b82
Add detail to ILLink XML descriptor file
Jan 4, 2021
d00ea59
Reformat ConfigureTrimming target tag
Jan 4, 2021
41e177b
Fix typo
Jan 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions eng/testing/ILLink.Descriptor.xunit.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<!-- xunit 2.x version is not under development. We make xunit trimming compatible in methods which ILLink cannot track by providing XML descriptor to keep the dependencies -->
<linker>
<assembly fullname="xunit.execution.dotnet">
<type fullname="Xunit.Sdk.ReflectionAssemblyInfo">
<method signature="System.Void .ctor(System.String)" />
</type>
<type fullname="Xunit.Sdk.TestFrameworkProxy">
<method signature="System.Void .ctor(System.Object,System.Object,System.Object)" />
</type>
<type fullname="Xunit.Sdk.FactDiscoverer" />
</assembly>
<assembly fullname="xunit.core">
<namespace fullname="Xunit" />
</assembly>
<!-- Temporary until https://github.com/mono/linker/issues/1713 is resolved -->
<assembly fullname="Microsoft.DotNet.RemoteExecutor">
<type fullname="Microsoft.DotNet.RemoteExecutor.Program">
<method signature="System.Int32 Main(System.String[])" />
</type>
</assembly>
</linker>
40 changes: 38 additions & 2 deletions eng/testing/tests.mobile.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,22 @@
<RunAOTCompilation Condition="'$(TargetOS)' == 'iOS' and $(TargetArchitecture.StartsWith('arm'))">true</RunAOTCompilation>
<JSEngine Condition="'$(TargetOS)' == 'Browser' and '$(JSEngine)' == ''">V8</JSEngine>
<JSEngineArgs Condition="'$(JSEngine)' == 'V8'">$(JSEngineArgs) --engine-arg=--stack-trace-limit=1000</JSEngineArgs>

<PublishingTestsRun>true</PublishingTestsRun>
</PropertyGroup>

<PropertyGroup Condition="'$(TargetOS)' == 'Browser'">
<!-- We need to set this in order to get extensibility on xunit category traits and other arguments we pass down to xunit via MSBuild properties -->
<RunScriptCommand>$HARNESS_RUNNER wasm $XHARNESS_COMMAND --app=. --engine=$(JSEngine) $(JSEngineArgs) --js-file=runtime.js --output-directory=$XHARNESS_OUT -- $(RunTestsJSArguments) --run WasmTestRunner.dll $(AssemblyName).dll</RunScriptCommand>
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>
<DebuggerSupport>false</DebuggerSupport>
</PropertyGroup>

<PropertyGroup Condition="'$(EnableAggressiveTrimming)' == 'true'">
<PublishTrimmed>true</PublishTrimmed>
</PropertyGroup>

<PropertyGroup Condition="'$(TargetOS)' == 'Android'">
Expand Down Expand Up @@ -146,6 +157,30 @@

</Target>

<Target Name="ConfigureTrimming" Condition="'$(EnableAggressiveTrimming)' == 'true'" BeforeTargets="PrepareForILLink">
<PropertyGroup>
<TrimMode>link</TrimMode>
</PropertyGroup>

<ItemGroup>
<ManagedAssemblyToLink Include="$(OutDir)\*xunit*">
<IsTrimmable>true</IsTrimmable>
<TrimMode>link</TrimMode>
</ManagedAssemblyToLink>
<ManagedAssemblyToLink Condition="('%(ManagedAssemblyToLink.FileName).dll' != '$(MSBuildProjectName).dll')" >
<TrimMode>link</TrimMode>
</ManagedAssemblyToLink>
<ManagedAssemblyToLink Condition="('%(ManagedAssemblyToLink.FileName).dll' == '$(AssemblyName).dll')" >
<TrimMode>copy</TrimMode>
</ManagedAssemblyToLink>
Comment on lines +166 to +175
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 https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets#L142-L144 , I think instead of this, we should have a target that adds to @(ResolvedFileToPublish), and sets the TrimMode metadata as needed.

This will get processed by https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets#L235-L237 and subsequently by PrepareForILLink target.

  • IIUC, you just need to add the xunit files, if they aren't there already, and set TrimMode=link for them.
  • And make sure that the test assembly doesn't have IsTrimmable, or TrimMode set. this should handle the rest for you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using ResolvedFileToPublish makes sense if we can hook it early enough. Though I'd keep copy setting to be explicit and maybe add a comment that we want the tested assembly to be always copied only.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will address in a follow-up PR

<TrimmerRootAssembly Condition="'$(TargetOS)' == 'Android'" Include="AndroidTestRunner"/>
<TrimmerRootAssembly Condition="'$(TargetOS)' == 'Browser'" Include="WasmTestRunner"/>
<TrimmerRootAssembly Condition="'$(TargetOS)' == 'iOS'" Include="AppleTestRunner"/>
<TrimmerRootAssembly Include="$(AssemblyName)"/>
Comment on lines +176 to +179
Copy link
Member

Choose a reason for hiding this comment

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

These shouldn't be needed, I think, since the PrepareForILLink target will set these up for us.

Copy link
Contributor

Choose a reason for hiding this comment

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

These are different settings. I think PrepareForILLink will set only <TrimmerRootAssembly Include="$(AssemblyName)"/>

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that they might get picked up by

runtime/eng/testing/tests.mobile.targets

Lines 222 to 226 in 3262912

<_runnerFilesToPublish Include="$(AndroidTestRunnerDir)*" Condition="'$(TargetOS)' == 'Android'" />
<_runnerFilesToPublish Include="$(AppleTestRunnerDir)*" Condition="'$(TargetOS)' == 'iOS' or '$(TargetOS)' == 'tvOS'" />
<_runnerFilesToPublish Include="$(WasmTestRunnerDir)*" Condition="'$(TargetOS)' == 'Browser'" />
<ResolvedFileToPublish Include="@(_runnerFilesToPublish)" RelativePath="%(FileName)%(Extension)" CopyToPublishDirectory="PreserveNewest" PostprocessAssembly="true" />
into @(ResolvedFileToPublish)

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 just tried removing L181 <TrimmerRootAssembly Include="$(AssemblyName)"/> and started seeing

 fail: 10:28:48.0231240 System.AggregateException: AggregateException_ctor_DefaultMessage ()

  fail: 10:28:48.0232360  ---> System.IO.FileNotFoundException:

  fail: 10:28:48.0232420 IO_FileName_Name, System.Buffers.Tests

  fail: 10:28:48.0232440    at System.Reflection.Assembly.Load(AssemblyName assemblyRef, StackCrawlMark& stackMark, AssemblyLoadContext assemblyLoadContext)

  fail: 10:28:48.0232480    at System.Reflection.Assembly.Load(AssemblyName assemblyRef)

  fail: 10:28:48.0232610    at Xunit.Sdk.ReflectionAssemblyInfo..ctor(String assemblyFileName)

  fail: 10:28:48.0232650    at System.Reflection.RuntimeConstructorInfo.InternalInvoke(Object obj, Object[] parameters, Boolean wrapExceptions)

  fail: 10:28:48.0232660 --- End of stack trace from previous location ---

for ./dotnet.sh build /t:Test src/libraries/System.Buffers/tests/ /p:TargetOS=Browser /p:TargetArchitecture=wasm /p:Configuration=Debug /p:EnableAggressiveTrimming=true

Copy link
Member Author

Choose a reason for hiding this comment

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

Will address in a follow-up PR

<TrimmerRootDescriptor Include="$(MSBuildThisFileDirectory)ILLink.Descriptor.xunit.xml" />
</ItemGroup>
</Target>

<Import Project="$(MonoProjectRoot)\wasm\build\WasmApp.targets" Condition="'$(TargetOS)' == 'Browser'" />
<PropertyGroup>
<WasmBuildAppDependsOn>PrepareForWasmBuildApp;$(WasmBuildAppDependsOn)</WasmBuildAppDependsOn>
Expand Down Expand Up @@ -186,13 +221,14 @@
<_runnerFilesToPublish Include="$(AppleTestRunnerDir)*" Condition="'$(TargetOS)' == 'iOS' or '$(TargetOS)' == 'tvOS'" />
<_runnerFilesToPublish Include="$(WasmTestRunnerDir)*" Condition="'$(TargetOS)' == 'Browser'" />

<!-- Exclude xunit assemblies as those should be resolved by our own package references -->
<ResolvedFileToPublish Include="@(_runnerFilesToPublish)" Condition="!$([System.String]::Copy('%(Filename)').StartsWith('xunit.'))" RelativePath="%(FileName)%(Extension)" CopyToPublishDirectory="PreserveNewest" PostprocessAssembly="true" />
<ResolvedFileToPublish Include="@(_runnerFilesToPublish)" RelativePath="%(FileName)%(Extension)" CopyToPublishDirectory="PreserveNewest" PostprocessAssembly="true" />
</ItemGroup>
</Target>

<Target Name="PublishTestAsSelfContained"
Condition="'$(IsCrossTargetingBuild)' != 'true'"
AfterTargets="Build"
DependsOnTargets="Publish;BundleTestAppleApp;BundleTestAndroidApp;BundleTestWasmApp;ArchiveTests" />

<Import Project="$(RepositoryEngineeringDir)illink.targets" />
</Project>
4 changes: 2 additions & 2 deletions eng/testing/xunit/xunit.console.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@

<!-- Overwrite the runner config file with the app local one. -->
<Target Name="OverwriteDesktopTestRunnerConfigs"
Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and
Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and
'$(GenerateAppConfigurationFile)' == 'true' and
'@(AppConfigWithTargetPath)' != ''"
AfterTargets="CopyFilesToOutputDirectory">
Expand All @@ -57,7 +57,7 @@
</Target>

<!-- ResolveAssemblyReferences is the target that populates ReferenceCopyLocalPaths which is what is copied to output directory. -->
<Target Name="CopyRunnerToOutputDirectory" BeforeTargets="ResolveAssemblyReferences">
<Target Name="CopyRunnerToOutputDirectory" BeforeTargets="ResolveAssemblyReferences" Condition="'$(TargetsMobile)' != 'true'">
<ItemGroup>
<!-- Copy test runner to output directory -->
<None Include="$([System.IO.Path]::GetDirectoryName('$(XunitConsole472Path)'))\*"
Expand Down
2 changes: 1 addition & 1 deletion eng/testing/xunit/xunit.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<PackageReference Include="Microsoft.DotNet.XUnitExtensions" Version="$(MicrosoftDotNetXUnitExtensionsVersion)" />
</ItemGroup>

<ItemGroup Condition="'$(ArchiveTests)' != 'true'">
<ItemGroup Condition="'$(ArchiveTests)' != 'true' AND '$(PublishingTestsRun)' != 'true'">
<!-- Microsoft.Net.Test.Sdk brings a lot of assemblies with it. To reduce helix payload submission size we disable it on CI. -->
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="$(MicrosoftNETTestSdkVersion)" />
<PackageReference Include="xunit.runner.visualstudio" Version="$(XUnitRunnerVisualStudioVersion)" GeneratePathProperty="true" />
Expand Down