Skip to content

Conversation

@radical
Copy link
Member

@radical radical commented Aug 26, 2021

Currently, if the wasm-tools workload is not installed, and a project
uses AOT, then the build fails with an error saying that the workload
is needed.

But if the project is using native references, but not AOT, then the
build does not fail. Instead, the @(NativeFileReference) just gets
ignored. Even though the wasm workload is needed to relink dotnet.wasm
with the native libraries.

Implementation:

  • $(RunAOTCompilation) is a property, so it can be checked, and
    wasm workload imports can be enabled.

  • But @(NativeFileReference) is an item, and that gets evaluated in
    the second phase, so we can't use that to affect the imports.

    • Instead, we emit a warning from a target run before Build, if the
      project has any native references, but the workload isn't enabled.
  • Users can explicitly enable the workload by setting
    $(WasmBuildNative)==true.

Fixes #56678 .

@radical radical requested a review from marek-safar as a code owner August 26, 2021 01:40
@ghost
Copy link

ghost commented Aug 26, 2021

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@radical
Copy link
Member Author

radical commented Aug 26, 2021

This is the same as #58148, but that is separate to ensure that the changes can get tested against rc1 (which is using 6.0* versions vs 7.0 in main).

@radical radical force-pushed the workload-native-ref-main branch from 851c885 to 0a3303a Compare August 26, 2021 04:14
@radical radical marked this pull request as draft August 26, 2021 07:01
@radical radical force-pushed the workload-native-ref-main branch 2 times, most recently from c825102 to 6e38e0c Compare August 26, 2021 10:23
@radical radical marked this pull request as ready for review August 26, 2021 10:24
@radical radical added arch-wasm WebAssembly architecture area-Build-mono labels Aug 26, 2021
@ghost
Copy link

ghost commented Aug 26, 2021

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

Issue Details

Currently, if the wasm-tools workload is not installed, and a project
uses AOT, then the build fails with an error saying that the workload
is needed.

But if the project is using native references, but not AOT, then the
build does not fail. Instead, the @(NativeFileReference) just gets
ignored. Even though the wasm workload is needed to relink dotnet.wasm
with the native libraries.

Implementation:

  • $(RunAOTCompilation) is a property, so it can be checked, and
    wasm workload imports can be enabled.
  • But @(NativeFileReference) is an item, and that gets evaluated in
    the second phase, so we can't use that to affect the imports. Instead,
    a custom target is used.

Fixes #56678 .

Author: radical
Assignees: -
Labels:

arch-wasm, area-Build-mono

Milestone: -

@radical radical added this to the 6.0.0 milestone Aug 26, 2021
Copy link
Member

@lewing lewing Aug 26, 2021

Choose a reason for hiding this comment

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

We need to make sure we are requiring the correct pack, we don't need to insert our own error. Inserting an error here will break the logic in the VS/SDK that would guide the user to install the correct workload.

Currently, if the `wasm-tools` workload is not installed, and a project
uses AOT, then the build fails with an error saying that the workload
is needed.

But if the project is using native references, but not AOT, then the
build does not fail. Instead, the `@(NativeFileReference)` just gets
ignored. Even though the wasm workload is needed to relink dotnet.wasm
with the native libraries.

Implementation:

- `$(RunAOTCompilation)` is a property, so it can be checked, and
  wasm workload imports can be enabled.
- But `@(NativeFileReference)` is an item, and that gets evaluated in
  the second phase, so we can't use that to affect the imports.
  - Instead, we emit a warning from a target run before Build, if the
    project has any native references, but the workload isn't enabled.

- Users can explicitly enable the workload by setting
  `$(WasmBuildNative)==true`.
@radical radical force-pushed the workload-native-ref-main branch from 6e38e0c to 0f5cc66 Compare August 26, 2021 22:15
@radical radical requested a review from lewing August 26, 2021 23:25
@lewing lewing merged commit 9e03312 into dotnet:main Aug 27, 2021
@radical radical deleted the workload-native-ref-main branch August 27, 2021 17:20
@lewing
Copy link
Member

lewing commented Aug 27, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1175586954

@ghost ghost locked as resolved and limited conversation to collaborators Sep 26, 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.

[wasm] Require wasm workload, if a native reference is being used

2 participants