[wasm][wasi] Throw error when WasmBuildNative is explicitly set to false during a single-file build #98087

mkhamoyan · 2024-02-07T11:36:59Z

WasmSingleFileBundle requires native build; therefore, explicitly setting WasmBuildNative to false is not allowed.

Fixes #96863

… build

mkhamoyan · 2024-02-07T11:37:06Z

/azp run runtime-wasm

azure-pipelines · 2024-02-07T11:37:22Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2024-02-07T11:37:55Z

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

Issue Details

WasmSingleFileBundle requires native build; therefore, explicitly setting WasmBuildNative to false is not allowed.

Fixes #96863

Author:	mkhamoyan
Assignees:	mkhamoyan
Labels:	`arch-wasm`, `area-Build-mono`, `os-wasi`
Milestone:	-

pavelsavara

I wonder if WasmSingleFileBundle should also be added to _BoolPropertiesThatTriggerRelinking instead of

runtime/src/mono/wasm/build/WasmApp.Common.targets

Line 524 in 97aba78

    
           <WasmBuildNative Condition="'$(WasmBuildNative)' == '' and '$(WasmSingleFileBundle)' == 'true'">true</WasmBuildNative>

maraf · 2024-02-07T11:45:43Z

I wonder if WasmSingleFileBundle should also be added to _BoolPropertiesThatTriggerRelinking instead of

That is a good idea. The error should happen everytime when _BoolPropertiesThatTriggerRelinking is triggered and the WasmBuildNative is explicitly set to false.

mkhamoyan · 2024-02-07T12:40:06Z

I wonder if WasmSingleFileBundle should also be added to _BoolPropertiesThatTriggerRelinking instead of

runtime/src/mono/wasm/build/WasmApp.Common.targets

Line 524 in 97aba78

<WasmBuildNative Condition="'$(WasmBuildNative)' == '' and '$(WasmSingleFileBundle)' == 'true'">true</WasmBuildNative>

Thanks! Updated.

mkhamoyan · 2024-02-07T12:42:38Z

/azp run runtime-wasm

azure-pipelines · 2024-02-07T12:42:48Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2024-02-07T12:43:53Z

src/mono/wasm/build/WasmApp.Common.targets

                        '$(_IsToolchainMissing)' == 'true'"
           Text="$(_ToolchainMissingErrorMessage) SDK is required for AOT'ing assemblies." />

+    <Error Condition="'$(WasmBuildNative)' == 'false' and '$(WasmSingleFileBundle)' == 'true'"


this check need to run after _BoolPropertiesThatTriggerRelinking caused the change

@pavelsavara could you please clarify why? _BoolPropertiesThatTriggerRelinking will only affect if $(WasmBuildNative)' == ''

runtime/src/mono/wasm/build/WasmApp.Common.targets

Line 505 in 59831ac

<WasmBuildNative Condition="'$(WasmBuildNative)' == '' and

so it shouldn't change the check outcome.

oh, it never sets it to false.

but if users sets WasmBuildNative false and also sets something which would make it true via _BoolPropertiesThatTriggerRelinking we have broken combination.

pavelsavara

I see few common _BoolPropertiesThatTriggerRelinking in BrowserWasmApp.targets and in WasiApp.targets could we have them in common place ?

…gets

mkhamoyan · 2024-02-07T16:11:18Z

I see few common _BoolPropertiesThatTriggerRelinking in BrowserWasmApp.targets and in WasiApp.targets could we have them in common place ?

Moved only WasmNativeStrip. WasmEnableSIMD has different default values on wasi/browser.

mkhamoyan · 2024-02-07T16:11:36Z

/azp run runtime-wasm

azure-pipelines · 2024-02-07T16:11:45Z

Azure Pipelines successfully started running 1 pipeline(s).

maraf · 2024-02-07T18:29:02Z

That is a good idea. The error should happen everytime when _BoolPropertiesThatTriggerRelinking is triggered and the WasmBuildNative is explicitly set to false.

Can we implement a general rule? For example setting WasmEnableSIMD=false and WasmBuildNative=false is also not supported combination.

mkhamoyan · 2024-02-15T15:09:33Z

/azp run runtime-wasm

maraf · 2024-02-15T15:09:47Z

eng/testing/linker/project.csproj.template

    {AdditionalProjectReferences}
  </ItemGroup>

+  <Target Name="RemoveInvariantGlobalization" BeforeTargets="_SetWasmBuildNativeDefaults">


The _SetWasmBuildNativeDefaults target won't be there for other RIDs

Suggested change

<Target Name="RemoveInvariantGlobalization" BeforeTargets="_SetWasmBuildNativeDefaults">

<Target Name="RemoveInvariantGlobalization" BeforeTargets="_SetWasmBuildNativeDefaults" Condition="'$(TargetArchitecture)' == 'wasm' and '$(TargetOS)' == 'browser'">

Do the trimming tests run for wasi?
Probably checking TargetArchitecture is enough

Yeah TargetArchitecture will be enough.

azure-pipelines · 2024-02-15T15:09:54Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2024-02-15T15:36:47Z

/azp run runtime-wasm

azure-pipelines · 2024-02-15T15:36:58Z

Azure Pipelines successfully started running 1 pipeline(s).

maraf · 2024-02-16T13:27:37Z

eng/testing/linker/project.csproj.template

+      <_BoolPropertiesThatTriggerRelinking Remove="InvariantGlobalization" />
+    </ItemGroup>
+  </Target>
+  <PropertyGroup Condition="'$(TargetArchitecture)' == 'wasm'">


Do we need to register the target? The execution should be enforced by BeforeTargets="_SetWasmBuildNativeDefaults"

radical · 2024-02-16T20:46:11Z

src/mono/wasm/build/WasmApp.Common.targets

+    </PropertyGroup>
+
+    <Error Condition="'$(WasmBuildNative)' == 'false' and '$(_WasmBuildNativeRequired)' == 'true'" 
+           Text="WasmBuildNative is required because %(_ChangedBoolPropertiesThatTriggerRelinking.Identity)=$(%(_ChangedBoolPropertiesThatTriggerRelinking.Identity)), but WasmBuildNative is already set to 'false'." />


Test needed for this.

mkhamoyan · 2024-02-21T09:37:41Z

/azp run runtime-wasm

azure-pipelines · 2024-02-21T09:37:51Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2024-02-21T14:20:49Z

PortableSourceBuild failure is not related, happens in other PRs also.

mkhamoyan added 2 commits February 7, 2024 12:24

Add error when WasmBuildNative is explicitly false during single-file…

bddc91b

… build

Remove the space

7b578c4

ghost added the area-Build-mono label Feb 7, 2024

ghost assigned mkhamoyan Feb 7, 2024

mkhamoyan added arch-wasm WebAssembly architecture os-wasi Related to WASI variant of arch-wasm labels Feb 7, 2024

mkhamoyan marked this pull request as ready for review February 7, 2024 11:38

mkhamoyan requested review from akoeplinger and maraf as code owners February 7, 2024 11:38

mkhamoyan requested review from lewing and pavelsavara February 7, 2024 11:38

pavelsavara reviewed Feb 7, 2024

View reviewed changes

Add WasmSingleFileBundle to _BoolPropertiesThatTriggerRelinking

9e49413

mkhamoyan requested a review from pavelsavara February 7, 2024 12:41

pavelsavara reviewed Feb 7, 2024

View reviewed changes

pavelsavara approved these changes Feb 7, 2024

View reviewed changes

Move common _BoolPropertiesThatTriggerRelinking to WasmApp.Common.tar…

88fb7ed

…gets

ilonatommy mentioned this pull request Feb 7, 2024

[debugger][mt][browser] TestDebugUsingMultiThreadedRuntime times out #98108

Closed

build-analysis bot mentioned this pull request Feb 7, 2024

[browser][MT] MutexTests - operation does not support unaligned accesses #96191

Open

mkhamoyan added 4 commits February 14, 2024 12:47

Try to enable WasmBuildNative only for InvariantGlobalizationTrue tests

e458b88

Remove InvariantGlobalization from trigger relinking for browser

9a61a09

Move under target

10f820a

disable trigger relinking for InvariantGlobalization

17bb799

mkhamoyan marked this pull request as draft February 14, 2024 15:39

build-analysis bot mentioned this pull request Feb 14, 2024

[mono][tvos] OOM in System.IO.Tests.MemoryStreamTests #92467

Closed

mkhamoyan added 2 commits February 15, 2024 15:31

Move RemoveInvariantGlobalization to testing/linker templates

71a7599

Add space

810a970

maraf reviewed Feb 15, 2024

View reviewed changes

Add target RemoveInvariantGlobalization only for wasm

4716ecc

mkhamoyan requested a review from maraf February 15, 2024 15:39

Fix syntax error

4f53311

mkhamoyan marked this pull request as ready for review February 15, 2024 17:07

build-analysis bot mentioned this pull request Feb 15, 2024

[browser][WBT] DownloadResourceProgressTests.DownloadProgressFinishes #98486

Closed

maraf approved these changes Feb 16, 2024

View reviewed changes

radical reviewed Feb 16, 2024

View reviewed changes

lewing and others added 2 commits February 21, 2024 01:40

Merge branch 'main' into wasi_error_message

4ce8ce6

Add test case

b6c55f4

mkhamoyan requested a review from ilonatommy as a code owner February 21, 2024 09:36

Update the test case

3cf41b4

mkhamoyan merged commit 7af3a6f into dotnet:main Feb 21, 2024

mkhamoyan deleted the wasi_error_message branch February 21, 2024 14:22

github-actions bot locked and limited conversation to collaborators Mar 23, 2024

	<Target Name="RemoveInvariantGlobalization" BeforeTargets="_SetWasmBuildNativeDefaults">
	<Target Name="RemoveInvariantGlobalization" BeforeTargets="_SetWasmBuildNativeDefaults" Condition="'$(TargetArchitecture)' == 'wasm' and '$(TargetOS)' == 'browser'">

[wasm][wasi] Throw error when WasmBuildNative is explicitly set to false during a single-file build #98087

[wasm][wasi] Throw error when WasmBuildNative is explicitly set to false during a single-file build #98087

Uh oh!

Conversation

mkhamoyan commented Feb 7, 2024

Uh oh!

mkhamoyan commented Feb 7, 2024

Uh oh!

azure-pipelines bot commented Feb 7, 2024

Uh oh!

ghost commented Feb 7, 2024

Uh oh!

pavelsavara left a comment

Choose a reason for hiding this comment

Uh oh!

maraf commented Feb 7, 2024

Uh oh!

mkhamoyan commented Feb 7, 2024

Uh oh!

mkhamoyan commented Feb 7, 2024

Uh oh!

azure-pipelines bot commented Feb 7, 2024

Uh oh!

pavelsavara Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

mkhamoyan Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavelsavara Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

pavelsavara Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

pavelsavara left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhamoyan commented Feb 7, 2024

Uh oh!

mkhamoyan commented Feb 7, 2024

Uh oh!

azure-pipelines bot commented Feb 7, 2024

Uh oh!

maraf commented Feb 7, 2024

Uh oh!

mkhamoyan commented Feb 15, 2024

Uh oh!

maraf Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

maraf Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhamoyan Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

azure-pipelines bot commented Feb 15, 2024

Uh oh!

mkhamoyan commented Feb 15, 2024

Uh oh!

azure-pipelines bot commented Feb 15, 2024

Uh oh!

maraf Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

radical Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

mkhamoyan commented Feb 21, 2024

Uh oh!

azure-pipelines bot commented Feb 21, 2024

Uh oh!

mkhamoyan commented Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

mkhamoyan Feb 7, 2024 •

edited

Loading

pavelsavara left a comment •

edited

Loading

maraf Feb 15, 2024 •

edited

Loading

mkhamoyan commented Feb 21, 2024 •

edited

Loading