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

[Blazor] Update SDK to account for the crypto worker file #26966

Merged
merged 9 commits into from
Aug 7, 2022

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Aug 3, 2022

  • Updates the Blazor SDK to account for the JS worker script used for crypto operations.
  • The change updates blazor.boot.json to include a new section runtimeAssets that contains additional assets that are passed directly to the runtime for consumption.

@dotnet-issue-labeler dotnet-issue-labeler bot added the Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch label Aug 3, 2022
@javiercn javiercn marked this pull request as ready for review August 5, 2022 09:27
@@ -157,6 +157,9 @@ Copyright (c) .NET Foundation. All rights reserved.
<!-- Remove dotnet.js/wasm from runtime pack, in favor of the relinked ones in @(WasmNativeAsset) -->
<ReferenceCopyLocalPaths Remove="@(ReferenceCopyLocalPaths)"
Condition="@(WasmNativeAsset->Count()) > 0 and '%(FileName)' == 'dotnet' and ('%(Extension)' == '.wasm' or '%(Extension)' == '.js')" />

<ReferenceCopyLocalPaths Remove="@(ReferenceCopyLocalPaths)"
Condition="@(WasmNativeAsset->Count()) > 0 and '%(FileName)' == 'dotnet-crypto-worker' and '%(Extension)' == '.js'" />
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that @(WasmNativeAsset->Count()) is the right condition, either here or above. Shouldn't it be checking for WasmBuildNative? That automatically becomes true if there are any WasmNativeAsset entries, but can also be true even if there are none (e.g., because the developer is just trying to change some config like EmccInitialHeapSize, which I have been doing recently)

Copy link
Member

Choose a reason for hiding this comment

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

Also why are we changing this in the 6.0 targets? Does the new feature impact 6.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really since the 6.0 runtime pack does not contain these files. The only change that .NET 6.0 will see is that we add an additional section to the manifest when we build with .NET 7.0 SDK (because we put the dotnet.wasm file there). But anything that .NET 6.0 reads should be the same.

We had 5.0 and 6.0 targets because we didn't want to change how 5.0 apps built with the .NET 6.0 SDK at the time, but 6.0 means current. We can rename the file in a follow up change afterwards if we need to.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that @(WasmNativeAsset->Count()) is the right condition, either here or above. Shouldn't it be checking for WasmBuildNative? That automatically becomes true if there are any WasmNativeAsset entries, but can also be true even if there are none (e.g., because the developer is just trying to change some config like EmccInitialHeapSize, which I have been doing recently)

The logic here predates me (this is something that Pranav worked out as part of 5.0) but essentially what this is doing is removing the file from the runtime pack if a different file was generated as part of AoT compilation so that the native version is picked up.

Copy link
Member

Choose a reason for hiding this comment

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

I know, but it's using the wrong condition to know whether a native compilation occurred. Native compilations occur if:

  • $(WasmBuildNative) was explicitly set to true
  • Or if @(WasmNativeAsset) is nonempty (which sets $(WasmBuildNative) to true automatically)
  • Or if $(RunAotCompilation) is true (which sets $(WasmBuildNative) to true automatically, and might also add entries to @(WasmNativeAsset) but I don't know)

So it's not just about AOT, nor just about having native dependencies. It's either of those, or the case when a developer explicitly turns on native compilation just because they want to tweak the Emscripten args.

As far as I can tell, the logic that predates you is incorrect, and probably behaves weirdly when developers just set $(WasmBuildNative) to true.

I appreciate you won't want to interfere with build logic unrelated to your PR here, so I totally accept if you want to just inherit the existing probably-buggy behavior. If this does turn out to lead to actual problems for customers, we can give the workaround of "just add an empty file as a native dependency". Longer term of course, we don't want to have any of this native build logic in the BlazorWebAssembly targets as it's all stuff that should be baked into the runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SteveSandersonMS I think I see what you mean and it definitely seems that the condition here is wrong. I'm trying to think what the best way to solve this is, since the goal of this is something like:

  • If there is a WasmNativeAsset file, prefer that.

javiercn and others added 3 commits August 5, 2022 16:02
Address feedback

Co-authored-by: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
@javiercn javiercn force-pushed the javiercn/update-sdk-crypto-worker branch from e3a4e2d to f082f8d Compare August 5, 2022 15:23
@javiercn javiercn merged commit 4547cf7 into main Aug 7, 2022
@javiercn javiercn deleted the javiercn/update-sdk-crypto-worker branch August 7, 2022 10:00
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.

3 participants