Adding browser-wasm trimming test leg - #48429

Merged
13 commits merged into
dotnet:masterfrom
joperezr:TrimmingWasmLeg
Feb 27, 2021
Merged

Adding browser-wasm trimming test leg#48429
13 commits merged into
dotnet:masterfrom
joperezr:TrimmingWasmLeg

Conversation

@joperezr

@joperezrjoperezr commented Feb 17, 2021•
edited
Loading

Copy link
Copy Markdown
Member

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.

@ghost

ghost commented Feb 17, 2021

Copy link
Copy Markdown

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

ghost commented Feb 17, 2021

Copy link
Copy Markdown

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:-

@eerhardteerhardt added the linkable-framework Issues associated with delivering a linker friendly framework label Feb 18, 2021
@ghost

ghost commented Feb 18, 2021

Copy link
Copy Markdown

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:-

marek-safar
marek-safar reviewed Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets Outdated
eerhardt
eerhardt reviewed Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template Outdated
eerhardt
eerhardt reviewed Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template Outdated
@ViktorHofer

ViktorHofer commented Feb 18, 2021

Copy link
Copy Markdown
Member

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

eerhardt
eerhardt reviewed Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template Outdated
eerhardt
eerhardt reviewed Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets Outdated
@joperezr

joperezr commented Feb 18, 2021

Copy link
Copy Markdown
MemberAuthor

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

ViktorHofer commented Feb 18, 2021

Copy link
Copy Markdown
Member

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

joperezr commented Feb 18, 2021

Copy link
Copy Markdown
MemberAuthor

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
eerhardt reviewed Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets Outdated

@eerhardteerhardtFeb 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be $(MicrosoftNetCoreAppRuntimePackDir) instead?

@joperezrjoperezrFeb 18, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

@joperezrjoperezrFeb 18, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

@eerhardteerhardtFeb 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

eerhardt commented Feb 19, 2021

Copy link
Copy Markdown
Member

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

@joperezr

joperezr commented Feb 19, 2021

Copy link
Copy Markdown
MemberAuthor

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.

This was referenced Feb 19, 2021
Closed
Merged
eerhardt
eerhardt reviewed Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets Outdated
eerhardt
eerhardt reviewed Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets Outdated
eerhardt
eerhardt reviewed Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/trimmingTests.targets Outdated
@runfoapprunfoappBot mentioned this pull request Feb 24, 2021
Closed
ViktorHofer
ViktorHofer reviewed Feb 24, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets Outdated
@runfoapprunfoappBot mentioned this pull request Feb 25, 2021
Closed
@marek-safar

marek-safar commented Feb 26, 2021

Copy link
Copy Markdown
Contributor

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

joperezr commented Feb 26, 2021

Copy link
Copy Markdown
MemberAuthor

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

marek-safar commented Feb 26, 2021

Copy link
Copy Markdown
Contributor

@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

eerhardt commented Feb 26, 2021

Copy link
Copy Markdown
Member

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>
<ImportProject="..\..\libraries\Directory.Build.props" />
</Project>

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

Lines 1 to 3 in afa7e27

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

@joperezr

joperezr commented Feb 26, 2021

Copy link
Copy Markdown
MemberAuthor

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

<TargetName="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 -->
<ResolvedFileToPublishTrimMode="link" />
<!-- Don't trim the main assembly.
TrimMode="" is needed so the root assemblies are correctly identified -->
<ResolvedFileToPublishTrimMode=""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>
<TrimmerRootDescriptorInclude="$(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.

@joperezrjoperezr mentioned this pull request Feb 26, 2021
Closed
@radical

radical commented Feb 26, 2021

Copy link
Copy Markdown
Member

@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

radical commented Feb 26, 2021

Copy link
Copy Markdown
Member

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
eerhardt approved these changes Feb 26, 2021
View reviewed changes

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

eerhardt
eerhardt reviewed Feb 27, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template Outdated
@ghost

ghost commented Feb 27, 2021

Copy link
Copy Markdown

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.

@ghost
ghost merged commit 76c2ac0 into dotnet:masterFeb 27, 2021
@ghostghost locked as resolved and limited conversation to collaborators Mar 29, 2021
@joperezr
joperezr deleted the TrimmingWasmLeg branch August 11, 2021 20:59
This pull request was closed.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lewinglewinglewing left review comments
@marek-safarmarek-safarmarek-safar left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@eerhardteerhardteerhardt approved these changes
@safernsafernsafern left review comments
+1 more reviewer
@buyaa-nbuyaa-nbuyaa-n left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Infrastructure-librarieslinkable-frameworkIssues associated with delivering a linker friendly framework

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Building linker tests spawns hundreds of NuGet out-of-process restore executables

8 participants

@joperezr@ViktorHofer@eerhardt@marek-safar@radical@lewing@safern@buyaa-n
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Adding browser-wasm trimming test leg - #48429
Merged
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Feb 27, 2021
Merged
Adding browser-wasm trimming test leg#48429
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Conversation
@joperezr
@joperezrjoperezr
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Member
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.
@ghost
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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:-
@eerhardteerhardt
added
the 
linkable-framework
Issues associated with delivering a linker friendly framework
label
Feb 18, 2021
@ghost
ghost
commented
Feb 18, 2021

Copy link


Copy Markdown

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:-
marek-safar
marek-safar
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ViktorHofer
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
Curious, what was the overall reason to not import the msbuild files from the repository?
eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@joperezr
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
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
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@eerhardteerhardtFeb 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Should this be $(MicrosoftNetCoreAppRuntimePackDir) instead?
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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
eerhardt
commented
Feb 19, 2021

Copy link


Copy Markdown

Member
@joperezr - is there a reason CI isn't running on this change?
@joperezr
joperezr
commented
Feb 19, 2021

Copy link


Copy Markdown

MemberAuthor
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.

This was referenced Feb 19, 2021

Closed

 Merged

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/trimmingTests.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 24, 2021

Closed

ViktorHofer
ViktorHofer
reviewed
Feb 24, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 25, 2021

Closed

@marek-safar
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
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
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
@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
eerhardt
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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> 
 <ImportProject="..\..\libraries\Directory.Build.props" /> 
 </Project> 
runtime/src/tests/FunctionalTests/Directory.Build.targets

Lines 1 to 3
in
afa7e27
 <Project> 
 <ImportProject="..\..\libraries\Directory.Build.targets" /> 
 </Project> 
@joperezr
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
 <TargetName="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 -->
 <ResolvedFileToPublishTrimMode="link" /> 
<!-- Don't trim the main assembly.
 TrimMode="" is needed so the root assemblies are correctly identified -->
 <ResolvedFileToPublishTrimMode=""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> 
 <TrimmerRootDescriptorInclude="$(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.
@joperezrjoperezr
mentioned this pull request
Feb 26, 2021

Closed

@radical
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
@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
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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
eerhardt
approved these changes
Feb 26, 2021
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This looks good to me. Thanks for the good work here, @joperezr!
eerhardt
eerhardt
reviewed
Feb 27, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ghost
ghost
commented
Feb 27, 2021

Copy link


Copy Markdown

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.
@ghost

ghost

merged commit 76c2ac0
into
dotnet:masterFeb 27, 2021
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2021
@joperezr
joperezr
deleted the

TrimmingWasmLeg

branch
August 11, 2021 20:59

This pull request was closed.

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@lewinglewinglewing left review comments
@marek-safarmarek-safarmarek-safar left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@eerhardteerhardteerhardt approved these changes
@safernsafernsafern left review comments

+1 more reviewer

@buyaa-nbuyaa-nbuyaa-n left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Infrastructure-librarieslinkable-frameworkIssues associated with delivering a linker friendly framework

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Building linker tests spawns hundreds of NuGet out-of-process restore executables


8 participants

@joperezr@ViktorHofer@eerhardt@marek-safar@radical@lewing@safern@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Adding browser-wasm trimming test leg - #48429
Merged
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Feb 27, 2021
Merged
Adding browser-wasm trimming test leg#48429
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Conversation
@joperezr
@joperezrjoperezr
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Member
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.
@ghost
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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:-
@eerhardteerhardt
added
the 
linkable-framework
Issues associated with delivering a linker friendly framework
label
Feb 18, 2021
@ghost
ghost
commented
Feb 18, 2021

Copy link


Copy Markdown

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:-
marek-safar
marek-safar
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ViktorHofer
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
Curious, what was the overall reason to not import the msbuild files from the repository?
eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@joperezr
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
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
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@eerhardteerhardtFeb 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Should this be $(MicrosoftNetCoreAppRuntimePackDir) instead?
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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
eerhardt
commented
Feb 19, 2021

Copy link


Copy Markdown

Member
@joperezr - is there a reason CI isn't running on this change?
@joperezr
joperezr
commented
Feb 19, 2021

Copy link


Copy Markdown

MemberAuthor
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.

This was referenced Feb 19, 2021

Closed

 Merged

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/trimmingTests.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 24, 2021

Closed

ViktorHofer
ViktorHofer
reviewed
Feb 24, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 25, 2021

Closed

@marek-safar
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
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
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
@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
eerhardt
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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> 
 <ImportProject="..\..\libraries\Directory.Build.props" /> 
 </Project> 
runtime/src/tests/FunctionalTests/Directory.Build.targets

Lines 1 to 3
in
afa7e27
 <Project> 
 <ImportProject="..\..\libraries\Directory.Build.targets" /> 
 </Project> 
@joperezr
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
 <TargetName="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 -->
 <ResolvedFileToPublishTrimMode="link" /> 
<!-- Don't trim the main assembly.
 TrimMode="" is needed so the root assemblies are correctly identified -->
 <ResolvedFileToPublishTrimMode=""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> 
 <TrimmerRootDescriptorInclude="$(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.
@joperezrjoperezr
mentioned this pull request
Feb 26, 2021

Closed

@radical
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
@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
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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
eerhardt
approved these changes
Feb 26, 2021
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This looks good to me. Thanks for the good work here, @joperezr!
eerhardt
eerhardt
reviewed
Feb 27, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ghost
ghost
commented
Feb 27, 2021

Copy link


Copy Markdown

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.
@ghost

ghost

merged commit 76c2ac0
into
dotnet:masterFeb 27, 2021
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2021
@joperezr
joperezr
deleted the

TrimmingWasmLeg

branch
August 11, 2021 20:59

This pull request was closed.

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@lewinglewinglewing left review comments
@marek-safarmarek-safarmarek-safar left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@eerhardteerhardteerhardt approved these changes
@safernsafernsafern left review comments

+1 more reviewer

@buyaa-nbuyaa-nbuyaa-n left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Infrastructure-librarieslinkable-frameworkIssues associated with delivering a linker friendly framework

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Building linker tests spawns hundreds of NuGet out-of-process restore executables


8 participants

@joperezr@ViktorHofer@eerhardt@marek-safar@radical@lewing@safern@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Adding browser-wasm trimming test leg - #48429
Merged
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Feb 27, 2021
Merged
Adding browser-wasm trimming test leg#48429
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Conversation
@joperezr
@joperezrjoperezr
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Member
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.
@ghost
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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:-
@eerhardteerhardt
added
the 
linkable-framework
Issues associated with delivering a linker friendly framework
label
Feb 18, 2021
@ghost
ghost
commented
Feb 18, 2021

Copy link


Copy Markdown

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:-
marek-safar
marek-safar
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ViktorHofer
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
Curious, what was the overall reason to not import the msbuild files from the repository?
eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@joperezr
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
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
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@eerhardteerhardtFeb 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Should this be $(MicrosoftNetCoreAppRuntimePackDir) instead?
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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
eerhardt
commented
Feb 19, 2021

Copy link


Copy Markdown

Member
@joperezr - is there a reason CI isn't running on this change?
@joperezr
joperezr
commented
Feb 19, 2021

Copy link


Copy Markdown

MemberAuthor
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.

This was referenced Feb 19, 2021

Closed

 Merged

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/trimmingTests.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 24, 2021

Closed

ViktorHofer
ViktorHofer
reviewed
Feb 24, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 25, 2021

Closed

@marek-safar
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
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
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
@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
eerhardt
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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> 
 <ImportProject="..\..\libraries\Directory.Build.props" /> 
 </Project> 
runtime/src/tests/FunctionalTests/Directory.Build.targets

Lines 1 to 3
in
afa7e27
 <Project> 
 <ImportProject="..\..\libraries\Directory.Build.targets" /> 
 </Project> 
@joperezr
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
 <TargetName="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 -->
 <ResolvedFileToPublishTrimMode="link" /> 
<!-- Don't trim the main assembly.
 TrimMode="" is needed so the root assemblies are correctly identified -->
 <ResolvedFileToPublishTrimMode=""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> 
 <TrimmerRootDescriptorInclude="$(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.
@joperezrjoperezr
mentioned this pull request
Feb 26, 2021

Closed

@radical
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
@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
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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
eerhardt
approved these changes
Feb 26, 2021
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This looks good to me. Thanks for the good work here, @joperezr!
eerhardt
eerhardt
reviewed
Feb 27, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ghost
ghost
commented
Feb 27, 2021

Copy link


Copy Markdown

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.
@ghost

ghost

merged commit 76c2ac0
into
dotnet:masterFeb 27, 2021
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2021
@joperezr
joperezr
deleted the

TrimmingWasmLeg

branch
August 11, 2021 20:59

This pull request was closed.

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@lewinglewinglewing left review comments
@marek-safarmarek-safarmarek-safar left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@eerhardteerhardteerhardt approved these changes
@safernsafernsafern left review comments

+1 more reviewer

@buyaa-nbuyaa-nbuyaa-n left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Infrastructure-librarieslinkable-frameworkIssues associated with delivering a linker friendly framework

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Building linker tests spawns hundreds of NuGet out-of-process restore executables


8 participants

@joperezr@ViktorHofer@eerhardt@marek-safar@radical@lewing@safern@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Adding browser-wasm trimming test leg - #48429
Merged
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Feb 27, 2021
Merged
Adding browser-wasm trimming test leg#48429
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Conversation
@joperezr
@joperezrjoperezr
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Member
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.
@ghost
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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:-
@eerhardteerhardt
added
the 
linkable-framework
Issues associated with delivering a linker friendly framework
label
Feb 18, 2021
@ghost
ghost
commented
Feb 18, 2021

Copy link


Copy Markdown

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:-
marek-safar
marek-safar
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ViktorHofer
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
Curious, what was the overall reason to not import the msbuild files from the repository?
eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@joperezr
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
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
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@eerhardteerhardtFeb 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Should this be $(MicrosoftNetCoreAppRuntimePackDir) instead?
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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
eerhardt
commented
Feb 19, 2021

Copy link


Copy Markdown

Member
@joperezr - is there a reason CI isn't running on this change?
@joperezr
joperezr
commented
Feb 19, 2021

Copy link


Copy Markdown

MemberAuthor
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.

This was referenced Feb 19, 2021

Closed

 Merged

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/trimmingTests.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 24, 2021

Closed

ViktorHofer
ViktorHofer
reviewed
Feb 24, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 25, 2021

Closed

@marek-safar
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
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
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
@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
eerhardt
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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> 
 <ImportProject="..\..\libraries\Directory.Build.props" /> 
 </Project> 
runtime/src/tests/FunctionalTests/Directory.Build.targets

Lines 1 to 3
in
afa7e27
 <Project> 
 <ImportProject="..\..\libraries\Directory.Build.targets" /> 
 </Project> 
@joperezr
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
 <TargetName="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 -->
 <ResolvedFileToPublishTrimMode="link" /> 
<!-- Don't trim the main assembly.
 TrimMode="" is needed so the root assemblies are correctly identified -->
 <ResolvedFileToPublishTrimMode=""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> 
 <TrimmerRootDescriptorInclude="$(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.
@joperezrjoperezr
mentioned this pull request
Feb 26, 2021

Closed

@radical
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
@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
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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
eerhardt
approved these changes
Feb 26, 2021
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This looks good to me. Thanks for the good work here, @joperezr!
eerhardt
eerhardt
reviewed
Feb 27, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ghost
ghost
commented
Feb 27, 2021

Copy link


Copy Markdown

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.
@ghost

ghost

merged commit 76c2ac0
into
dotnet:masterFeb 27, 2021
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2021
@joperezr
joperezr
deleted the

TrimmingWasmLeg

branch
August 11, 2021 20:59

This pull request was closed.

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@lewinglewinglewing left review comments
@marek-safarmarek-safarmarek-safar left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@eerhardteerhardteerhardt approved these changes
@safernsafernsafern left review comments

+1 more reviewer

@buyaa-nbuyaa-nbuyaa-n left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Infrastructure-librarieslinkable-frameworkIssues associated with delivering a linker friendly framework

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Building linker tests spawns hundreds of NuGet out-of-process restore executables


8 participants

@joperezr@ViktorHofer@eerhardt@marek-safar@radical@lewing@safern@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Adding browser-wasm trimming test leg - #48429
Merged
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Feb 27, 2021
Merged
Adding browser-wasm trimming test leg#48429
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Conversation
@joperezr
@joperezrjoperezr
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Member
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.
@ghost
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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:-
@eerhardteerhardt
added
the 
linkable-framework
Issues associated with delivering a linker friendly framework
label
Feb 18, 2021
@ghost
ghost
commented
Feb 18, 2021

Copy link


Copy Markdown

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:-
marek-safar
marek-safar
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ViktorHofer
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
Curious, what was the overall reason to not import the msbuild files from the repository?
eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@joperezr
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
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
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@eerhardteerhardtFeb 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Should this be $(MicrosoftNetCoreAppRuntimePackDir) instead?
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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
eerhardt
commented
Feb 19, 2021

Copy link


Copy Markdown

Member
@joperezr - is there a reason CI isn't running on this change?
@joperezr
joperezr
commented
Feb 19, 2021

Copy link


Copy Markdown

MemberAuthor
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.

This was referenced Feb 19, 2021

Closed

 Merged

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/trimmingTests.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 24, 2021

Closed

ViktorHofer
ViktorHofer
reviewed
Feb 24, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 25, 2021

Closed

@marek-safar
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
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
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
@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
eerhardt
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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> 
 <ImportProject="..\..\libraries\Directory.Build.props" /> 
 </Project> 
runtime/src/tests/FunctionalTests/Directory.Build.targets

Lines 1 to 3
in
afa7e27
 <Project> 
 <ImportProject="..\..\libraries\Directory.Build.targets" /> 
 </Project> 
@joperezr
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
 <TargetName="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 -->
 <ResolvedFileToPublishTrimMode="link" /> 
<!-- Don't trim the main assembly.
 TrimMode="" is needed so the root assemblies are correctly identified -->
 <ResolvedFileToPublishTrimMode=""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> 
 <TrimmerRootDescriptorInclude="$(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.
@joperezrjoperezr
mentioned this pull request
Feb 26, 2021

Closed

@radical
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
@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
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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
eerhardt
approved these changes
Feb 26, 2021
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This looks good to me. Thanks for the good work here, @joperezr!
eerhardt
eerhardt
reviewed
Feb 27, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ghost
ghost
commented
Feb 27, 2021

Copy link


Copy Markdown

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.
@ghost

ghost

merged commit 76c2ac0
into
dotnet:masterFeb 27, 2021
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2021
@joperezr
joperezr
deleted the

TrimmingWasmLeg

branch
August 11, 2021 20:59

This pull request was closed.

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@lewinglewinglewing left review comments
@marek-safarmarek-safarmarek-safar left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@eerhardteerhardteerhardt approved these changes
@safernsafernsafern left review comments

+1 more reviewer

@buyaa-nbuyaa-nbuyaa-n left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Infrastructure-librarieslinkable-frameworkIssues associated with delivering a linker friendly framework

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Building linker tests spawns hundreds of NuGet out-of-process restore executables


8 participants

@joperezr@ViktorHofer@eerhardt@marek-safar@radical@lewing@safern@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Adding browser-wasm trimming test leg - #48429
Merged
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Feb 27, 2021
Merged
Adding browser-wasm trimming test leg#48429
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Conversation
@joperezr
@joperezrjoperezr
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Member
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.
@ghost
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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:-
@eerhardteerhardt
added
the 
linkable-framework
Issues associated with delivering a linker friendly framework
label
Feb 18, 2021
@ghost
ghost
commented
Feb 18, 2021

Copy link


Copy Markdown

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:-
marek-safar
marek-safar
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ViktorHofer
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
Curious, what was the overall reason to not import the msbuild files from the repository?
eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@joperezr
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
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
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@eerhardteerhardtFeb 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Should this be $(MicrosoftNetCoreAppRuntimePackDir) instead?
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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
eerhardt
commented
Feb 19, 2021

Copy link


Copy Markdown

Member
@joperezr - is there a reason CI isn't running on this change?
@joperezr
joperezr
commented
Feb 19, 2021

Copy link


Copy Markdown

MemberAuthor
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.

This was referenced Feb 19, 2021

Closed

 Merged

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/trimmingTests.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 24, 2021

Closed

ViktorHofer
ViktorHofer
reviewed
Feb 24, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 25, 2021

Closed

@marek-safar
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
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
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
@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
eerhardt
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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> 
 <ImportProject="..\..\libraries\Directory.Build.props" /> 
 </Project> 
runtime/src/tests/FunctionalTests/Directory.Build.targets

Lines 1 to 3
in
afa7e27
 <Project> 
 <ImportProject="..\..\libraries\Directory.Build.targets" /> 
 </Project> 
@joperezr
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
 <TargetName="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 -->
 <ResolvedFileToPublishTrimMode="link" /> 
<!-- Don't trim the main assembly.
 TrimMode="" is needed so the root assemblies are correctly identified -->
 <ResolvedFileToPublishTrimMode=""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> 
 <TrimmerRootDescriptorInclude="$(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.
@joperezrjoperezr
mentioned this pull request
Feb 26, 2021

Closed

@radical
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
@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
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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
eerhardt
approved these changes
Feb 26, 2021
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This looks good to me. Thanks for the good work here, @joperezr!
eerhardt
eerhardt
reviewed
Feb 27, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ghost
ghost
commented
Feb 27, 2021

Copy link


Copy Markdown

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.
@ghost

ghost

merged commit 76c2ac0
into
dotnet:masterFeb 27, 2021
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2021
@joperezr
joperezr
deleted the

TrimmingWasmLeg

branch
August 11, 2021 20:59

This pull request was closed.

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@lewinglewinglewing left review comments
@marek-safarmarek-safarmarek-safar left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@eerhardteerhardteerhardt approved these changes
@safernsafernsafern left review comments

+1 more reviewer

@buyaa-nbuyaa-nbuyaa-n left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Infrastructure-librarieslinkable-frameworkIssues associated with delivering a linker friendly framework

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Building linker tests spawns hundreds of NuGet out-of-process restore executables


8 participants

@joperezr@ViktorHofer@eerhardt@marek-safar@radical@lewing@safern@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Adding browser-wasm trimming test leg - #48429
Merged
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Feb 27, 2021
Merged
Adding browser-wasm trimming test leg#48429
13 commits merged into
dotnet:masterfrom 
joperezr:TrimmingWasmLeg
Conversation
@joperezr
@joperezrjoperezr
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Member
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.
@ghost
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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
ghost
commented
Feb 17, 2021

Copy link


Copy Markdown

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:-
@eerhardteerhardt
added
the 
linkable-framework
Issues associated with delivering a linker friendly framework
label
Feb 18, 2021
@ghost
ghost
commented
Feb 18, 2021

Copy link


Copy Markdown

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:-
marek-safar
marek-safar
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ViktorHofer
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
Curious, what was the overall reason to not import the msbuild files from the repository?
eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

eerhardt
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@joperezr
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
ViktorHofer
commented
Feb 18, 2021

Copy link


Copy Markdown

Member
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
joperezr
commented
Feb 18, 2021

Copy link


Copy Markdown

MemberAuthor
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
eerhardt
reviewed
Feb 18, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@eerhardteerhardtFeb 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Should this be $(MicrosoftNetCoreAppRuntimePackDir) instead?
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@joperezrjoperezrFeb 18, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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
eerhardt
commented
Feb 19, 2021

Copy link


Copy Markdown

Member
@joperezr - is there a reason CI isn't running on this change?
@joperezr
joperezr
commented
Feb 19, 2021

Copy link


Copy Markdown

MemberAuthor
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.

This was referenced Feb 19, 2021

Closed

 Merged

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

eerhardt
eerhardt
reviewed
Feb 19, 2021
View reviewed changes
Comment threadeng/testing/linker/trimmingTests.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 24, 2021

Closed

ViktorHofer
ViktorHofer
reviewed
Feb 24, 2021
View reviewed changes
Comment threadeng/testing/linker/SupportFiles/Directory.Build.targets
Outdated

@runfoapprunfoappBot
mentioned this pull request
Feb 25, 2021

Closed

@marek-safar
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
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
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
marek-safar
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
@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
eerhardt
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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> 
 <ImportProject="..\..\libraries\Directory.Build.props" /> 
 </Project> 
runtime/src/tests/FunctionalTests/Directory.Build.targets

Lines 1 to 3
in
afa7e27
 <Project> 
 <ImportProject="..\..\libraries\Directory.Build.targets" /> 
 </Project> 
@joperezr
joperezr
commented
Feb 26, 2021

Copy link


Copy Markdown

MemberAuthor
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
 <TargetName="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 -->
 <ResolvedFileToPublishTrimMode="link" /> 
<!-- Don't trim the main assembly.
 TrimMode="" is needed so the root assemblies are correctly identified -->
 <ResolvedFileToPublishTrimMode=""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> 
 <TrimmerRootDescriptorInclude="$(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.
@joperezrjoperezr
mentioned this pull request
Feb 26, 2021

Closed

@radical
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
@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
radical
commented
Feb 26, 2021

Copy link


Copy Markdown

Member
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
eerhardt
approved these changes
Feb 26, 2021
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This looks good to me. Thanks for the good work here, @joperezr!
eerhardt
eerhardt
reviewed
Feb 27, 2021
View reviewed changes
Comment threadeng/testing/linker/project.csproj.template
Outdated

@ghost
ghost
commented
Feb 27, 2021

Copy link


Copy Markdown

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.
@ghost

ghost

merged commit 76c2ac0
into
dotnet:masterFeb 27, 2021
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2021
@joperezr
joperezr
deleted the

TrimmingWasmLeg

branch
August 11, 2021 20:59

This pull request was closed.

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@lewinglewinglewing left review comments
@marek-safarmarek-safarmarek-safar left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@eerhardteerhardteerhardt approved these changes
@safernsafernsafern left review comments

+1 more reviewer

@buyaa-nbuyaa-nbuyaa-n left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Infrastructure-librarieslinkable-frameworkIssues associated with delivering a linker friendly framework

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Building linker tests spawns hundreds of NuGet out-of-process restore executables


8 participants

@joperezr@ViktorHofer@eerhardt@marek-safar@radical@lewing@safern@buyaa-n