Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wasm] Run Wasm.Build.Tests against workloads #54451

Merged
merged 25 commits into from
Jul 14, 2021

Conversation

radical
Copy link
Member

@radical radical commented Jun 19, 2021

Currently we run Wasm.Build.Tests on helix, by setting various paths to reference files from artifacts. This PR adds another mode, we try to test against workload packs.

For this:

  1. we need the nugets for the various workload packs, and the manifest
  2. manifest id, path to those nugets
  3. and a SDK version to test

Steps:

  1. The specified SDK is installed in artifacts/bin/dotnet-workload
  2. the specified manifest is installed from the built nugets, and any depends-on manifests in the json.
  3. dotnet workload install is used to install the packs, with the built nugets path being used as one the nuget sourcs

Once this setup is done, then the tests are run in an environment such that they use dotnet from above, and try to resolve packs from there.

  • We still want to test without packs, the case of using EMSDK_PATH directly, for example, in the library tests. So, we now run Wasm.Build.Tests for Workloads, and the regular EMSDK_PATH.

  • Two workload specific tests were added in a recent PR ([wasm] Update Wasm.Build.Tests to build with net6.0 #54936)

  • simple test for building blazorwasm template project with AOT

  • a test to validate that the files in UnixFilePermissions.xml for the packs actually exist on disk

.. and these are enabled here.

  • To use this, the project sets TestUsingWorkloads=true, and specifies the workload with:
    <WorkloadIdForTesting Include="wasm-tools"
                           Name="microsoft.net.workload.mono.toolchain"
                           ManifestName="Microsoft.NET.Workload.Mono.ToolChain"
                           Version="$(PackageVersion)"
                           VersionBand="$(SdkBandVersion)" />

Notes:

  • This also fixes using the correct runtime pack based on the version in the manifest json.
  • The sdk version is specified with <SdkVersionForWorkloadTesting>6.0.100-preview.7.21326.4</SdkVersionForWorkloadTesting> in eng/Versions.props
  • Known issues: I'm explicitly invoking Pack on the aotcross pkgproj

TODO:

  • Run the blazorwasm test with playwright
  • Add sdk-with-no-packs case also

@radical radical force-pushed the wasm-native-libs-workloads branch 2 times, most recently from 3a62ae1 to 81373d9 Compare June 22, 2021 07:29
@radical radical force-pushed the wasm-native-libs-workloads branch from 6d43522 to 9165c39 Compare June 26, 2021 01:18
@radical radical changed the title [IGNORE] [wip] wasm native libs, and testing wbt with workloads [wasm] Run Wasm.Build.Tests against workload Jun 28, 2021
@radical radical added the arch-wasm WebAssembly architecture label Jun 29, 2021
@ghost
Copy link

ghost commented Jun 29, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Currently we run Wasm.Build.Tests on helix, by setting various paths to reference files from artifacts. This PR adds another mode, we try to test against workload packs.

For this:

  1. we need the nugets for the various workload packs, and the manifest
  2. manifest id, path to those nugets
  3. and a SDK version to test

The specified SDK is installed in artifacts/bin/dotnet-workload. Then we use InstallWorkload to "install" the packs for this SDK. This does not use dotnet workload install, instead it:

  1. extracts the WorkloadManifest.json from the manifest nuget
  2. Given the manifest id, gets the list of packs
  3. For the packs with nugets in the artifacts dir, it unpacks them into the correct locations
  4. For the remaining ones (Emscripten in this case), it restores them from nuget

Once this setup is done, then the tests are run in an environment such that they use dotnet from above, and try to resolve packs from there.

Known issues:

  1. I wasn't able to build the cross compiler package, so for this PR, I'm copying over the files explicitly

We still want to test without packs, the case of using EMSDK_PATH directly, for example, in the library tests. So, we now run Wasm.Build.Tests for Workloads, and the regular EMSDK_PATH.

Tests:

  • a simple test is added for building blazorwasm template project with AOT
  • a test to validate that the files in UnixFilePermissions.xml for the packs actually exist on disk

Notes:

  • This also fixes using the correct runtime pack based on the version in the manifest json.
  • And it changes, and fixes building test projects with net6.0
  • The sdk version is specified with <SdkVersionForWorkloadTesting>6.0.100-preview.7.21326.4</SdkVersionForWorkloadTesting> in eng/Versions.props
Author: radical
Assignees: -
Labels:

arch-wasm, area-Build-mono

Milestone: -

@radical
Copy link
Member Author

radical commented Jun 29, 2021

@radical radical changed the title [wasm] Run Wasm.Build.Tests against workload [wasm] Run Wasm.Build.Tests against workloads Jun 29, 2021
<Project>
<PropertyGroup>
<WasmNativeWorkload>true</WasmNativeWorkload>
<UseMonoRuntime>true</UseMonoRuntime>
Copy link
Member

Choose a reason for hiding this comment

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

This file is always included once the workload is installed, you should stick any new properties in a conditionally included group.

Copy link
Member

Choose a reason for hiding this comment

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

But the right place to still all these is in Sdk.props I think

@radical radical force-pushed the wasm-native-libs-workloads branch from d42f53c to b7dfd99 Compare June 30, 2021 20:32
@radical radical force-pushed the wasm-native-libs-workloads branch 3 times, most recently from 9326d56 to 76cd866 Compare July 8, 2021 05:56
@radical radical marked this pull request as ready for review July 8, 2021 08:10
@radical radical requested a review from marek-safar as a code owner July 8, 2021 08:10
@radical
Copy link
Member Author

radical commented Jul 8, 2021

Just doing some final tidbits, but this should be good for reviewing.

@radical radical requested a review from akoeplinger July 8, 2021 22:34
@radical radical force-pushed the wasm-native-libs-workloads branch from da822a5 to 6ca6281 Compare July 11, 2021 08:59
@radical radical added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 12, 2021
@radical
Copy link
Member Author

radical commented Jul 12, 2021

/azp run

@azure-pipelines
Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@radical
Copy link
Member Author

radical commented Jul 12, 2021

/azp run runtime,runtime-staging

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@radical radical force-pushed the wasm-native-libs-workloads branch from b73db09 to df94014 Compare July 12, 2021 23:49
@radical radical removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 13, 2021
@@ -1,4 +1,8 @@
<Project>
<PropertyGroup>
<UseMonoRuntime Condition="'$(RuntimeIdentifier)' == 'Browser-wasm'">true</UseMonoRuntime>
Copy link
Member

Choose a reason for hiding this comment

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

I think/hope this is no longer needed after my #55258

Copy link
Member Author

Choose a reason for hiding this comment

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

Your PR changes the InTree, and LocalBuild targets, which don't get used for the workloads.

Copy link
Member

Choose a reason for hiding this comment

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

I think we could move it to an Sdk.props in the WebAssembly sdk though, that way it will only get picked up when that workload is in use.

Copy link
Member Author

Choose a reason for hiding this comment

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

Follow up PR? :) And if we are moving it to wasm sdk, then might as well move it to wasm props, then it won't be needed in InTree/LocalBuild either.

@radical
Copy link
Member Author

radical commented Jul 14, 2021

Windows x86 failure is #55611

Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

Discussed some follow up with @radical but since we're already have a pending pr building off this the changes can happen there

radekdoulik added a commit to radekdoulik/runtime that referenced this pull request Jul 14, 2021
@radical radical merged commit d953229 into dotnet:main Jul 14, 2021
@radical radical deleted the wasm-native-libs-workloads branch July 14, 2021 14:47
lewing pushed a commit that referenced this pull request Jul 19, 2021
* [wasm] Run browser tests on helix/windows

* Build just wasm/browsertests on helix/windows

* Use $(ChromiumRevision) in windows links

* Fix conditions

* Set PATH differently

* Use backslash in PATH on windows

* Try different version of chromium

* Pass scenario and browser host to build

And set browser path on windows to be able to start chrome
from xharness

* Try to get more info from the helix workitems

* Fix dir separator, add broser path

* Create WasmBuildSupportDir

* Revert "Try to get more info from the helix workitems"

This reverts commit 8807434.

* Put the dir cmds back, fix mkdir call

* More debug info

* Bump xharness

* Bump xharness again

With darc this time

* StressLogAnalyzer didn't print the number of messages correctly if it exceeded the int range (2 billion). (#54832)

Fix is to just use 64 bit ints instead.

* Found a race condition where the LOH flag on a segment is set too late. This gives another thread the chance to allocate in a fresh LOH region that doesn't have the LOH flag set just yet and trip over an assert in Object::ValidateInner. (#54839)

The fix is simply to set the flag in get_new_region before the region is put on the list for the LOH generation.

* Try to show the chrome logs

* Use different path for chrome logs

* Use newer image with font for chrome

The chrome was crashing, because it didn't find any sans-serif font.

* Increase timeouts

* Disable tests which timeout

* Remove debug calls

* Put back normal scenario

* Do not set scenario in build args

* Add browser sample exclusion

* Restore the platform matrix

* Remove the wasm build test changes

That will be handled in #54451

* Remove duplicate exclusion

* Suggested property name change

* Fix last merge

* Simplify condition

We don't pass Scenario anymore

* Include chrome and chromedriver in the payload

Co-authored-by: Peter Sollich <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants