Adding browser-wasm trimming test leg

[joperezr]

Fixes #48027

cc: @eerhardt @safern @marek-safar

contributes to #39274

This will add the execution of browser-wasm trimming tests. I have tested this locally and only have 4 remaining failing tests which I have to investigate, but I'm sending the PR in the meantime to start getting some feedback.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

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

Issue Details

---

cc: @eerhardt @safern @marek-safar

contributes to #39274

This will add the execution of browser-wasm trimming tests. I have tested this locally and only have 4 remaining failing tests which I have to investigate, but I'm sending the PR in the meantime to start getting some feedback.

Author:	joperezr
Assignees:	-
Labels:	area-Infrastructure-libraries
Milestone:	-

[ghost]

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

---

cc: @eerhardt @safern @marek-safar

contributes to #39274

This will add the execution of browser-wasm trimming tests. I have tested this locally and only have 4 remaining failing tests which I have to investigate, but I'm sending the PR in the meantime to start getting some feedback.

Author:	joperezr
Assignees:	-
Labels:	area-Infrastructure-libraries, linkable-framework
Milestone:	-

[ViktorHofer]

Curious, what was the overall reason to not import the msbuild files from the repository?

[joperezr]

Curious, what was the overall reason to not import the msbuild files from the repository?

Same reason our packaging tests currently won't import our own infrastructure for building: The whole point of these tests is for them to build as close as possible to how real consumer apps would be built (using built-in SDK targets and props only) with the only difference that we wanted to override the framework they build and run against to instead use the one we are currently live-building. The difference why we are in here adding partial set of those files here is that the logic within the files we are importing (all of the logic in test-mobile.targets) doesn't really exist on the sdk and in order to avoid the duplication of targets of how to build wasm appbundles we decided to instead reuse only those set of targets as part of these project build.

[ViktorHofer]

Thanks for the clarification Joe.

It would be great if #48462 could be merged first as it currently blocks Arcade's validation builds and dependency flow into runtime.

[joperezr]

It would be great if #48462 could be merged first as it currently blocks Arcade's validation builds and dependency flow into runtime.

Yeah, looks like I'll have to fix one more thing on the docker image first as it looks like the user running our builds doesn't have access to v8 as all trimming tests are failing with Permission Denied so I don't expect this PR to go in today.

[eerhardt]

Should this be $(MicrosoftNetCoreAppRuntimePackDir) instead?

[joperezr]

If I do that, then I need to pass in the two properties (MicrosoftNetCoreAppRuntimePackRidDir and MicrosoftNetCoreAppRuntimePackDir) since the one that the Wasm targets read is the Rid-specific one.

[joperezr]

which I'm fine if we want to do that, but I thought the idea was to avoid duplication of properties.

[eerhardt]

It just feels like there should be one base property that needs to be set, and everything flows from there. If not, then I'm fine with what you have now.

[eerhardt]

@joperezr - is there a reason CI isn't running on this change?

[joperezr]

It was running before, no idea why it didn't run on my latest commit. I won't trigger it manually for now since I know I'll need dotnet/dotnet-buildtools-prereqs-docker#412 to get merged before I can get the wasm tests to pass.

[marek-safar]

The fact that you can’t publish a plain ‘browser-wasm’ console app with the product is the root cause of this problem.

Why can you not do that? We publish for browser-wasm in other tests so I don't fully understand the problem here.

[joperezr]

Why can you not do that? We publish for browser-wasm in other tests so I don't fully understand the problem here.

You can publish for browser-wasm, I guess what @eerhardt meant is that you can't really run a console app published for browser-wasm just on its own. Instead you need to be able to load it in either a browser or on an engine (like v8) so the complaint here is that it would be ideal if the SDK could automatically 'pack' an AppBundle for an engine like v8 so that you could just do something like:

dotnet new console
dotnet publish -r browser-wasm
cd <publish out dir>
<run the executable which presumably would be something that calls v8 engine>

Of course this is not a mainline scenario as most people that target browser wasm will likely be doing dotnet new blazorwasm instead of console apps, so I don't expect the SDK adding this feature any time soon. For our blazor-wasm tests, we do the exact same thing that I'm doing on this PR for the trimming tests, which is creating a console app, publishing it trimmed for browser-wasm, and then calling special targets in our repo to create an appbundle so that the app can be executed using v8.

[marek-safar]

@joperezr it's actually one of our supported scenarios. I don't have deep insight into the difference compared to your setup but https://github.com/dotnet/runtime/blob/master/src/tests/FunctionalTests/wasm/Interpreter/console/Wasm.Interpreter.Console.Test.csproj works and @akoeplinger, @radical can provide more details.

[eerhardt]

it's actually one of our supported scenarios... and @akoeplinger, @radical can provide more details.

@akoeplinger, @radical can you show me how to do it with only using an SDK installed from https://github.com/dotnet/installer#installers-and-binaries and without custom MSBuild tasks/targets?

My understanding is that this only works for console apps that are using the dotnet/runtime build infrastructure. Specifically these files:

runtime/src/tests/FunctionalTests/Directory.Build.props

Lines 1 to 11 in afa7e27

	<Project>
	<PropertyGroup>
	<RunAnalyzers>false</RunAnalyzers>
	<TreatWarningsAsErrors>false</TreatWarningsAsErrors>
	<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
	<IsTestProject>true</IsTestProject>
	<IsFunctionalTest>true</IsFunctionalTest>
	</PropertyGroup>
	<Import Project="..\..\libraries\Directory.Build.props" />
	</Project>

runtime/src/tests/FunctionalTests/Directory.Build.targets

Lines 1 to 3 in afa7e27

	<Project>
	<Import Project="..\..\libraries\Directory.Build.targets" />
	</Project>

[joperezr]

My biggest concern with this change is that the linker infrastructure now imports selective files (tests.mobile.targets) which aren't supposed to be imported without the rest of the infrastructure in dotnet/runtime being present. Imagine someone changing some of the targets in that file or importing yet another bunch of files from it which would result in linker tests being broken if properties / items / targets are referenced that aren't known to the linker tests.

Your concern is valid @ViktorHofer and something we have thought about. We do however specifically don't want to import the rest of the infrastructure from runtime in these tests as it would be very easy to run into a case where settings made on the build infra (which does change trimming-related settings) would "loosen" how trimming happens and make the tests to silently pass when they should have failed and our customers would be broken. I know originally these targets weren't really meant to be used in isolation, but it actually didn't require many global properties to be set (which are basically the ones that are getting passed down to the template in this PR) and if new dependencies on global properties are added it would be easy to catch as the linker test CI would be broken. In terms of isolating the file more to just have what we need, that is about 95% of the file, so basically everything except for this:

runtime/eng/testing/tests.mobile.targets

Lines 165 to 188 in 121b732

	<Target Name="ConfigureTrimming" Condition="'$(EnableAggressiveTrimming)' == 'true'" AfterTargets="AddTestRunnersToPublishedFiles">
	<PropertyGroup>
	<TrimMode>link</TrimMode>
	</PropertyGroup>
	<ItemGroup>
	<!-- Mark all the assemblies for link. We will explicitly mark the non-trimmable ones -->
	<ResolvedFileToPublish TrimMode="link" />
	<!-- Don't trim the main assembly.
	TrimMode="" is needed so the root assemblies are correctly identified -->
	<ResolvedFileToPublish TrimMode="" Condition="'%(FileName)' == '$(AssemblyName)'" />
	<!-- Even though we are trimming the test runner assembly, we want it to be treated
	as a root -->
	<TrimmerRootAssembly
	Condition="$([System.String]::Copy('%(ResolvedFileToPublish.FileName)%(ResolvedFileToPublish.Extension)').EndsWith('TestRunner.dll'))"
	Include="%(ResolvedFileToPublish.FullPath)" />
	</ItemGroup>
	<ItemGroup>
	<TrimmerRootDescriptor Include="$(MSBuildThisFileDirectory)ILLink.Descriptor.xunit.xml" />
	</ItemGroup>
	</Target>

which is actually something we specifically want to make sure it doesn't run, but today that is the case as this target is opt-in and the condition is false today.

[radical]

@akoeplinger, @radical can you show me how to do it with only using an SDK installed from https://github.com/dotnet/installer#installers-and-binaries and without custom MSBuild tasks/targets?

You can't do it just with just the sdk right now. But it is possible to build non-blazorwasm projects out of the tree. See https://github.com/radical/blazor-wasm-test/tree/main/console-out-of-tree . With the current master, you will also need a little patch:

diff --git a/src/mono/wasm/build/WasmApp.LocalBuild.props b/src/mono/wasm/build/WasmApp.LocalBuild.props
index b545e5764d8..145e315cc00 100644
--- a/src/mono/wasm/build/WasmApp.LocalBuild.props
+++ b/src/mono/wasm/build/WasmApp.LocalBuild.props
@@ -45,7 +45,7 @@

   <PropertyGroup>
     <MicrosoftNetCoreAppRuntimePackRidDir>$(MicrosoftNetCoreAppRuntimePackLocationToUse)runtimes\browser-wasm\</MicrosoftNetCoreAppRuntimePackRidDir>
-    <MonoAotCrossCompilerPath>$(MicrosoftNetCoreAppRuntimePackRidDir)native\cross\$(RuntimeIdentifier)\mono-aot-cross</MonoAotCrossCompilerPath>
+    <MonoAotCrossCompilerPath>$(MicrosoftNetCoreAppRuntimePackRidDir)native\cross\browser-wasm\mono-aot-cross</MonoAotCrossCompilerPath>
     <MonoAotCrossCompilerPath Condition="$([MSBuild]::IsOSPlatform('WINDOWS'))">$(MonoAotCrossCompilerPath).exe</MonoAotCrossCompilerPath>

     <WasmAppBuilderTasksAssemblyPath>$([MSBuild]::NormalizePath('$(WasmAppBuilderDir)', 'WasmAppBuilder.dll'))</WasmAppBuilderTasksAssemblyPath>
diff --git a/src/mono/wasm/build/WasmApp.targets b/src/mono/wasm/build/WasmApp.targets
index d041b0a5aac..67010c876b4 100644
--- a/src/mono/wasm/build/WasmApp.targets
+++ b/src/mono/wasm/build/WasmApp.targets
@@ -157,7 +157,7 @@
       <WasmBuildNative Condition="'$(RunAOTCompilation)' == 'true'">true</WasmBuildNative>
       <WasmAppDir Condition="'$(WasmAppDir)' == ''">$(OutputPath)AppBundle\</WasmAppDir>
       <WasmMainAssemblyFileName Condition="'$(WasmMainAssemblyFileName)' == ''">$(TargetFileName)</WasmMainAssemblyFileName>
-      <MonoAotCrossCompilerPath Condition="'$(MonoAotCrossCompilerPath)' == ''">$(MicrosoftNetCoreAppRuntimePackRidDir)native\cross\$(PackageRID)\mono-aot-cross$(_ExeExt)</MonoAotCrossCompilerPath>
+      <MonoAotCrossCompilerPath Condition="'$(MonoAotCrossCompilerPath)' == ''">$(MicrosoftNetCoreAppRuntimePackRidDir)native\cross\browser-wasm\mono-aot-cross$(_ExeExt)</MonoAotCrossCompilerPath>

       <!-- emcc, and mono-aot-cross don't like relative paths for output files -->
       <_WasmIntermediateOutputPath>$([System.IO.Path]::GetFullPath('$(IntermediateOutputPath)\wasm\'))</_WasmIntermediateOutputPath>

Follow the steps in the README.md to build, and then make run.

[radical]

We have WasmApp.InTree.*, and WasmApp.LocalBuild* files to help with in-tree, and out-of-tree builds - https://github.com/dotnet/runtime/tree/master/src/mono/wasm/build

[eerhardt]

This looks good to me. Thanks for the good work here, @joperezr!

[ghost]

Hello @joperezr!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.