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

[release/6.0][wasm] Fix publishing *blazorwasm* project with lazy loaded assembly #60497

Closed
wants to merge 3 commits into from
Closed

[release/6.0][wasm] Fix publishing *blazorwasm* project with lazy loaded assembly #60497

wants to merge 3 commits into from

Conversation

radical
Copy link
Member

@radical radical commented Oct 15, 2021 •
edited
Loading

Fixes a user reported issue - #60479

Customer Impact

This fixes the blazorwasm projects that use lazy loaded assemblies, when the wasm-tools workload is installed.

Testing

Some manual testing, manually running blazor/sdk tests with the workload, and a new unit test.

Risk

Medium.

@ghost
Copy link

ghost commented Oct 15, 2021

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

TBD

Author: radical
Assignees: -
Labels:

area-Infrastructure-mono
Milestone: -

@radical radical added arch-wasm WebAssembly architecture area-Build-mono labels Oct 15, 2021
@ghost
Copy link

ghost commented Oct 15, 2021

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

Issue Details

TBD

Author: radical
Assignees: -
Labels:

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

@radical
[wasm] Allow selecting which target to run before, for nested publish
3bca81d 
Part of fix for dotnet#60479 .
The other part of the fix will be in `dotnet/sdk`.

- Currently for the nested publish, we run `Publish`, and then our
  native targets. But that can be an overkill, and can cause issues like
  the one being fixed here.
- Instead, allow blazor to configure which target to run instead, which
  would provide us with the correct set of inputs.
  - In future, this will be cleaned up, and implemented differently.

- Also, a fixed up copy of
  `Microsoft.NET.Sdk.BlazorWebAssembly.6_0.targets` is included here, so
  we can run the corresponding test. And it can be removed once the SDK
  part of the fix gets merged.
@radical
bump sdk for workload testing to 6.0.100-rtm.21515.27
75baa77
@radical radical force-pushed the fix-blazor-lazyload-6-0 branch from f23f39e to be38383 Compare October 18, 2021 18:09
@radical radical requested review from lewing and javiercn October 18, 2021 18:11
@radical radical marked this pull request as ready for review October 18, 2021 18:11
@radical radical requested a review from marek-safar as a code owner October 18, 2021 18:11
@radical
Copy link
Member Author

radical commented Oct 18, 2021

This has a corresponding dotnet/sdk fix - dotnet/sdk#22145 .
I'll re-enable the non-ways builds, once I can confirm here that the wasm tests pass.

lewing
lewing reviewed Oct 18, 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.

can you clean up the platform-matrix changes?

@radical radical changed the title [wasm] Fix https://github.com/dotnet/runtime/issues/60479 [release/6.0][wasm] Fix publishing *blazorwasm* project with lazy loaded assembly Oct 18, 2021
@lewing
Copy link
Member

lewing commented Oct 18, 2021

@Anipik this is a late breaking change we'd like to get in. Working on the template and email now.

@lewing lewing added this to the 6.0.0 milestone Oct 18, 2021
@lewing lewing added the Servicing-consider Issue for next servicing release review label Oct 18, 2021
@radical radical mentioned this pull request Oct 18, 2021
Merged
@lewing lewing force-pushed the fix-blazor-lazyload-6-0 branch from be38383 to 8b0fdd8 Compare October 18, 2021 20:18
@radical
Copy link
Member Author

radical commented Oct 18, 2021

Since the blazorwasm targets from the sdk are imported after the WasmApp.targets, the original property _WasmNestedPublishAppPreTarget can be overridden in the blazorwasm targets file. And we can skip the change here, in an effort to minimize the overhead for merging a fix in the sdk for the issue. Closing this.

@radical radical closed this Oct 18, 2021
@lewing
Copy link
Member

lewing commented Oct 19, 2021

@radical can you pull the test here into main?

@radical radical deleted the fix-blazor-lazyload-6-0 branch October 19, 2021 23:26
@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lewing lewing lewing left review comments

@javiercn javiercn Awaiting requested review from javiercn

@marek-safar marek-safar Awaiting requested review from marek-safar marek-safar is a code owner

Assignees
No one assigned
Labels
arch-wasm WebAssembly architecture area-Build-mono area-Infrastructure-mono Servicing-consider Issue for next servicing release review
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@radical @lewing