Shift Most of Wasm AOT test build to helix - #48226

Merged
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix
Apr 22, 2021
Merged

Shift Most of Wasm AOT test build to helix#48226
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix

Conversation

@steveisok

@steveisoksteveisok commented Feb 12, 2021

Copy link
Copy Markdown
Member

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

This is done by:

  1. building the test assemblies on the build machine
    • the wasm part of the build is not executed on the build machine,
      because it has the AOT build part
  2. Zip up the test assembly+friends, and any bits required to run the wasm
    app build for that on helix (eg. emsdk, wasm app targets, cross compiler etc)
  3. Send all this to helix, and use a custom aot-build.proj
    • which recreates all the build inputs for the WasmBuildApp target
      using the paths for the assets on helix
    • then we can run WasmBuildApp for the build, resulting in a wasm app
      bundle.
  4. Run the tests!
  • We already have the bits required for building wasm apps on helix, supported
    for Wasm.Build.Tests, which we can use here too.

Trimming:

  • Since, AOT can be so expensive, we use EnableAggressiveTrimming=true(EAT), but
    that means that we could have issues due to trimming.

  • And it can sometimes be unclear whether the build/test failures are due to trimming
    or AOT.

  • Because these builds+test runs are different from other builds, owing to the
    "build partially on helix" step, a normal EAT build would not be the same as

  • to help with testing this, we add two lanes to runtime-staging:

    • *_Mono_AOT: builds AOT+EAT on helix
    • *_Mono_EAT: builds EAT, on helix
      • this is required because we want to run almost the same kinda
        build: 1. build test assembly; 2. send to helix; 3. build wasm app; 4. run tests
  • This should effectively mean that we can see which errors might be due to EAT, and
    which are clearly because of EAT+AOT.

@ghost

Copy link
Copy Markdown

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

Issue Details

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

Author:steveisok
Assignees:-
Labels:

area-Infrastructure-mono

Milestone:-

Base automatically changed from master to mainMarch 1, 2021 09:07
Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

Some these need to have values different from what UseDefaultBlazorWasmFeatureSwitches wants:

  • DebuggerSupport - this is needed by some library tests because they depend on pdbs, or debugger attributes
    • Currently, I am setting this in the specific projects that need it
  • UseSystemResourceKeys - IIUC, this is needed so that the resources don't get trimmed, which is needed by some library tests

Setting these only for some library projects makes it kinda inconsistent. And I'm wondering if that affects what the intention of UseDefaultBlazorWasmFeatureSwitches is.
So, do we need switch at all? If so, then what should be done here?

/cc @lewing@akoeplinger

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

@steveisok@mdh1418 Do you know why this change was made? main is still using the older version.

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

…LoadContext
The test project explicitly copies, and needs `mscorlib.dll`, which gets
trimmed out with `EnableAggressiveTrimming=true`. Preserve that.
Fails because it can't find `xunit.runner.utility.netcoreapp10`
`InlineDataDiscoverer`, and `MemberDataDiscoverer` were getting
trimmed, which would cause some tests to get skipped. For example, ~9k
tests in `System.Data.Common.Tests`
@radical
radicalforce-pushed the build-wasm-aot-helix branch from 88b8234 to f33e181CompareApril 16, 2021 10:31
@radical

Copy link
Copy Markdown
Member

Failing test in runtime (Libraries Test Run release mono Linux x64 Debug): #51588

Failing test in Libraries Test Run release coreclr Linux x64 Debug is possibly: #48658

Failing test in Libraries Test Run release mono OSX x64 Debug: #51611

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

</ItemGroup>

<!-- To recreate the original project on helix, we need to set the wasm properties also, same as the
library test project. Eg. $(InvariantGlobalization) -->

@akoeplingerakoeplingerApr 21, 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.

do we have a way of knowing the list of properties is complete or when we need to update it?

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.

Um .. not really. So, when something breaks? I didn't add all the properties either, because I want to do that as it comes up.

<PropertyGroup>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(TargetOS)' == 'Browser'">true</DebuggerSupport>

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.

curious why does this need .pdb?

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.

Eg:

 fail: [FAIL] System.Collections.Tests.HashtableTests.DebuggerAttribute
info: System.InvalidOperationException : Expected one DebuggerDisplayAttribute on System.Collections.Hashtable.
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerDisplayReferences(Object obj)
info: at System.Collections.Tests.HashtableTests.DebuggerAttribute()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
fail: [FAIL] System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException
info: System.InvalidOperationException : Expected one DebuggerTypeProxyAttribute on System.Collections.Queue.
info: at System.Diagnostics.DebuggerAttributes.GetProxyType(Type type, Type[] genericTypeArguments)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Type[] genericTypeArguments, Object obj)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Object obj)
info: at System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

@@ -0,0 +1,55 @@
// Licensed to the .NET Foundation under one or more agreements.

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 should ideally be added to

<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmAppBuilder\WasmAppBuilder.csproj"
Condition="'$(TargetOS)' != 'Browser'" />
<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmBuildTasks\WasmBuildTasks.csproj"
Condition="'$(TargetOS)' != 'Browser'" />

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.

I'm not sure I understand. This file will get built with WasmBuildTasks. What should be added to tasks.proj?

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

looks green to me, there is some minor whitespace damage but this was a long road so lets land it.

@radical
radical merged commit 6ca3d91 into dotnet:mainApr 22, 2021
@karelzkarelz added this to the 6.0.0 milestone May 20, 2021
@ghostghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@steveisok@mdh1418@radical@SamMonoRT@naricc@lewing@akoeplinger@karelz@ViktorHofer@marek-safar
, '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

Shift Most of Wasm AOT test build to helix - #48226

Merged
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix
Apr 22, 2021
Merged

Shift Most of Wasm AOT test build to helix#48226
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix

Conversation

@steveisok

@steveisoksteveisok commented Feb 12, 2021

Copy link
Copy Markdown
Member

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

This is done by:

  1. building the test assemblies on the build machine
    • the wasm part of the build is not executed on the build machine,
      because it has the AOT build part
  2. Zip up the test assembly+friends, and any bits required to run the wasm
    app build for that on helix (eg. emsdk, wasm app targets, cross compiler etc)
  3. Send all this to helix, and use a custom aot-build.proj
    • which recreates all the build inputs for the WasmBuildApp target
      using the paths for the assets on helix
    • then we can run WasmBuildApp for the build, resulting in a wasm app
      bundle.
  4. Run the tests!
  • We already have the bits required for building wasm apps on helix, supported
    for Wasm.Build.Tests, which we can use here too.

Trimming:

  • Since, AOT can be so expensive, we use EnableAggressiveTrimming=true(EAT), but
    that means that we could have issues due to trimming.

  • And it can sometimes be unclear whether the build/test failures are due to trimming
    or AOT.

  • Because these builds+test runs are different from other builds, owing to the
    "build partially on helix" step, a normal EAT build would not be the same as

  • to help with testing this, we add two lanes to runtime-staging:

    • *_Mono_AOT: builds AOT+EAT on helix
    • *_Mono_EAT: builds EAT, on helix
      • this is required because we want to run almost the same kinda
        build: 1. build test assembly; 2. send to helix; 3. build wasm app; 4. run tests
  • This should effectively mean that we can see which errors might be due to EAT, and
    which are clearly because of EAT+AOT.

@ghost

Copy link
Copy Markdown

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

Issue Details

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

Author:steveisok
Assignees:-
Labels:

area-Infrastructure-mono

Milestone:-

Base automatically changed from master to mainMarch 1, 2021 09:07
Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

Some these need to have values different from what UseDefaultBlazorWasmFeatureSwitches wants:

  • DebuggerSupport - this is needed by some library tests because they depend on pdbs, or debugger attributes
    • Currently, I am setting this in the specific projects that need it
  • UseSystemResourceKeys - IIUC, this is needed so that the resources don't get trimmed, which is needed by some library tests

Setting these only for some library projects makes it kinda inconsistent. And I'm wondering if that affects what the intention of UseDefaultBlazorWasmFeatureSwitches is.
So, do we need switch at all? If so, then what should be done here?

/cc @lewing@akoeplinger

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

@steveisok@mdh1418 Do you know why this change was made? main is still using the older version.

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

…LoadContext
The test project explicitly copies, and needs `mscorlib.dll`, which gets
trimmed out with `EnableAggressiveTrimming=true`. Preserve that.
Fails because it can't find `xunit.runner.utility.netcoreapp10`
`InlineDataDiscoverer`, and `MemberDataDiscoverer` were getting
trimmed, which would cause some tests to get skipped. For example, ~9k
tests in `System.Data.Common.Tests`
@radical
radicalforce-pushed the build-wasm-aot-helix branch from 88b8234 to f33e181CompareApril 16, 2021 10:31
@radical

Copy link
Copy Markdown
Member

Failing test in runtime (Libraries Test Run release mono Linux x64 Debug): #51588

Failing test in Libraries Test Run release coreclr Linux x64 Debug is possibly: #48658

Failing test in Libraries Test Run release mono OSX x64 Debug: #51611

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

</ItemGroup>

<!-- To recreate the original project on helix, we need to set the wasm properties also, same as the
library test project. Eg. $(InvariantGlobalization) -->

@akoeplingerakoeplingerApr 21, 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.

do we have a way of knowing the list of properties is complete or when we need to update it?

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.

Um .. not really. So, when something breaks? I didn't add all the properties either, because I want to do that as it comes up.

<PropertyGroup>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(TargetOS)' == 'Browser'">true</DebuggerSupport>

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.

curious why does this need .pdb?

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.

Eg:

 fail: [FAIL] System.Collections.Tests.HashtableTests.DebuggerAttribute
info: System.InvalidOperationException : Expected one DebuggerDisplayAttribute on System.Collections.Hashtable.
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerDisplayReferences(Object obj)
info: at System.Collections.Tests.HashtableTests.DebuggerAttribute()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
fail: [FAIL] System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException
info: System.InvalidOperationException : Expected one DebuggerTypeProxyAttribute on System.Collections.Queue.
info: at System.Diagnostics.DebuggerAttributes.GetProxyType(Type type, Type[] genericTypeArguments)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Type[] genericTypeArguments, Object obj)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Object obj)
info: at System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

@@ -0,0 +1,55 @@
// Licensed to the .NET Foundation under one or more agreements.

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 should ideally be added to

<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmAppBuilder\WasmAppBuilder.csproj"
Condition="'$(TargetOS)' != 'Browser'" />
<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmBuildTasks\WasmBuildTasks.csproj"
Condition="'$(TargetOS)' != 'Browser'" />

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.

I'm not sure I understand. This file will get built with WasmBuildTasks. What should be added to tasks.proj?

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

looks green to me, there is some minor whitespace damage but this was a long road so lets land it.

@radical
radical merged commit 6ca3d91 into dotnet:mainApr 22, 2021
@karelzkarelz added this to the 6.0.0 milestone May 20, 2021
@ghostghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@steveisok@mdh1418@radical@SamMonoRT@naricc@lewing@akoeplinger@karelz@ViktorHofer@marek-safar
, '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

Shift Most of Wasm AOT test build to helix - #48226

Merged
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix
Apr 22, 2021
Merged

Shift Most of Wasm AOT test build to helix#48226
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix

Conversation

@steveisok

@steveisoksteveisok commented Feb 12, 2021

Copy link
Copy Markdown
Member

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

This is done by:

  1. building the test assemblies on the build machine
    • the wasm part of the build is not executed on the build machine,
      because it has the AOT build part
  2. Zip up the test assembly+friends, and any bits required to run the wasm
    app build for that on helix (eg. emsdk, wasm app targets, cross compiler etc)
  3. Send all this to helix, and use a custom aot-build.proj
    • which recreates all the build inputs for the WasmBuildApp target
      using the paths for the assets on helix
    • then we can run WasmBuildApp for the build, resulting in a wasm app
      bundle.
  4. Run the tests!
  • We already have the bits required for building wasm apps on helix, supported
    for Wasm.Build.Tests, which we can use here too.

Trimming:

  • Since, AOT can be so expensive, we use EnableAggressiveTrimming=true(EAT), but
    that means that we could have issues due to trimming.

  • And it can sometimes be unclear whether the build/test failures are due to trimming
    or AOT.

  • Because these builds+test runs are different from other builds, owing to the
    "build partially on helix" step, a normal EAT build would not be the same as

  • to help with testing this, we add two lanes to runtime-staging:

    • *_Mono_AOT: builds AOT+EAT on helix
    • *_Mono_EAT: builds EAT, on helix
      • this is required because we want to run almost the same kinda
        build: 1. build test assembly; 2. send to helix; 3. build wasm app; 4. run tests
  • This should effectively mean that we can see which errors might be due to EAT, and
    which are clearly because of EAT+AOT.

@ghost

Copy link
Copy Markdown

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

Issue Details

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

Author:steveisok
Assignees:-
Labels:

area-Infrastructure-mono

Milestone:-

Base automatically changed from master to mainMarch 1, 2021 09:07
Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

Some these need to have values different from what UseDefaultBlazorWasmFeatureSwitches wants:

  • DebuggerSupport - this is needed by some library tests because they depend on pdbs, or debugger attributes
    • Currently, I am setting this in the specific projects that need it
  • UseSystemResourceKeys - IIUC, this is needed so that the resources don't get trimmed, which is needed by some library tests

Setting these only for some library projects makes it kinda inconsistent. And I'm wondering if that affects what the intention of UseDefaultBlazorWasmFeatureSwitches is.
So, do we need switch at all? If so, then what should be done here?

/cc @lewing@akoeplinger

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

@steveisok@mdh1418 Do you know why this change was made? main is still using the older version.

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

…LoadContext
The test project explicitly copies, and needs `mscorlib.dll`, which gets
trimmed out with `EnableAggressiveTrimming=true`. Preserve that.
Fails because it can't find `xunit.runner.utility.netcoreapp10`
`InlineDataDiscoverer`, and `MemberDataDiscoverer` were getting
trimmed, which would cause some tests to get skipped. For example, ~9k
tests in `System.Data.Common.Tests`
@radical
radicalforce-pushed the build-wasm-aot-helix branch from 88b8234 to f33e181CompareApril 16, 2021 10:31
@radical

Copy link
Copy Markdown
Member

Failing test in runtime (Libraries Test Run release mono Linux x64 Debug): #51588

Failing test in Libraries Test Run release coreclr Linux x64 Debug is possibly: #48658

Failing test in Libraries Test Run release mono OSX x64 Debug: #51611

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

</ItemGroup>

<!-- To recreate the original project on helix, we need to set the wasm properties also, same as the
library test project. Eg. $(InvariantGlobalization) -->

@akoeplingerakoeplingerApr 21, 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.

do we have a way of knowing the list of properties is complete or when we need to update it?

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.

Um .. not really. So, when something breaks? I didn't add all the properties either, because I want to do that as it comes up.

<PropertyGroup>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(TargetOS)' == 'Browser'">true</DebuggerSupport>

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.

curious why does this need .pdb?

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.

Eg:

 fail: [FAIL] System.Collections.Tests.HashtableTests.DebuggerAttribute
info: System.InvalidOperationException : Expected one DebuggerDisplayAttribute on System.Collections.Hashtable.
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerDisplayReferences(Object obj)
info: at System.Collections.Tests.HashtableTests.DebuggerAttribute()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
fail: [FAIL] System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException
info: System.InvalidOperationException : Expected one DebuggerTypeProxyAttribute on System.Collections.Queue.
info: at System.Diagnostics.DebuggerAttributes.GetProxyType(Type type, Type[] genericTypeArguments)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Type[] genericTypeArguments, Object obj)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Object obj)
info: at System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

@@ -0,0 +1,55 @@
// Licensed to the .NET Foundation under one or more agreements.

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 should ideally be added to

<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmAppBuilder\WasmAppBuilder.csproj"
Condition="'$(TargetOS)' != 'Browser'" />
<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmBuildTasks\WasmBuildTasks.csproj"
Condition="'$(TargetOS)' != 'Browser'" />

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.

I'm not sure I understand. This file will get built with WasmBuildTasks. What should be added to tasks.proj?

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

looks green to me, there is some minor whitespace damage but this was a long road so lets land it.

@radical
radical merged commit 6ca3d91 into dotnet:mainApr 22, 2021
@karelzkarelz added this to the 6.0.0 milestone May 20, 2021
@ghostghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@steveisok@mdh1418@radical@SamMonoRT@naricc@lewing@akoeplinger@karelz@ViktorHofer@marek-safar
, '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

Shift Most of Wasm AOT test build to helix - #48226

Merged
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix
Apr 22, 2021
Merged

Shift Most of Wasm AOT test build to helix#48226
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix

Conversation

@steveisok

@steveisoksteveisok commented Feb 12, 2021

Copy link
Copy Markdown
Member

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

This is done by:

  1. building the test assemblies on the build machine
    • the wasm part of the build is not executed on the build machine,
      because it has the AOT build part
  2. Zip up the test assembly+friends, and any bits required to run the wasm
    app build for that on helix (eg. emsdk, wasm app targets, cross compiler etc)
  3. Send all this to helix, and use a custom aot-build.proj
    • which recreates all the build inputs for the WasmBuildApp target
      using the paths for the assets on helix
    • then we can run WasmBuildApp for the build, resulting in a wasm app
      bundle.
  4. Run the tests!
  • We already have the bits required for building wasm apps on helix, supported
    for Wasm.Build.Tests, which we can use here too.

Trimming:

  • Since, AOT can be so expensive, we use EnableAggressiveTrimming=true(EAT), but
    that means that we could have issues due to trimming.

  • And it can sometimes be unclear whether the build/test failures are due to trimming
    or AOT.

  • Because these builds+test runs are different from other builds, owing to the
    "build partially on helix" step, a normal EAT build would not be the same as

  • to help with testing this, we add two lanes to runtime-staging:

    • *_Mono_AOT: builds AOT+EAT on helix
    • *_Mono_EAT: builds EAT, on helix
      • this is required because we want to run almost the same kinda
        build: 1. build test assembly; 2. send to helix; 3. build wasm app; 4. run tests
  • This should effectively mean that we can see which errors might be due to EAT, and
    which are clearly because of EAT+AOT.

@ghost

Copy link
Copy Markdown

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

Issue Details

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

Author:steveisok
Assignees:-
Labels:

area-Infrastructure-mono

Milestone:-

Base automatically changed from master to mainMarch 1, 2021 09:07
Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

Some these need to have values different from what UseDefaultBlazorWasmFeatureSwitches wants:

  • DebuggerSupport - this is needed by some library tests because they depend on pdbs, or debugger attributes
    • Currently, I am setting this in the specific projects that need it
  • UseSystemResourceKeys - IIUC, this is needed so that the resources don't get trimmed, which is needed by some library tests

Setting these only for some library projects makes it kinda inconsistent. And I'm wondering if that affects what the intention of UseDefaultBlazorWasmFeatureSwitches is.
So, do we need switch at all? If so, then what should be done here?

/cc @lewing@akoeplinger

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

@steveisok@mdh1418 Do you know why this change was made? main is still using the older version.

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

…LoadContext
The test project explicitly copies, and needs `mscorlib.dll`, which gets
trimmed out with `EnableAggressiveTrimming=true`. Preserve that.
Fails because it can't find `xunit.runner.utility.netcoreapp10`
`InlineDataDiscoverer`, and `MemberDataDiscoverer` were getting
trimmed, which would cause some tests to get skipped. For example, ~9k
tests in `System.Data.Common.Tests`
@radical
radicalforce-pushed the build-wasm-aot-helix branch from 88b8234 to f33e181CompareApril 16, 2021 10:31
@radical

Copy link
Copy Markdown
Member

Failing test in runtime (Libraries Test Run release mono Linux x64 Debug): #51588

Failing test in Libraries Test Run release coreclr Linux x64 Debug is possibly: #48658

Failing test in Libraries Test Run release mono OSX x64 Debug: #51611

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

</ItemGroup>

<!-- To recreate the original project on helix, we need to set the wasm properties also, same as the
library test project. Eg. $(InvariantGlobalization) -->

@akoeplingerakoeplingerApr 21, 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.

do we have a way of knowing the list of properties is complete or when we need to update it?

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.

Um .. not really. So, when something breaks? I didn't add all the properties either, because I want to do that as it comes up.

<PropertyGroup>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(TargetOS)' == 'Browser'">true</DebuggerSupport>

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.

curious why does this need .pdb?

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.

Eg:

 fail: [FAIL] System.Collections.Tests.HashtableTests.DebuggerAttribute
info: System.InvalidOperationException : Expected one DebuggerDisplayAttribute on System.Collections.Hashtable.
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerDisplayReferences(Object obj)
info: at System.Collections.Tests.HashtableTests.DebuggerAttribute()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
fail: [FAIL] System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException
info: System.InvalidOperationException : Expected one DebuggerTypeProxyAttribute on System.Collections.Queue.
info: at System.Diagnostics.DebuggerAttributes.GetProxyType(Type type, Type[] genericTypeArguments)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Type[] genericTypeArguments, Object obj)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Object obj)
info: at System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

@@ -0,0 +1,55 @@
// Licensed to the .NET Foundation under one or more agreements.

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 should ideally be added to

<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmAppBuilder\WasmAppBuilder.csproj"
Condition="'$(TargetOS)' != 'Browser'" />
<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmBuildTasks\WasmBuildTasks.csproj"
Condition="'$(TargetOS)' != 'Browser'" />

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.

I'm not sure I understand. This file will get built with WasmBuildTasks. What should be added to tasks.proj?

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

looks green to me, there is some minor whitespace damage but this was a long road so lets land it.

@radical
radical merged commit 6ca3d91 into dotnet:mainApr 22, 2021
@karelzkarelz added this to the 6.0.0 milestone May 20, 2021
@ghostghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@steveisok@mdh1418@radical@SamMonoRT@naricc@lewing@akoeplinger@karelz@ViktorHofer@marek-safar
, '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

Shift Most of Wasm AOT test build to helix - #48226

Merged
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix
Apr 22, 2021
Merged

Shift Most of Wasm AOT test build to helix#48226
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix

Conversation

@steveisok

@steveisoksteveisok commented Feb 12, 2021

Copy link
Copy Markdown
Member

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

This is done by:

  1. building the test assemblies on the build machine
    • the wasm part of the build is not executed on the build machine,
      because it has the AOT build part
  2. Zip up the test assembly+friends, and any bits required to run the wasm
    app build for that on helix (eg. emsdk, wasm app targets, cross compiler etc)
  3. Send all this to helix, and use a custom aot-build.proj
    • which recreates all the build inputs for the WasmBuildApp target
      using the paths for the assets on helix
    • then we can run WasmBuildApp for the build, resulting in a wasm app
      bundle.
  4. Run the tests!
  • We already have the bits required for building wasm apps on helix, supported
    for Wasm.Build.Tests, which we can use here too.

Trimming:

  • Since, AOT can be so expensive, we use EnableAggressiveTrimming=true(EAT), but
    that means that we could have issues due to trimming.

  • And it can sometimes be unclear whether the build/test failures are due to trimming
    or AOT.

  • Because these builds+test runs are different from other builds, owing to the
    "build partially on helix" step, a normal EAT build would not be the same as

  • to help with testing this, we add two lanes to runtime-staging:

    • *_Mono_AOT: builds AOT+EAT on helix
    • *_Mono_EAT: builds EAT, on helix
      • this is required because we want to run almost the same kinda
        build: 1. build test assembly; 2. send to helix; 3. build wasm app; 4. run tests
  • This should effectively mean that we can see which errors might be due to EAT, and
    which are clearly because of EAT+AOT.

@ghost

Copy link
Copy Markdown

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

Issue Details

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

Author:steveisok
Assignees:-
Labels:

area-Infrastructure-mono

Milestone:-

Base automatically changed from master to mainMarch 1, 2021 09:07
Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

Some these need to have values different from what UseDefaultBlazorWasmFeatureSwitches wants:

  • DebuggerSupport - this is needed by some library tests because they depend on pdbs, or debugger attributes
    • Currently, I am setting this in the specific projects that need it
  • UseSystemResourceKeys - IIUC, this is needed so that the resources don't get trimmed, which is needed by some library tests

Setting these only for some library projects makes it kinda inconsistent. And I'm wondering if that affects what the intention of UseDefaultBlazorWasmFeatureSwitches is.
So, do we need switch at all? If so, then what should be done here?

/cc @lewing@akoeplinger

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

@steveisok@mdh1418 Do you know why this change was made? main is still using the older version.

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

…LoadContext
The test project explicitly copies, and needs `mscorlib.dll`, which gets
trimmed out with `EnableAggressiveTrimming=true`. Preserve that.
Fails because it can't find `xunit.runner.utility.netcoreapp10`
`InlineDataDiscoverer`, and `MemberDataDiscoverer` were getting
trimmed, which would cause some tests to get skipped. For example, ~9k
tests in `System.Data.Common.Tests`
@radical
radicalforce-pushed the build-wasm-aot-helix branch from 88b8234 to f33e181CompareApril 16, 2021 10:31
@radical

Copy link
Copy Markdown
Member

Failing test in runtime (Libraries Test Run release mono Linux x64 Debug): #51588

Failing test in Libraries Test Run release coreclr Linux x64 Debug is possibly: #48658

Failing test in Libraries Test Run release mono OSX x64 Debug: #51611

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

</ItemGroup>

<!-- To recreate the original project on helix, we need to set the wasm properties also, same as the
library test project. Eg. $(InvariantGlobalization) -->

@akoeplingerakoeplingerApr 21, 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.

do we have a way of knowing the list of properties is complete or when we need to update it?

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.

Um .. not really. So, when something breaks? I didn't add all the properties either, because I want to do that as it comes up.

<PropertyGroup>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(TargetOS)' == 'Browser'">true</DebuggerSupport>

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.

curious why does this need .pdb?

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.

Eg:

 fail: [FAIL] System.Collections.Tests.HashtableTests.DebuggerAttribute
info: System.InvalidOperationException : Expected one DebuggerDisplayAttribute on System.Collections.Hashtable.
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerDisplayReferences(Object obj)
info: at System.Collections.Tests.HashtableTests.DebuggerAttribute()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
fail: [FAIL] System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException
info: System.InvalidOperationException : Expected one DebuggerTypeProxyAttribute on System.Collections.Queue.
info: at System.Diagnostics.DebuggerAttributes.GetProxyType(Type type, Type[] genericTypeArguments)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Type[] genericTypeArguments, Object obj)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Object obj)
info: at System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

@@ -0,0 +1,55 @@
// Licensed to the .NET Foundation under one or more agreements.

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 should ideally be added to

<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmAppBuilder\WasmAppBuilder.csproj"
Condition="'$(TargetOS)' != 'Browser'" />
<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmBuildTasks\WasmBuildTasks.csproj"
Condition="'$(TargetOS)' != 'Browser'" />

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.

I'm not sure I understand. This file will get built with WasmBuildTasks. What should be added to tasks.proj?

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

looks green to me, there is some minor whitespace damage but this was a long road so lets land it.

@radical
radical merged commit 6ca3d91 into dotnet:mainApr 22, 2021
@karelzkarelz added this to the 6.0.0 milestone May 20, 2021
@ghostghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@steveisok@mdh1418@radical@SamMonoRT@naricc@lewing@akoeplinger@karelz@ViktorHofer@marek-safar
, '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

Shift Most of Wasm AOT test build to helix - #48226

Merged
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix
Apr 22, 2021
Merged

Shift Most of Wasm AOT test build to helix#48226
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix

Conversation

@steveisok

@steveisoksteveisok commented Feb 12, 2021

Copy link
Copy Markdown
Member

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

This is done by:

  1. building the test assemblies on the build machine
    • the wasm part of the build is not executed on the build machine,
      because it has the AOT build part
  2. Zip up the test assembly+friends, and any bits required to run the wasm
    app build for that on helix (eg. emsdk, wasm app targets, cross compiler etc)
  3. Send all this to helix, and use a custom aot-build.proj
    • which recreates all the build inputs for the WasmBuildApp target
      using the paths for the assets on helix
    • then we can run WasmBuildApp for the build, resulting in a wasm app
      bundle.
  4. Run the tests!
  • We already have the bits required for building wasm apps on helix, supported
    for Wasm.Build.Tests, which we can use here too.

Trimming:

  • Since, AOT can be so expensive, we use EnableAggressiveTrimming=true(EAT), but
    that means that we could have issues due to trimming.

  • And it can sometimes be unclear whether the build/test failures are due to trimming
    or AOT.

  • Because these builds+test runs are different from other builds, owing to the
    "build partially on helix" step, a normal EAT build would not be the same as

  • to help with testing this, we add two lanes to runtime-staging:

    • *_Mono_AOT: builds AOT+EAT on helix
    • *_Mono_EAT: builds EAT, on helix
      • this is required because we want to run almost the same kinda
        build: 1. build test assembly; 2. send to helix; 3. build wasm app; 4. run tests
  • This should effectively mean that we can see which errors might be due to EAT, and
    which are clearly because of EAT+AOT.

@ghost

Copy link
Copy Markdown

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

Issue Details

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

Author:steveisok
Assignees:-
Labels:

area-Infrastructure-mono

Milestone:-

Base automatically changed from master to mainMarch 1, 2021 09:07
Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

Some these need to have values different from what UseDefaultBlazorWasmFeatureSwitches wants:

  • DebuggerSupport - this is needed by some library tests because they depend on pdbs, or debugger attributes
    • Currently, I am setting this in the specific projects that need it
  • UseSystemResourceKeys - IIUC, this is needed so that the resources don't get trimmed, which is needed by some library tests

Setting these only for some library projects makes it kinda inconsistent. And I'm wondering if that affects what the intention of UseDefaultBlazorWasmFeatureSwitches is.
So, do we need switch at all? If so, then what should be done here?

/cc @lewing@akoeplinger

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

@steveisok@mdh1418 Do you know why this change was made? main is still using the older version.

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

…LoadContext
The test project explicitly copies, and needs `mscorlib.dll`, which gets
trimmed out with `EnableAggressiveTrimming=true`. Preserve that.
Fails because it can't find `xunit.runner.utility.netcoreapp10`
`InlineDataDiscoverer`, and `MemberDataDiscoverer` were getting
trimmed, which would cause some tests to get skipped. For example, ~9k
tests in `System.Data.Common.Tests`
@radical
radicalforce-pushed the build-wasm-aot-helix branch from 88b8234 to f33e181CompareApril 16, 2021 10:31
@radical

Copy link
Copy Markdown
Member

Failing test in runtime (Libraries Test Run release mono Linux x64 Debug): #51588

Failing test in Libraries Test Run release coreclr Linux x64 Debug is possibly: #48658

Failing test in Libraries Test Run release mono OSX x64 Debug: #51611

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

</ItemGroup>

<!-- To recreate the original project on helix, we need to set the wasm properties also, same as the
library test project. Eg. $(InvariantGlobalization) -->

@akoeplingerakoeplingerApr 21, 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.

do we have a way of knowing the list of properties is complete or when we need to update it?

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.

Um .. not really. So, when something breaks? I didn't add all the properties either, because I want to do that as it comes up.

<PropertyGroup>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(TargetOS)' == 'Browser'">true</DebuggerSupport>

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.

curious why does this need .pdb?

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.

Eg:

 fail: [FAIL] System.Collections.Tests.HashtableTests.DebuggerAttribute
info: System.InvalidOperationException : Expected one DebuggerDisplayAttribute on System.Collections.Hashtable.
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerDisplayReferences(Object obj)
info: at System.Collections.Tests.HashtableTests.DebuggerAttribute()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
fail: [FAIL] System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException
info: System.InvalidOperationException : Expected one DebuggerTypeProxyAttribute on System.Collections.Queue.
info: at System.Diagnostics.DebuggerAttributes.GetProxyType(Type type, Type[] genericTypeArguments)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Type[] genericTypeArguments, Object obj)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Object obj)
info: at System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

@@ -0,0 +1,55 @@
// Licensed to the .NET Foundation under one or more agreements.

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 should ideally be added to

<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmAppBuilder\WasmAppBuilder.csproj"
Condition="'$(TargetOS)' != 'Browser'" />
<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmBuildTasks\WasmBuildTasks.csproj"
Condition="'$(TargetOS)' != 'Browser'" />

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.

I'm not sure I understand. This file will get built with WasmBuildTasks. What should be added to tasks.proj?

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

looks green to me, there is some minor whitespace damage but this was a long road so lets land it.

@radical
radical merged commit 6ca3d91 into dotnet:mainApr 22, 2021
@karelzkarelz added this to the 6.0.0 milestone May 20, 2021
@ghostghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@steveisok@mdh1418@radical@SamMonoRT@naricc@lewing@akoeplinger@karelz@ViktorHofer@marek-safar
, '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

Shift Most of Wasm AOT test build to helix - #48226

Merged
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix
Apr 22, 2021
Merged

Shift Most of Wasm AOT test build to helix#48226
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix

Conversation

@steveisok

@steveisoksteveisok commented Feb 12, 2021

Copy link
Copy Markdown
Member

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

This is done by:

  1. building the test assemblies on the build machine
    • the wasm part of the build is not executed on the build machine,
      because it has the AOT build part
  2. Zip up the test assembly+friends, and any bits required to run the wasm
    app build for that on helix (eg. emsdk, wasm app targets, cross compiler etc)
  3. Send all this to helix, and use a custom aot-build.proj
    • which recreates all the build inputs for the WasmBuildApp target
      using the paths for the assets on helix
    • then we can run WasmBuildApp for the build, resulting in a wasm app
      bundle.
  4. Run the tests!
  • We already have the bits required for building wasm apps on helix, supported
    for Wasm.Build.Tests, which we can use here too.

Trimming:

  • Since, AOT can be so expensive, we use EnableAggressiveTrimming=true(EAT), but
    that means that we could have issues due to trimming.

  • And it can sometimes be unclear whether the build/test failures are due to trimming
    or AOT.

  • Because these builds+test runs are different from other builds, owing to the
    "build partially on helix" step, a normal EAT build would not be the same as

  • to help with testing this, we add two lanes to runtime-staging:

    • *_Mono_AOT: builds AOT+EAT on helix
    • *_Mono_EAT: builds EAT, on helix
      • this is required because we want to run almost the same kinda
        build: 1. build test assembly; 2. send to helix; 3. build wasm app; 4. run tests
  • This should effectively mean that we can see which errors might be due to EAT, and
    which are clearly because of EAT+AOT.

@ghost

Copy link
Copy Markdown

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

Issue Details

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

Author:steveisok
Assignees:-
Labels:

area-Infrastructure-mono

Milestone:-

Base automatically changed from master to mainMarch 1, 2021 09:07
Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

Some these need to have values different from what UseDefaultBlazorWasmFeatureSwitches wants:

  • DebuggerSupport - this is needed by some library tests because they depend on pdbs, or debugger attributes
    • Currently, I am setting this in the specific projects that need it
  • UseSystemResourceKeys - IIUC, this is needed so that the resources don't get trimmed, which is needed by some library tests

Setting these only for some library projects makes it kinda inconsistent. And I'm wondering if that affects what the intention of UseDefaultBlazorWasmFeatureSwitches is.
So, do we need switch at all? If so, then what should be done here?

/cc @lewing@akoeplinger

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

@steveisok@mdh1418 Do you know why this change was made? main is still using the older version.

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

…LoadContext
The test project explicitly copies, and needs `mscorlib.dll`, which gets
trimmed out with `EnableAggressiveTrimming=true`. Preserve that.
Fails because it can't find `xunit.runner.utility.netcoreapp10`
`InlineDataDiscoverer`, and `MemberDataDiscoverer` were getting
trimmed, which would cause some tests to get skipped. For example, ~9k
tests in `System.Data.Common.Tests`
@radical
radicalforce-pushed the build-wasm-aot-helix branch from 88b8234 to f33e181CompareApril 16, 2021 10:31
@radical

Copy link
Copy Markdown
Member

Failing test in runtime (Libraries Test Run release mono Linux x64 Debug): #51588

Failing test in Libraries Test Run release coreclr Linux x64 Debug is possibly: #48658

Failing test in Libraries Test Run release mono OSX x64 Debug: #51611

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

</ItemGroup>

<!-- To recreate the original project on helix, we need to set the wasm properties also, same as the
library test project. Eg. $(InvariantGlobalization) -->

@akoeplingerakoeplingerApr 21, 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.

do we have a way of knowing the list of properties is complete or when we need to update it?

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.

Um .. not really. So, when something breaks? I didn't add all the properties either, because I want to do that as it comes up.

<PropertyGroup>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(TargetOS)' == 'Browser'">true</DebuggerSupport>

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.

curious why does this need .pdb?

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.

Eg:

 fail: [FAIL] System.Collections.Tests.HashtableTests.DebuggerAttribute
info: System.InvalidOperationException : Expected one DebuggerDisplayAttribute on System.Collections.Hashtable.
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerDisplayReferences(Object obj)
info: at System.Collections.Tests.HashtableTests.DebuggerAttribute()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
fail: [FAIL] System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException
info: System.InvalidOperationException : Expected one DebuggerTypeProxyAttribute on System.Collections.Queue.
info: at System.Diagnostics.DebuggerAttributes.GetProxyType(Type type, Type[] genericTypeArguments)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Type[] genericTypeArguments, Object obj)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Object obj)
info: at System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

@@ -0,0 +1,55 @@
// Licensed to the .NET Foundation under one or more agreements.

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 should ideally be added to

<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmAppBuilder\WasmAppBuilder.csproj"
Condition="'$(TargetOS)' != 'Browser'" />
<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmBuildTasks\WasmBuildTasks.csproj"
Condition="'$(TargetOS)' != 'Browser'" />

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.

I'm not sure I understand. This file will get built with WasmBuildTasks. What should be added to tasks.proj?

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

looks green to me, there is some minor whitespace damage but this was a long road so lets land it.

@radical
radical merged commit 6ca3d91 into dotnet:mainApr 22, 2021
@karelzkarelz added this to the 6.0.0 milestone May 20, 2021
@ghostghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@steveisok@mdh1418@radical@SamMonoRT@naricc@lewing@akoeplinger@karelz@ViktorHofer@marek-safar
, '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

Shift Most of Wasm AOT test build to helix - #48226

Merged
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix
Apr 22, 2021
Merged

Shift Most of Wasm AOT test build to helix#48226
radical merged 128 commits into
dotnet:mainfrom
steveisok:build-wasm-aot-helix

Conversation

@steveisok

@steveisoksteveisok commented Feb 12, 2021

Copy link
Copy Markdown
Member

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

This is done by:

  1. building the test assemblies on the build machine
    • the wasm part of the build is not executed on the build machine,
      because it has the AOT build part
  2. Zip up the test assembly+friends, and any bits required to run the wasm
    app build for that on helix (eg. emsdk, wasm app targets, cross compiler etc)
  3. Send all this to helix, and use a custom aot-build.proj
    • which recreates all the build inputs for the WasmBuildApp target
      using the paths for the assets on helix
    • then we can run WasmBuildApp for the build, resulting in a wasm app
      bundle.
  4. Run the tests!
  • We already have the bits required for building wasm apps on helix, supported
    for Wasm.Build.Tests, which we can use here too.

Trimming:

  • Since, AOT can be so expensive, we use EnableAggressiveTrimming=true(EAT), but
    that means that we could have issues due to trimming.

  • And it can sometimes be unclear whether the build/test failures are due to trimming
    or AOT.

  • Because these builds+test runs are different from other builds, owing to the
    "build partially on helix" step, a normal EAT build would not be the same as

  • to help with testing this, we add two lanes to runtime-staging:

    • *_Mono_AOT: builds AOT+EAT on helix
    • *_Mono_EAT: builds EAT, on helix
      • this is required because we want to run almost the same kinda
        build: 1. build test assembly; 2. send to helix; 3. build wasm app; 4. run tests
  • This should effectively mean that we can see which errors might be due to EAT, and
    which are clearly because of EAT+AOT.

@ghost

Copy link
Copy Markdown

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

Issue Details

Since AOT'ing each test suite takes between 3-9 min, we need to shift the burden over to helix.

Author:steveisok
Assignees:-
Labels:

area-Infrastructure-mono

Milestone:-

Base automatically changed from master to mainMarch 1, 2021 09:07
Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

Some these need to have values different from what UseDefaultBlazorWasmFeatureSwitches wants:

  • DebuggerSupport - this is needed by some library tests because they depend on pdbs, or debugger attributes
    • Currently, I am setting this in the specific projects that need it
  • UseSystemResourceKeys - IIUC, this is needed so that the resources don't get trimmed, which is needed by some library tests

Setting these only for some library projects makes it kinda inconsistent. And I'm wondering if that affects what the intention of UseDefaultBlazorWasmFeatureSwitches is.
So, do we need switch at all? If so, then what should be done here?

/cc @lewing@akoeplinger

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

@steveisok@mdh1418 Do you know why this change was made? main is still using the older version.

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

…LoadContext
The test project explicitly copies, and needs `mscorlib.dll`, which gets
trimmed out with `EnableAggressiveTrimming=true`. Preserve that.
Fails because it can't find `xunit.runner.utility.netcoreapp10`
`InlineDataDiscoverer`, and `MemberDataDiscoverer` were getting
trimmed, which would cause some tests to get skipped. For example, ~9k
tests in `System.Data.Common.Tests`
@radical
radicalforce-pushed the build-wasm-aot-helix branch from 88b8234 to f33e181CompareApril 16, 2021 10:31
@radical

Copy link
Copy Markdown
Member

Failing test in runtime (Libraries Test Run release mono Linux x64 Debug): #51588

Failing test in Libraries Test Run release coreclr Linux x64 Debug is possibly: #48658

Failing test in Libraries Test Run release mono OSX x64 Debug: #51611

platform: Browser_wasm
container:
image: ubuntu-18.04-webassembly-20210223133559-4800846
image: ubuntu-18.04-webassembly-20210309143118-005aab4

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.

judging from the date it's to bring in dotnet/dotnet-buildtools-prereqs-docker@f25f6cd

Comment on lines 50 to 58
<PropertyGroup Condition="'$(UseDefaultBlazorWASMFeatureSwitches)' == 'true'">
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<UseSystemResourceKeys>false</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
<HttpActivityPropagationSupport>false</HttpActivityPropagationSupport>

<!-- we want to default to what Blazor has, except if we are building in Debug config -->
<DebuggerSupport Condition="'$(Configuration)' != 'Debug'">false</DebuggerSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' != 'Debug'">false</DebuggerSupport>
</PropertyGroup>

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.

I agree with @radical, the intention of UseDefaultBlazorWASMFeatureSwitches is to make us consistent with Blazor. I think overriding in the individual library test projects where necessary is fine for now.

</ItemGroup>

<!-- To recreate the original project on helix, we need to set the wasm properties also, same as the
library test project. Eg. $(InvariantGlobalization) -->

@akoeplingerakoeplingerApr 21, 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.

do we have a way of knowing the list of properties is complete or when we need to update it?

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.

Um .. not really. So, when something breaks? I didn't add all the properties either, because I want to do that as it comes up.

<PropertyGroup>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(TargetOS)' == 'Browser'">true</DebuggerSupport>

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.

curious why does this need .pdb?

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.

Eg:

 fail: [FAIL] System.Collections.Tests.HashtableTests.DebuggerAttribute
info: System.InvalidOperationException : Expected one DebuggerDisplayAttribute on System.Collections.Hashtable.
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerDisplayReferences(Object obj)
info: at System.Collections.Tests.HashtableTests.DebuggerAttribute()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
fail: [FAIL] System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException
info: System.InvalidOperationException : Expected one DebuggerTypeProxyAttribute on System.Collections.Queue.
info: at System.Diagnostics.DebuggerAttributes.GetProxyType(Type type, Type[] genericTypeArguments)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Type[] genericTypeArguments, Object obj)
info: at System.Diagnostics.DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Type type, Object obj)
info: at System.Collections.Tests.QueueTests.DebuggerAttribute_NullQueue_ThrowsArgumentNullException()
info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

@@ -0,0 +1,55 @@
// Licensed to the .NET Foundation under one or more agreements.

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 should ideally be added to

<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmAppBuilder\WasmAppBuilder.csproj"
Condition="'$(TargetOS)' != 'Browser'" />
<ProjectReferenceRemove="$(MSBuildThisFileDirectory)WasmBuildTasks\WasmBuildTasks.csproj"
Condition="'$(TargetOS)' != 'Browser'" />

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.

I'm not sure I understand. This file will get built with WasmBuildTasks. What should be added to tasks.proj?

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

looks green to me, there is some minor whitespace damage but this was a long road so lets land it.

@radical
radical merged commit 6ca3d91 into dotnet:mainApr 22, 2021
@karelzkarelz added this to the 6.0.0 milestone May 20, 2021
@ghostghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@steveisok@mdh1418@radical@SamMonoRT@naricc@lewing@akoeplinger@karelz@ViktorHofer@marek-safar