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

Fix order of targets in WasmNestedPublishAppDependsOn #31640

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

maraf
Copy link
Member

@maraf maraf commented Apr 6, 2023

Regression from #31154.

The _GatherBlazorFilesToPublish must run after _GatherWasmFilesToPublish, because
the _GatherBlazorFilesToPublish modifies metadata on item WasmAssembliesToBundle added by _GatherWasmFilesToPublish.

Fix for failing Wasm.Build.Test in dotnet/runtime#84368.

@maraf maraf requested a review from lewing April 6, 2023 12:47
@maraf maraf self-assigned this Apr 6, 2023
@maraf maraf requested a review from a team as a code owner April 6, 2023 12:47
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch untriaged Request triage from a team member labels Apr 6, 2023
@ghost
Copy link

ghost commented Apr 6, 2023

Thanks for your PR, @maraf.
To learn about the PR process and branching schedule of this repo, please take a look at the SDK PR Guide.

@maraf maraf removed the untriaged Request triage from a team member label Apr 6, 2023
@maraf maraf merged commit 484a83d into dotnet:main Apr 6, 2023
@maraf maraf deleted the WasmFixAotJsInterop branch April 18, 2023 21:09
radical added a commit that referenced this pull request Apr 19, 2023
- For blazor AOT, the blazor targets explicitly skip
  `Microsoft.JSInterop.WebAssembly.dll` in `_GatherBlazorFilesToPublish` target.
  - it is done by marking the assembly item in
    `@(WasmAssembliesToBundle)` with `%(AOT_InternalForceToInterpret)="true"`.

- this target needs to run after `_GatherWasmFilesToPublish` which builds
  the list of assemblies to AOT in `@(WasmAssembliesToBundle)`

- the order of these two targets was corrected in #31640
- but the changes in #31559 added the
  target twice, resulting in the order:

  `_GatherBlazorFilesToPublish;_GatherWasmFilesToPublish;_GatherBlazorFilesToPublish`

  1. in which the first `_GatherBlazorFilesToPublish` tries to remove
     the assembly from an empty list
  2. then `_GatherWasmFilesToPublish` populates the list
  3. and the last instance of `_GatherBlazorFilesToPublish` target
     doesn't run because it has already run before in (2).
- thus the assembly never gets removed from the list.

Fixes: dotnet/runtime#85010
radical added a commit that referenced this pull request Apr 19, 2023
- For blazor AOT, the blazor targets explicitly skip `Microsoft.JSInterop.WebAssembly.dll` in `_GatherBlazorFilesToPublish` target.
  - it is done by marking the assembly item in `@(WasmAssembliesToBundle)` with `%(AOT_InternalForceToInterpret)="true"`.

- this target needs to run after `_GatherWasmFilesToPublish` which builds the list of assemblies to AOT in `@(WasmAssembliesToBundle)`

- the order of these two targets was corrected in #31640
- but the changes in #31559 added the target twice, resulting in the order:

  `_GatherBlazorFilesToPublish;_GatherWasmFilesToPublish;_GatherBlazorFilesToPublish`

  1. in which the first `_GatherBlazorFilesToPublish` tries to remove the assembly from an empty list
  2. then `_GatherWasmFilesToPublish` populates the list
  3. and the last instance of `_GatherBlazorFilesToPublish` target doesn't run because it has already run before in (2).
- thus the assembly never gets removed from the list.

Fixes: dotnet/runtime#85010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants