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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 22 commits into from
Feb 21, 2024
Merged

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

merged 22 commits into from
Feb 21, 2024

Conversation

mkhamoyan
Copy link
Contributor

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

Fixes #96863

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-wasm

@ghost ghost assigned mkhamoyan Feb 7, 2024
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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

ghost commented Feb 7, 2024

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: -

@mkhamoyan mkhamoyan marked this pull request as ready for review February 7, 2024 11:38
pavelsavara
pavelsavara reviewed Feb 7, 2024
View reviewed changes
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.

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
Copy link
Member

maraf commented Feb 7, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara
pavelsavara reviewed Feb 7, 2024
View reviewed changes
src/mono/wasm/build/WasmApp.Common.targets Outdated
@@ -496,9 +496,13 @@
'$(_IsToolchainMissing)' == 'true'"
Text="$(_ToolchainMissingErrorMessage) SDK is required for AOT'ing assemblies." />

<Error Condition="'$(WasmBuildNative)' == 'false' and '$(WasmSingleFileBundle)' == 'true'"
Copy link
Member

Choose a reason for hiding this comment

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

this check need to run after _BoolPropertiesThatTriggerRelinking caused the change

Copy link
Contributor Author

@mkhamoyan mkhamoyan Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

oh, it never sets it to false.

Copy link
Member

Choose a reason for hiding this comment

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

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

pavelsavara
pavelsavara approved these changes Feb 7, 2024
View reviewed changes
Copy link
Member

@pavelsavara pavelsavara left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

@mkhamoyan
Copy link
Contributor Author

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
Copy link
Contributor Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy ilonatommy mentioned this pull request Feb 7, 2024
Closed
@maraf
Copy link
Member

maraf commented Feb 7, 2024

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.

@build-analysis build-analysis bot mentioned this pull request Feb 7, 2024
Open
@mkhamoyan mkhamoyan marked this pull request as draft February 14, 2024 15:39
@build-analysis build-analysis bot mentioned this pull request Feb 14, 2024
Closed
@mkhamoyan
Copy link
Contributor Author

/azp run runtime-wasm

maraf
maraf reviewed Feb 15, 2024
View reviewed changes
eng/testing/linker/project.csproj.template Outdated
@@ -75,6 +75,18 @@
{AdditionalProjectReferences}
</ItemGroup>

<Target Name="RemoveInvariantGlobalization" BeforeTargets="_SetWasmBuildNativeDefaults">
Copy link
Member

Choose a reason for hiding this comment

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

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'">

Copy link
Member

@maraf maraf Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah TargetArchitecture will be enough.

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan mkhamoyan requested a review from maraf February 15, 2024 15:39
@mkhamoyan mkhamoyan marked this pull request as ready for review February 15, 2024 17:07
@build-analysis build-analysis bot mentioned this pull request Feb 15, 2024
Closed
maraf
maraf approved these changes Feb 16, 2024
View reviewed changes
eng/testing/linker/project.csproj.template Outdated
<_BoolPropertiesThatTriggerRelinking Remove="InvariantGlobalization" />
</ItemGroup>
</Target>
<PropertyGroup Condition="'$(TargetArchitecture)' == 'wasm'">
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 to register the target? The execution should be enforced by BeforeTargets="_SetWasmBuildNativeDefaults"

radical
radical reviewed Feb 16, 2024
View reviewed changes
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'." />
Copy link
Member

Choose a reason for hiding this comment

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

Test needed for this.

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

mkhamoyan commented Feb 21, 2024 •
edited
Loading

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

@mkhamoyan mkhamoyan merged commit 7af3a6f into dotnet:main Feb 21, 2024
187 of 189 checks passed
@mkhamoyan mkhamoyan deleted the wasi_error_message branch February 21, 2024 14:22
@github-actions github-actions bot locked and limited conversation to collaborators Mar 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pavelsavara pavelsavara pavelsavara approved these changes

@radical radical radical left review comments

@maraf maraf maraf approved these changes

@akoeplinger akoeplinger Awaiting requested review from akoeplinger akoeplinger is a code owner

@lewing lewing Awaiting requested review from lewing

@ilonatommy ilonatommy Awaiting requested review from ilonatommy ilonatommy is a code owner

Assignees

@mkhamoyan mkhamoyan

Labels
arch-wasm WebAssembly architecture area-Build-mono os-wasi Related to WASI variant of arch-wasm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[wasm] Setting WasmBuildNative to false produces no output wasm binary
5 participants
@mkhamoyan @maraf @pavelsavara @radical @lewing