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

[release/6.0-rc1][wasm] Require workloads if using @(NativeFileReference) #58148

Closed
wants to merge 2 commits into from

Conversation

radical
Copy link
Member

@radical radical commented Aug 25, 2021

When a blazorwasm project has $(RunAOTCompilation)=true, then the wasm-tools workload is marked as required. But this is not done if the project wants to use native libraries, without AOT.

This PR makes the workload a requirement when such native libraries are being used (@(NativeFileReference)). This will cause the build to fail if the workload is not installed.

Fixes #56678 .

Customer Impact

Instead of the native library, silently, not getting linked, and failing at runtime, this fails the build.

Testing

CI. New tests added.

Risk

@radical radical added arch-wasm WebAssembly architecture area-Build-mono labels Aug 25, 2021
@radical radical requested a review from marek-safar as a code owner August 25, 2021 23:54
@ghost
Copy link

ghost commented Aug 25, 2021

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

Issue Details

If a project is using @(NativeFileReference), and is being built with
a sdk that doesn't have wasm-tools workload installed, then it will
fail.

Fixes #56678 .

Author: radical
Assignees: -
Labels:

arch-wasm, area-Build-mono

Milestone: -

@radical radical force-pushed the workload-native-ref branch from 7879452 to 4f692c7 Compare August 26, 2021 00:21
@radical radical changed the base branch from main to release/6.0 August 26, 2021 00:22
@radical radical changed the title [wasm] Require workloads if using @(NativeFileReference) [release/6.0][wasm] Require workloads if using @(NativeFileReference) Aug 26, 2021
@radical
Copy link
Member Author

radical commented Aug 26, 2021

I have opened this against release/6.0, mainly to ensure that the Wasm.Build.Tests get run with that, and my changes can be validated, since main is using 7.0 now. I will open a PR for main too, and confirm that it is testing the correct thing there.

@radical radical force-pushed the workload-native-ref branch from 4f692c7 to 6b97708 Compare August 26, 2021 01:35
@radical radical requested a review from BrzVlad as a code owner August 26, 2021 01:35
@radical radical changed the base branch from release/6.0 to release/6.0-rc1 August 26, 2021 01:35
@radical radical changed the title [release/6.0][wasm] Require workloads if using @(NativeFileReference) [release/6.0-rc1][wasm] Require workloads if using @(NativeFileReference) Aug 26, 2021
@radical radical removed the request for review from BrzVlad August 26, 2021 02:07
@radical radical force-pushed the workload-native-ref branch from 68b0b53 to 127d48f Compare August 26, 2021 04:15
@radical radical added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 26, 2021
@radical radical marked this pull request as draft August 26, 2021 06:17
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.
@radical radical force-pushed the workload-native-ref branch from 127d48f to cdeff4d Compare August 26, 2021 10:09
@radical radical marked this pull request as ready for review August 26, 2021 10:24
@radical radical removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 26, 2021
@lambdageek lambdageek added the Servicing-consider Issue for next servicing release review label Aug 26, 2021
@lambdageek lambdageek added this to the 6.0.0 milestone Aug 26, 2021
@lewing lewing removed the Servicing-consider Issue for next servicing release review label Aug 26, 2021
@lewing
Copy link
Member

lewing commented Aug 26, 2021

closing this while we discuss an alternative approach.

@lewing lewing closed this Aug 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 25, 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
3 participants