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

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

Merged
merged 9 commits into from
Aug 7, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8e592ce
Account for the crypto-worker-js file from the runtime at build time
javiercn Aug 3, 2022
836b9ac
Update baselines
javiercn Aug 3, 2022
c1066c6
rename runtimeAssets
pavelsavara Aug 4, 2022
8cc58cb
fixed publish
pavelsavara Aug 4, 2022
a995950
Update baselines
javiercn Aug 5, 2022
73f35a8
Apply suggestions from code review
javiercn Aug 5, 2022
11e9411
Address feedback
javiercn Aug 5, 2022
9a56f31
Fix culture bug
javiercn Aug 5, 2022
f082f8d
Fix bad merge
javiercn Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/BlazorWasmSdk/Targets/Microsoft.NET.Sdk.BlazorWebAssembly.6_0.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

</ItemGroup>

<ComputeBlazorBuildAssets
Expand Down Expand Up @@ -403,6 +406,7 @@ Copyright (c) .NET Foundation. All rights reserved.

<ItemGroup>
<_DotNetJsItem Include="@(ResolvedFileToPublish)" Condition="'%(ResolvedFileToPublish.DestinationSubPath)' == 'dotnet.js' AND '%(ResolvedFileToPublish.AssetType)' == 'native'" />
<_DotNetJsItem Include="@(ResolvedFileToPublish)" Condition="'%(ResolvedFileToPublish.DestinationSubPath)' == 'dotnet-crypto-worker.js' AND '%(ResolvedFileToPublish.AssetType)' == 'native'" />
</ItemGroup>

<PropertyGroup>
Expand Down
7 changes: 7 additions & 0 deletions src/BlazorWasmSdk/Tasks/BootJsonData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ public class ResourcesData
/// </summary>
[DataMember(EmitDefaultValue = false)]
public Dictionary<string, ResourceHashesByNameDictionary> extensions { get; set; }

/// <summary>
/// Additional assets that the runtime consumes as part of the boot process.
/// </summary>
[DataMember(EmitDefaultValue = false)]
public Dictionary<string, AdditionalAsset> runtimeAssets { get; set; }
SteveSandersonMS marked this conversation as resolved.
Show resolved Hide resolved

}

public enum ICUDataMode : int
Expand Down
25 changes: 24 additions & 1 deletion src/BlazorWasmSdk/Tasks/ComputeBlazorBuildAssets.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,28 @@ public override bool Execute()
assetCandidates.Add(newDotNetJs);
continue;
}
else if (candidate.GetMetadata("FileName") == "dotnet-crypto-worker" && candidate.GetMetadata("Extension") == ".js")
{
var itemHash = FileHasher.GetFileHash(candidate.ItemSpec);
var cacheBustedDotNetCryptoWorkerJSFileName = $"dotnet-crypto-worker.{candidate.GetMetadata("NuGetPackageVersion")}.{itemHash}.js";

var originalFileFullPath = Path.GetFullPath(candidate.ItemSpec);
var originalFileDirectory = Path.GetDirectoryName(originalFileFullPath);

var cacheBustedDotNetCryptoWorkerJSFullPath = Path.Combine(originalFileDirectory, cacheBustedDotNetCryptoWorkerJSFileName);

var newDotnetCryptoWorkerJs = new TaskItem(cacheBustedDotNetCryptoWorkerJSFullPath, candidate.CloneCustomMetadata());
newDotnetCryptoWorkerJs.SetMetadata("OriginalItemSpec", candidate.ItemSpec);

var newRelativePath = $"_framework/{cacheBustedDotNetCryptoWorkerJSFileName}";
newDotnetCryptoWorkerJs.SetMetadata("RelativePath", newRelativePath);

newDotnetCryptoWorkerJs.SetMetadata("AssetTraitName", "BlazorWebAssemblyResource");
newDotnetCryptoWorkerJs.SetMetadata("AssetTraitValue", "js-module-crypto");

assetCandidates.Add(newDotnetCryptoWorkerJs);
continue;
}
else if (string.IsNullOrEmpty(destinationSubPath))
{
var relativePath = candidate.GetMetadata("FileName") + candidate.GetMetadata("Extension");
Expand Down Expand Up @@ -280,7 +302,8 @@ public static bool ShouldFilterCandidate(
".dat" when invariantGlobalization && fileName.StartsWith("icudt") => "invariant globalization is enabled",
".json" when fromMonoPackage && (fileName == "emcc-props" || fileName == "package") => $"{fileName}{extension} is not used by Blazor",
".ts" when fromMonoPackage && fileName == "dotnet.d" => "dotnet type definition is not used by Blazor",
".js" when assetType == "native" && fileName != "dotnet" => $"{fileName}{extension} is not used by Blazor",
".ts" when fromMonoPackage && fileName == "dotnet-legacy.d" => "dotnet type definition is not used by Blazor",
".js" when assetType == "native" && fileName != "dotnet" && fileName != "dotnet-crypto-worker" => $"{fileName}{extension} is not used by Blazor",
".pdb" when !copySymbols => "copying symbols is disabled",
".symbols" when fromMonoPackage => "extension .symbols is not required.",
_ => null
Expand Down
48 changes: 41 additions & 7 deletions src/BlazorWasmSdk/Tasks/ComputeBlazorPublishAssets.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,9 @@ private List<ITaskItem> ProcessNativeAssets(
var key = kvp.Key;
var asset = kvp.Value;
var isDotNetJs = IsDotNetJs(key);
var isDotNetCryptoJs = IsDotNetCryptoJs(key);
var isDotNetWasm = IsDotNetWasm(key);
if (!isDotNetJs && !isDotNetWasm)
if (!isDotNetJs && !isDotNetWasm && !isDotNetCryptoJs)
{
if (resolvedNativeAssetToPublish.TryGetValue(Path.GetFileName(asset.GetMetadata("OriginalItemSpec")), out var existing))
{
Expand All @@ -175,6 +176,7 @@ private List<ITaskItem> ProcessNativeAssets(
}
else
{
Log.LogMessage(MessageImportance.Low, "Removing asset '{0}'.", existing.ItemSpec);
// This was a file that was filtered, so just remove it, we don't need to add any publish static web asset
filesToRemove.Add(removed);

Expand Down Expand Up @@ -214,6 +216,34 @@ private List<ITaskItem> ProcessNativeAssets(
continue;
}

if (isDotNetCryptoJs)
{
var aotDotNetCryptoJs = WasmAotAssets.SingleOrDefault(a => $"{a.GetMetadata("FileName")}{a.GetMetadata("Extension")}" == "dotnet-crypto-worker.js");
ITaskItem newDotNetCryptoJs = null;
if (aotDotNetCryptoJs != null)
{
newDotNetCryptoJs = new TaskItem(Path.GetFullPath(aotDotNetCryptoJs.ItemSpec), asset.CloneCustomMetadata());
newDotNetCryptoJs.SetMetadata("OriginalItemSpec", aotDotNetCryptoJs.ItemSpec);
newDotNetCryptoJs.SetMetadata("RelativePath", $"_framework/{$"dotnet-crypto-worker.{DotNetJsVersion}.{FileHasher.GetFileHash(aotDotNetCryptoJs.ItemSpec)}.js"}");

updateMap.Add(asset.ItemSpec, newDotNetCryptoJs);
Log.LogMessage(MessageImportance.Low, "Replacing asset '{0}' with AoT version '{1}'", asset.ItemSpec, newDotNetCryptoJs.ItemSpec);
}
else
{
newDotNetCryptoJs = new TaskItem(asset);
Log.LogMessage(MessageImportance.Low, "Promoting asset '{0}' to Publish asset.", asset.ItemSpec);
}

ApplyPublishProperties(newDotNetCryptoJs);
nativeStaticWebAssets.Add(newDotNetCryptoJs);
if (resolvedNativeAssetToPublish.TryGetValue("dotnet-crypto-worker.js", out var resolved))
{
filesToRemove.Add(resolved);
}
continue;
}

if (isDotNetWasm)
{
var aotDotNetWasm = WasmAotAssets.SingleOrDefault(a => $"{a.GetMetadata("FileName")}{a.GetMetadata("Extension")}" == "dotnet.wasm");
Expand Down Expand Up @@ -252,9 +282,14 @@ private List<ITaskItem> ProcessNativeAssets(
static bool IsDotNetJs(string key)
{
var fileName = Path.GetFileName(key);
return fileName.StartsWith("dotnet.", StringComparison.Ordinal) && fileName.EndsWith(".js", StringComparison.Ordinal);
return fileName.StartsWith("dotnet.", StringComparison.Ordinal) && fileName.EndsWith(".js", StringComparison.Ordinal) && !fileName.Contains("worker");
}

static bool IsDotNetCryptoJs(string key)
javiercn marked this conversation as resolved.
Show resolved Hide resolved
{
var fileName = Path.GetFileName(key);
return fileName.StartsWith("dotnet-crypto-worker.", StringComparison.Ordinal) && fileName.EndsWith(".js", StringComparison.Ordinal);
}
static bool IsDotNetWasm(string key) => string.Equals("dotnet.wasm", Path.GetFileName(key), StringComparison.Ordinal);
}

Expand Down Expand Up @@ -411,7 +446,7 @@ private List<ITaskItem> ProcessCompressedAssets(
Dictionary<string, ITaskItem> updatedAssets)
{
var processed = new List<string>();
var additionalAssetsToUpdate = new List<ITaskItem>();
var runtimeAssetsToUpdate = new List<ITaskItem>();
foreach (var kvp in compressedRepresentations)
{
var compressedAsset = kvp.Value;
Expand All @@ -423,7 +458,7 @@ private List<ITaskItem> ProcessCompressedAssets(
Log.LogMessage(MessageImportance.Low, "Related assembly for '{0}' was not updated and the compressed asset can be reused.", relatedAsset);
var newCompressedAsset = new TaskItem(compressedAsset);
ApplyPublishProperties(newCompressedAsset);
additionalAssetsToUpdate.Add(newCompressedAsset);
runtimeAssetsToUpdate.Add(newCompressedAsset);
}
else
{
Expand All @@ -440,7 +475,7 @@ private List<ITaskItem> ProcessCompressedAssets(
compressedRepresentations.Remove(element);
}

return additionalAssetsToUpdate;
return runtimeAssetsToUpdate;
}

private static void UpdateRelatedAssetProperty(ITaskItem asset, TaskItem newAsset, Dictionary<string, ITaskItem> updatedAssetsMap)
Expand Down Expand Up @@ -603,10 +638,9 @@ private void GroupResolvedFilesToPublish(
}
}

private static bool IsNativeAsset(string traitValue) => string.Equals(traitValue, "native", StringComparison.Ordinal);
private static bool IsNativeAsset(string traitValue) => string.Equals(traitValue, "native", StringComparison.Ordinal) || string.Equals(traitValue, "js-module-crypto", StringComparison.Ordinal);

private static bool IsRuntimeAsset(string traitValue) => string.Equals(traitValue, "runtime", StringComparison.Ordinal);

private static bool IsSymbolAsset(string traitValue) => string.Equals(traitValue, "symbol", StringComparison.Ordinal);

private static bool IsAlternative(ITaskItem asset) => string.Equals(asset.GetMetadata("AssetRole"), "Alternative", StringComparison.Ordinal);
Expand Down
52 changes: 49 additions & 3 deletions src/BlazorWasmSdk/Tasks/GenerateBlazorWebAssemblyBootJson.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.IO;
using System.Linq;
using System.Reflection;
using System.Runtime.Serialization;
using System.Runtime.Serialization.Json;
using System.Text;
using Microsoft.Build.Framework;
Expand Down Expand Up @@ -99,9 +100,11 @@ public void WriteBootJson(Stream output, string entryAssemblyName)
var resourceData = result.resources;
foreach (var resource in Resources)
{
ResourceHashesByNameDictionary resourceList;
ResourceHashesByNameDictionary resourceList = null;

string behavior = null;
var fileName = resource.GetMetadata("FileName");
var fileExtension = resource.GetMetadata("Extension");
var assetTraitName = resource.GetMetadata("AssetTraitName");
var assetTraitValue = resource.GetMetadata("AssetTraitValue");
var resourceName = Path.GetFileName(resource.GetMetadata("RelativePath"));
Expand All @@ -113,7 +116,7 @@ public void WriteBootJson(Stream output, string entryAssemblyName)
resourceData.lazyAssembly ??= new ResourceHashesByNameDictionary();
resourceList = resourceData.lazyAssembly;
}
else if (string.Equals("Culture", assetTraitName))
else if (string.Equals("Culture", assetTraitName, StringComparison.OrdinalIgnoreCase))
{
Log.LogMessage(MessageImportance.Low, "Candidate '{0}' is defined as satellite assembly with culture '{1}'.", resource.ItemSpec, assetTraitValue);
resourceData.satelliteResources ??= new Dictionary<string, ResourceHashesByNameDictionary>(StringComparer.OrdinalIgnoreCase);
Expand Down Expand Up @@ -149,6 +152,12 @@ public void WriteBootJson(Stream output, string entryAssemblyName)
string.Equals(assetTraitValue, "native", StringComparison.OrdinalIgnoreCase))
{
Log.LogMessage(MessageImportance.Low, "Candidate '{0}' is defined as a native application resource.", resource.ItemSpec);
if (string.Equals(fileName, "dotnet", StringComparison.OrdinalIgnoreCase) &&
string.Equals(fileExtension, ".wasm", StringComparison.OrdinalIgnoreCase))
javiercn marked this conversation as resolved.
Show resolved Hide resolved
{
behavior = "dotnetwasm";
}

resourceList = resourceData.runtime;
}
else if (string.Equals("JSModule", assetTraitName, StringComparison.OrdinalIgnoreCase) &&
Expand All @@ -161,6 +170,11 @@ public void WriteBootJson(Stream output, string entryAssemblyName)
Debug.Assert(!string.IsNullOrEmpty(targetPath), "Target path for '{0}' must exist.", resource.ItemSpec);
AddResourceToList(resource, resourceList, targetPath);
continue;
}
else if(string.Equals(assetTraitName, "BlazorWebAssemblyResource", StringComparison.OrdinalIgnoreCase) &&
javiercn marked this conversation as resolved.
Show resolved Hide resolved
string.Equals(assetTraitValue, "js-module-crypto", StringComparison.OrdinalIgnoreCase))
{
behavior = assetTraitValue;
}
else if (string.Equals("BlazorWebAssemblyResource", assetTraitName, StringComparison.OrdinalIgnoreCase) &&
assetTraitValue.StartsWith("extension:", StringComparison.OrdinalIgnoreCase))
Expand All @@ -185,7 +199,16 @@ public void WriteBootJson(Stream output, string entryAssemblyName)
continue;
}

AddResourceToList(resource, resourceList, resourceName);
if (resourceList != null)
{
AddResourceToList(resource, resourceList, resourceName);
}

if (!string.IsNullOrEmpty(behavior))
{
resourceData.runtimeAssets ??= new Dictionary<string, AdditionalAsset>();
AddToAdditionalResources(resource, resourceData.runtimeAssets, resourceName, behavior);
}
}

if (remainingLazyLoadAssemblies.Count > 0)
Expand Down Expand Up @@ -235,9 +258,32 @@ void AddResourceToList(ITaskItem resource, ResourceHashesByNameDictionary resour
}
}

private void AddToAdditionalResources(ITaskItem resource, Dictionary<string, AdditionalAsset> additionalResources, string resourceName, string behavior)
{
if (!additionalResources.ContainsKey(resourceName))
{
Log.LogMessage(MessageImportance.Low, "Added resource '{0}' to the list of additional assets in the manifest.", resource.ItemSpec);
additionalResources.Add(resourceName, new AdditionalAsset
{
Hash = $"sha256-{resource.GetMetadata("FileHash")}",
Behavior = behavior
});
}
}

private bool TryGetLazyLoadedAssembly(string fileName, out ITaskItem lazyLoadedAssembly)
{
return (lazyLoadedAssembly = LazyLoadedAssemblies?.SingleOrDefault(a => a.ItemSpec == fileName)) != null;
}
}

[DataContract]
public class AdditionalAsset
{
[DataMember(Name = "hash")]
public string Hash { get; set; }

[DataMember(Name = "behavior")]
public string Behavior { get; set; }
}
}
Loading