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

Split WebAssembly SDK from Blazor SDK #31154

Merged
merged 33 commits into from
Mar 28, 2023
Merged

Split WebAssembly SDK from Blazor SDK #31154

merged 33 commits into from
Mar 28, 2023

Conversation

maraf
Copy link
Member

@maraf maraf commented Mar 13, 2023

  • Overall implementation plan in the [wasm] Tracking issue for build/bundler changes runtime#70762
  • This PR doesn't produce any change in the build output (some will come after integration on the runtime side in following PRs).
    • All blazor properties, items and targets should work as before.
  • All tests remain unchanged (some parts might be moved after runtime side integration).
  • All existing boot config options are generated from wasm boot generator. We will probably move some to blazor config extension after integration with blazor JS side. In the meanwhile new blazor-only flags should go to blazor config extension.
  • Wasm SDK will probably go through some clean up later as we will integrate runtime side.

Contributes to dotnet/aspnetcore#47001

@pavelsavara pavelsavara self-requested a review March 15, 2023 08:14
@maraf maraf requested a review from radical March 15, 2023 16:07
@maraf maraf added Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch and removed Area-Infrastructure labels Mar 16, 2023
@ghost
Copy link

ghost commented Mar 16, 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 marked this pull request as ready for review March 20, 2023 12:30
@maraf maraf requested review from a team and vijayrkn as code owners March 20, 2023 12:30
@maraf maraf changed the title WebAssembly SDK Split WebAssembly SDK from Blazor SDK Mar 20, 2023
@@ -91,7 +92,7 @@ public void GroupsResourcesByType()
("FileHash", "my-custom-extensionhash"),
("RelativePath", "my-custom-extension.blz"),
("TargetPath", "_bin/my-custom-extension.blz"),
("AssetTraitName", "BlazorWebAssemblyResource"),
("AssetTraitName", "WasmResource"),
Copy link
Member

Choose a reason for hiding this comment

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

We need to be careful here because I believe we use this on the service worker. Just making a note here so that I don't forget as I haven't gone through the entire PR

<Project ToolsVersion="14.0" TreatAsLocalProperty="RuntimeIdentifier">
<PropertyGroup>
<!-- FIXME: Important to get rid of missing workload -->
<UsingMicrosoftNETSdkBlazorWebAssembly>true</UsingMicrosoftNETSdkBlazorWebAssembly>
Copy link
Member

Choose a reason for hiding this comment

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

Is this temporary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'll update the property during the migration to runtime repo once I update the wasm-tools workload manifest

src/WasmSdk/Sdk/Sdk.props Outdated Show resolved Hide resolved
/// <summary>
/// "debug" (.pdb) resources
/// </summary>
[DataMember(EmitDefaultValue = false)]
Copy link
Member

Choose a reason for hiding this comment

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

FYI, this can be updated to use System.Text.Json. It used datacontractserializer because it predates it, but I did support to add System.Text.Json support to the SDK tasks, so you should be able to use it. (It might be that I only did it in the RazorSDK, but it should be "copiable")

Copy link
Member

Choose a reason for hiding this comment

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

Not asking you to change it, just letting you know

Copy link
Member Author

@maraf maraf Mar 22, 2023

Choose a reason for hiding this comment

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

I would love to do it. I think I struggled on the package version when I tried it last week.

So you are saying that I can use version 6.0 like in here https://github.com/dotnet/razor/blob/33e06ed0fd64c8b67b80d464370ee0380f9ab32c/eng/Versions.props#L179, right?

Copy link
Member

Choose a reason for hiding this comment

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

@@ -6,7 +6,7 @@
using System.Security.Cryptography;
using System.Text;

namespace Microsoft.NET.Sdk.BlazorWebAssembly
namespace Microsoft.NET.Sdk.WebAssembly
{
public static class FileHasher
Copy link
Member

Choose a reason for hiding this comment

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

@MackinnonBuck this will move to static web assets as part of dotnet/aspnetcore#47000

# Conflicts:
#	src/BlazorWasmSdk/Targets/Microsoft.NET.Sdk.BlazorWebAssembly.6_0.targets
#	src/BlazorWasmSdk/Tasks/BootJsonData.cs
#	src/WasmSdk/Tasks/GenerateWasmBootJson.cs
# Conflicts:
#	src/BlazorWasmSdk/Targets/Microsoft.NET.Sdk.BlazorWebAssembly.6_0.targets
#	src/WasmSdk/Tasks/ComputeWasmBuildAssets.cs
#	src/WasmSdk/Tasks/ComputeWasmPublishAssets.cs
#	src/WasmSdk/Tasks/GenerateWasmBootJson.cs
@maraf maraf merged commit 3e34299 into dotnet:main Mar 28, 2023
@maraf maraf deleted the WasmSdk branch March 28, 2023 13:48
radical added a commit to radical/sdk that referenced this pull request May 25, 2023
`Microsoft.NET.Sdk.BlazorWebAssembly.Current.targets` unconditionally
imports:

```xml
  <Import Sdk="Microsoft.NET.Sdk.Razor" Project="Sdk.targets" />
  <Import Sdk="Microsoft.NET.Sdk.Web.ProjectSystem" Project="Sdk.targets" />
  <Import Sdk="Microsoft.NET.Sdk.Publish" Project="Sdk.targets" />
```

.. *after* which it can import
`Microsoft.NET.Sdk.BlazorWebAssembly.5_0.targets`, which then
unconditionally imports:

```xml
  <Import Sdk="Microsoft.NET.Sdk.Razor" Project="Sdk.targets" />
  <Import Sdk="Microsoft.NET.Sdk.Web.ProjectSystem" Project="Sdk.targets" />
  <Import Sdk="Microsoft.NET.Sdk.Publish" Project="Sdk.targets" />
```

.. which produces warning:

```
[2023/05/24 07:06:03][INFO] /home/helixbot/work/BAA509D0/p/dotnet/sdk/8.0.100-preview.5.23273.2/Sdks/Microsoft.NET.Sdk.BlazorWebAssembly/targets/Microsoft.NET.Sdk.BlazorWebAssembly.5_0.targets(13,3): warning MSB4011: "/home/helixbot/work/BAA509D0/p/dotnet/sdk/8.0.100-preview.5.23273.2/Sdks/Microsoft.NET.Sdk.Razor/Sdk/Sdk.targets" cannot be imported again. It was already imported at "/home/helixbot/work/BAA509D0/p/dotnet/sdk/8.0.100-preview.5.23273.2/Sdks/Microsoft.NET.Sdk.BlazorWebAssembly/targets/Microsoft.NET.Sdk.BlazorWebAssembly.Current.targets (25,3)". This is most likely a build authoring error. This subsequent import will be ignored.
```

.. and the same for `Microsoft.NET.Sdk.Web.ProjectSystem`, and `Microsoft.NET.Sdk.Publish`.

Since the `5_0.targets` is only ever imported from the
`Current.targets`, it does not need to import the same SDKs again.

This was introduced in:
```
commit 3e34299
Author: Marek Fišera <mara@neptuo.com>
Date:   Tue Mar 28 15:47:55 2023 +0200

    Split WebAssembly SDK from Blazor SDK (dotnet#31154)
```

.. and the blazor scenario perf runs have been broken since it got
merged.
radical added a commit that referenced this pull request May 25, 2023
`Microsoft.NET.Sdk.BlazorWebAssembly.Current.targets` unconditionally imports:

```xml
  <Import Sdk="Microsoft.NET.Sdk.Razor" Project="Sdk.targets" />
  <Import Sdk="Microsoft.NET.Sdk.Web.ProjectSystem" Project="Sdk.targets" />
  <Import Sdk="Microsoft.NET.Sdk.Publish" Project="Sdk.targets" />
```

.. *after* which it can import
`Microsoft.NET.Sdk.BlazorWebAssembly.5_0.targets`, which then unconditionally imports:

```xml
  <Import Sdk="Microsoft.NET.Sdk.Razor" Project="Sdk.targets" />
  <Import Sdk="Microsoft.NET.Sdk.Web.ProjectSystem" Project="Sdk.targets" />
  <Import Sdk="Microsoft.NET.Sdk.Publish" Project="Sdk.targets" />
```

.. which produces warning:

```
[2023/05/24 07:06:03][INFO] /home/helixbot/work/BAA509D0/p/dotnet/sdk/8.0.100-preview.5.23273.2/Sdks/Microsoft.NET.Sdk.BlazorWebAssembly/targets/Microsoft.NET.Sdk.BlazorWebAssembly.5_0.targets(13,3): warning MSB4011: "/home/helixbot/work/BAA509D0/p/dotnet/sdk/8.0.100-preview.5.23273.2/Sdks/Microsoft.NET.Sdk.Razor/Sdk/Sdk.targets" cannot be imported again. It was already imported at "/home/helixbot/work/BAA509D0/p/dotnet/sdk/8.0.100-preview.5.23273.2/Sdks/Microsoft.NET.Sdk.BlazorWebAssembly/targets/Microsoft.NET.Sdk.BlazorWebAssembly.Current.targets (25,3)". This is most likely a build authoring error. This subsequent import will be ignored.
```

.. and the same for `Microsoft.NET.Sdk.Web.ProjectSystem`, and `Microsoft.NET.Sdk.Publish`.

Since the `5_0.targets` is only ever imported from the `Current.targets`, it does not need to import the same SDKs again.

This was introduced in:
```
commit 3e34299
Author: Marek Fišera <mara@neptuo.com>
Date:   Tue Mar 28 15:47:55 2023 +0200

    Split WebAssembly SDK from Blazor SDK (#31154)
```

.. and the blazor scenario perf runs have been broken since it got merged.
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.

None yet

2 participants