-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Conversation
Tagging subscribers to 'arch-wasm': @lewing Issue Detailsnull
|
acf4e33
to
270002c
Compare
I would like to see this merged so that I could start looking at the tests. How can I help ? |
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. |
84553f5
to
d918291
Compare
There was a problem hiding this 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.
@@ -0,0 +1,344 @@ | |||
<Project TreatAsLocalProperty="ArchiveTests"> |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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!
eng/testing/tests.wasi.targets
Outdated
<BundleTestAppTargets>$(BundleTestAppTargets);BundleTestWasmApp</BundleTestAppTargets> | ||
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' == 'Debug'">true</DebuggerSupport> | ||
|
||
<!-- set this when provisioning emsdk on CI --> |
There was a problem hiding this comment.
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
eng/testing/tests.wasi.targets
Outdated
--> | ||
<WasmNativeStrip Condition="'$(WasmNativeStrip)' == ''">false</WasmNativeStrip> | ||
<WasmEmitSymbolMap Condition="'$(WasmEmitSymbolMap)' == '' and '$(RunAOTCompilation)' != 'true'">true</WasmEmitSymbolMap> | ||
<WasmMainAssemblyFileName Condition="'$(WasmMainAssemblyFileName)' == ''">WasmTestRunner.dll</WasmMainAssemblyFileName> |
There was a problem hiding this comment.
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'" /> |
There was a problem hiding this comment.
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)" |
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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 ?
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. |
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-wasm |
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) |
There was a problem hiding this comment.
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
Wasi tests are failing on |
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); ```
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
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:
Fixes #72641 .