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] Don't use workload for tfm < net6.0 #56606

Merged
merged 12 commits into from
Aug 15, 2021
Merged

[wasm] Don't use workload for tfm < net6.0 #56606

merged 12 commits into from
Aug 15, 2021

Conversation

radical
Copy link
Member

@radical radical commented Jul 30, 2021 •
edited
Loading

If the project requires the workload (eg. for AOT), but the project targets <net6.0, then error out before Publish.

To support this, the Wasm.Build.Tests not support testing with sdk, with no workload installed.

Currently, Wasm.Build.Tests are run in two modes:

  1. sdk($(SdkVersionForWorkloadTesting))+workload installed
  2. sdk(from global.json)+EMSDK+LocalBuild targets

this commit changes (2) to:

  1. sdk($(SdkVersionForWorkloadTesting)), no workload installed, +
    EMSDK+LocalBuild targets

This makes it closer to what a user would actually use.

Also, for the workload tests, this removes $(WasmNativeWorkload)
always being set to true. The workload should be setting that, and not
the tests.

Fixes #54308

@radical radical requested a review from marek-safar as a code owner July 30, 2021 08:58
@radical radical added arch-wasm WebAssembly architecture area-Build-mono labels Jul 30, 2021
@ghost
Copy link

ghost commented Jul 30, 2021

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

Issue Details

Fixes #54308

Author: radical
Assignees: -
Labels:

arch-wasm, area-Build-mono
Milestone: -

@radical radical requested a review from akoeplinger July 31, 2021 03:28
lewing
lewing reviewed Jul 31, 2021
View reviewed changes
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

I think we probably want an error/warning that explains this?

@radical
Copy link
Member Author

radical commented Aug 1, 2021 •
edited
Loading

I think we probably want an error/warning that explains this?

I've made it an error when the project requires a workload, but we have to disable it due to tfm<net6.0 . And emitting the error before publish, since that's when the workload would have kicked in. It will be changed to run before build, once we move to that from publish.

@radical radical requested a review from lewing August 1, 2021 01:10
akoeplinger
akoeplinger reviewed Aug 2, 2021
View reviewed changes
src/tests/BuildWasmApps/testassets/Blazor_net50/Program.cs
public static async Task Main(string[] args)
{
var builder = WebAssemblyHostBuilder.CreateDefault(args);
builder.RootComponents.Add<App>("#app");
Copy link
Member

@akoeplinger akoeplinger Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

nit: there are still a bunch of .css, .razor, weather.json, favicon.ico etc assets that don't seem relevant if we're not running the app so we could get rid of them too as having this Main() here should be enough to test the workload behavior right?

It looks good to me otherwise :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing the razor files, it ends up with pretty much an empty Main. I think, it would be preferable to have the razor files around at least, so they can get processed by the build. I'll remove the other files that are needed for running.

@radical radical added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 2, 2021
@radical
Copy link
Member Author

radical commented Aug 2, 2021

I think this needs some fixes right now, so marked it no-merge.

@radical
Copy link
Member Author

radical commented Aug 3, 2021

I had to make bigger changes to test the case of testing with latest sdk, and no workload installed. And that exposed other issues, so there are new changes here now. And I'll re-request for review, once I have it working on helix too.

@radical radical marked this pull request as draft August 3, 2021 04:50
@radical
Copy link
Member Author

radical commented Aug 3, 2021

/azp run runtime,runtime-staging

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@radical radical mentioned this pull request Aug 3, 2021
Merged
radical added 5 commits August 3, 2021 14:45
@radical
Bump sdk used for testing to 6.0.100-rc.1.21402.6
036b317
@radical
InstallWorkloadFromArtifacts: Add OnlyUpdateManifests parameter
5707c4b
@radical
[wasm] Run Wasm.Build.Tests with sdk/no-workload/+EMSDK
60850c0 
Currently, Wasm.Build.Tests are run in two modes:

1. sdk(`$(SdkVersionForWorkloadTesting)`)+workload installed
2. sdk(from global.json)+EMSDK+LocalBuild targets

this commit changes (2) to:

2. sdk(`$(SdkVersionForWorkloadTesting)`), no workload installed, +
   EMSDK+LocalBuild targets

This makes it closer to what a user would actually use.

Also, for the workload tests, this removes `$(WasmNativeWorkload)`
always being set to `true`. The workload should be setting that, and not
the tests.
@radical
Wasm.Build.Tests: Add test for net5.0 blazor projects
4b09e3b
@radical
Disable unrelated builds for now
6fa0efe
@radical radical force-pushed the wasm-skip-workload-5.0 branch from 790c3f0 to 6fa0efe Compare August 3, 2021 18:50
@radical
Copy link
Member Author

radical commented Aug 3, 2021

Remaining test failure is waiting on #56606 .

@radical radical removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 3, 2021
@radical radical marked this pull request as ready for review August 3, 2021 22:01
@radical
Copy link
Member Author

radical commented Aug 10, 2021

Blocked on #56974 to get the emsdk update.

@lewing lewing added this to the 6.0.0 milestone Aug 10, 2021
@radical radical mentioned this pull request Aug 11, 2021
Closed
@radical radical mentioned this pull request Aug 11, 2021
Closed
@radical
Copy link
Member Author

radical commented Aug 11, 2021

Installer test failure is #57243 , unrelated to this PR.

@radical
Copy link
Member Author

radical commented Aug 11, 2021

This could use some testing with VS, runtime/library tests, and blazorwasm projects.

lewing
lewing reviewed Aug 11, 2021
View reviewed changes
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

Looking good, I'll do some manual testing then approve.

@lewing lewing merged commit 98ca2ff into dotnet:main Aug 15, 2021
@jaredpar jaredpar mentioned this pull request Aug 17, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Sep 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lewing lewing lewing approved these changes

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

@steveisok steveisok Awaiting requested review from steveisok

Assignees
No one assigned
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Milestone
6.0.0
3 participants
@radical @lewing @akoeplinger