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] wasi: Enable library tests on CI #81052

Merged
merged 40 commits into from
Feb 4, 2023
Merged

Conversation

radical
Copy link
Member

@radical radical commented Jan 23, 2023

  • Adds tasks/targets to run a wasi console sample, and tests on CI - for linux, and windows
  • Only few tests are enabled by default
  • Other tests fail due to GC issues, missing fs/network access etc
  • Also, adds runtime-wasm-optional pipeline which will have the wasi build with all the tests enabled, and will be RED. This is disabled right now.

Some differences from browser/wasm case:

    [wasi] Use the real entrypoint even with async Main

    When the main assembly has a async main, roslyn generates a wrapper
    method which invokes the async main. And in case of wasm that
    async-main method is invoked so it can yield to the host.

    But in case of wasi we invoke the actual entrypoint.

Fixes #72641 .

@radical radical added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it arch-wasm WebAssembly architecture labels Jan 23, 2023
@ghost ghost assigned radical Jan 23, 2023
@ghost
Copy link

ghost commented Jan 23, 2023

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

Issue Details

null

Author: radical
Assignees: -
Labels:

NO-MERGE, NO-REVIEW, arch-wasm

Milestone: -

@pavelsavara
Copy link
Member

I would like to see this merged so that I could start looking at the tests. How can I help ?

@radical
Copy link
Member Author

radical commented Jan 30, 2023

update: I'm cleaning this up now. I have to check the various runtime/native changes/cmake changes to make sure that they are still needed, after the earlier wasi PR being merged. I will open this up for review in a few days.

@radical radical removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Jan 31, 2023
@radical radical changed the title WIP: [wasm] Wasi build improvements [wasm] wasi: Enable library tests on CI Jan 31, 2023
Copy link
Member

@pavelsavara pavelsavara left a comment

Choose a reason for hiding this comment

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

good work!

This is large PR and I made many comments, but I don't insist on any of them.
I would like to merge it soon and fix things incrementally on top of this in further PRs.

eng/pipelines/common/platform-matrix.yml Outdated Show resolved Hide resolved
eng/testing/WasiRunnerTemplate.sh Show resolved Hide resolved
@@ -0,0 +1,344 @@
<Project TreatAsLocalProperty="ArchiveTests">
Copy link
Member

Choose a reason for hiding this comment

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

Is this just rename ? Or is there something to review in detail ?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are bits that I'm extracting from this to the base targets file. But this is very much WIP, so tests passing is the check for the changes right now!

<BundleTestAppTargets>$(BundleTestAppTargets);BundleTestWasmApp</BundleTestAppTargets>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' == 'Debug'">true</DebuggerSupport>

<!-- set this when provisioning emsdk on CI -->
Copy link
Member

Choose a reason for hiding this comment

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

wasi SDK & WASI_SDK_PATH

-->
<WasmNativeStrip Condition="'$(WasmNativeStrip)' == ''">false</WasmNativeStrip>
<WasmEmitSymbolMap Condition="'$(WasmEmitSymbolMap)' == '' and '$(RunAOTCompilation)' != 'true'">true</WasmEmitSymbolMap>
<WasmMainAssemblyFileName Condition="'$(WasmMainAssemblyFileName)' == ''">WasmTestRunner.dll</WasmMainAssemblyFileName>
Copy link
Member

Choose a reason for hiding this comment

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

Q: is WasmTestRunner.dll identical to browser ? If not, should they have different project or name ?

<WasmNativeAsset Include="$(MicrosoftNetCoreAppRuntimePackRidNativeDir)$(WasmIcuDataFileName)" Condition="'$(InvariantGlobalization)' != 'true'" />
<WasmNativeAsset Include="$(MicrosoftNetCoreAppRuntimePackRidNativeDir)dotnet.timezones.blat" />

<WasmFilesToIncludeInFileSystem Include="@(WasmNativeAsset)" Condition="'%(WasmNativeAsset.FileName)%(WasmNativeAsset.Extension)' == 'dotnet.js.symbols'" />
Copy link
Member

Choose a reason for hiding this comment

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

dotnet.js.symbols could be removed. dotnet.timezones.blat needs to be discussed in context of VFS #81418

<RemoveDir Directories="$(WasmAppDir)" />
<WasmAppBuilder
AppDir="$(WasmAppDir)"
MainJS="$(WasmMainJSPath)"
Copy link
Member

Choose a reason for hiding this comment

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

no JS

<WriteLinesToFile File="$(WasmAppDir)\.stamp" Lines="" Overwrite="true" />
</Target>

<Target Name="_GenerateRunV8Script">
Copy link
Member

Choose a reason for hiding this comment

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

do we need wasmtime script ?

src/mono/wasi/mono-include/driver.h Outdated Show resolved Hide resolved
@radical
Copy link
Member Author

radical commented Jan 31, 2023

I've read the feedback on all the tests targets, and wasi targets files, but ignoring them for now, because they are very much in WIP. Most of them are copies of the existing wasm files, and will get pruned in future PRs.

@radical
Copy link
Member Author

radical commented Feb 1, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

 * also for relink
@@ -2293,7 +2293,7 @@ sgen_client_scan_thread_data (void *start_nursery, void *end_nursery, gboolean p
{
scan_area_arg_start = start_nursery;
scan_area_arg_end = end_nursery;
#ifdef HOST_WASM
#if defined(HOST_WASM) || defined(HOST_WASI)
Copy link
Member

Choose a reason for hiding this comment

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

should be rather HOST_BROWSER ?

 - asserts about order of memory segments
 - further unified re-link and get rig of --stack-first
@radical
Copy link
Member Author

radical commented Feb 3, 2023

the current memory layout:

```
  /*
   * | -- increasing address ---> |
   * | data (data_end)| stack |(heap_base) heap |
   */
```

.. based on which we can calculate a reasonable stack size.

Tests were failing with the earlier code on the following assert:

```c
size_t stack_size_pad = wasm_get_stack_size() + 12; // there is padding
g_assert (heap_base - stack_size_pad == data_end);
```
@radical
Copy link
Member Author

radical commented Feb 4, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical radical merged commit f8daacf into dotnet:main Feb 4, 2023
@radical radical deleted the aj-wasi_build branch February 4, 2023 22:35
@ghost ghost locked as resolved and limited conversation to collaborators Mar 7, 2023
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 os-wasi Related to WASI variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mono][wasi] Add a CI lane for a new wasi RID
3 participants