Replat blazor on net5 #23078

pranavkm · 2020-06-18T07:17:12Z

Still need to get through some test issues, but the apps work now without issues and the sdk tests are passing locally.

Things pending

Revive gzip compression (TBD based on discussion)
Fix skipped tests
~~Scramble the dotnet.js file name~~
~~Figure out packing for blazor.webassembly.js~~
~~Update templates to net5~~

The largest issue I noticed since the sync up today is that RuntimeIdentifer is required to be specified in the project file. The Razor SDK is imported too late to set this up correctly. ~~I'm going to talk to the SDK folks to figure out how to not have this in the project file. Ignore that for now.~~ We had a conversation with some folks about possibly having a Blazor WASM specific SDK. This should remedy the problem. I'll hold off on this for now.

src/Components/Components.sln

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts

pranavkm · 2020-06-19T20:20:42Z

src/Components/WebAssembly/testassets/MonoSanity/MonoSanity.csproj

@@ -1,16 +0,0 @@
-<Project Sdk="Microsoft.NET.Sdk.Web">


I didn't bring this app forward. It seems low value and requires additional work to get the loader script functioning. Let me know if you feel strongly about keeping this.

I think it's great to remove this 👍

This project (and its tests) was created before Blazor, so we had some way to make sense of how .NET code should behave on WebAssembly, and so that if the runtime behaviors around low-level basics like exception handling, boxing, etc. varied then we'd have a way to observe that objectively.

By now, .NET on WebAssembly is well enough established that we don't have to get independent proof that the low-level basics work correctly!

...mponents/WebAssembly/testassets/Wasm.Authentication.Server/Wasm.Authentication.Server.csproj

pranavkm · 2020-06-19T20:21:52Z

src/Components/test/E2ETest/Tests/WebAssemblyAuthenticationTests.cs

@@ -63,13 +63,13 @@ protected override void InitializeAsyncCore()
            WaitUntilLoaded();
        }

-        [Fact]
+        [Fact(Skip = "https://github.com/dotnet/runtime/issues/38098")]


Async JSInterop code paths are busted because TCS.SetResult throws

src/Components/test/E2ETest/Tests/WebAssemblyLocalizationTest.cs

pranavkm · 2020-06-19T20:37:39Z

src/Razor/Microsoft.NET.Sdk.Razor/src/GenerateServiceWorkerAssetsManifest.cs

+
+namespace Microsoft.AspNetCore.Razor.Tasks
+{
+    public partial class GenerateServiceWorkerAssetsManifest : Task


Similar to boot.json, we operate on a different set of inputs during build and publish. This task has been modified to do what was previously done across multiple msbuild targets. I need to spend a little bit of time to verify, but it should be just as incremental as before.

That's great. Do you feel the test coverage we have for this is adequate? We've had multiple regressions related to SWAM and build/publish/incrementalism, perhaps because it's pretty hard to tell whether it's right just by casually running an app.

I retained all the tests from before. My plan's to go over these again and see if there are gaps and see if we could have more combination of tests (VS-style publish + build incrementalism for instance)

...rosoft.NET.Sdk.Razor/src/build/netstandard2.0/Microsoft.NET.Sdk.Razor.CodeGeneration.targets

pranavkm · 2020-06-19T20:39:48Z

...osoft.NET.Sdk.Razor/src/build/netstandard2.0/Microsoft.NET.Sdk.Razor.StaticWebAssets.targets

@@ -513,7 +513,7 @@ Copyright (c) .NET Foundation. All rights reserved.
       -->
      <_ExternalPublishStaticWebAsset
        Include="%(StaticWebAsset.FullPath)"
-        Condition="'%(SourceType)' != ''">
+        Condition="'%(SourceType)' != '' AND '%(StaticWebAsset.CopyToPublishDirectory)' != 'Never'">


We don't want assets such as the boot.json produced during build to be copied to the publish directory. There was a bug in these targets where it wouldn't honor the CopyToPublishDirectory setting.

We shouldn't leave random stuff in the SWA definition. What problem are we trying to solve here? I'm not sure what boot.json you are referring to, blazor.boot.json? How is that not going to be an SWA?

pranavkm · 2020-06-19T20:39:57Z

src/Razor/Microsoft.NET.Sdk.Razor/src/build/netstandard2.0/Microsoft.NET.Sdk.Razor.props

@@ -14,4 +14,5 @@ Copyright (c) .NET Foundation. All rights reserved.
    <RazorSdkCurrentVersionProps>$(MSBuildThisFileDirectory)Sdk.Razor.CurrentVersion.props</RazorSdkCurrentVersionProps>
    <RazorSdkCurrentVersionTargets>$(MSBuildThisFileDirectory)Sdk.Razor.CurrentVersion.targets</RazorSdkCurrentVersionTargets>
  </PropertyGroup>
-</Project>
+
+</Project>


pranavkm · 2020-06-19T20:43:25Z

src/Razor/Microsoft.NET.Sdk.Razor/src/build/netstandard2.0/Sdk.Razor.CurrentVersion.targets

@@ -354,6 +356,8 @@ Copyright (c) .NET Foundation. All rights reserved.

  <Import Project="Microsoft.NET.Sdk.Razor.MvcApplicationPartsDiscovery.targets" Condition="'$(_TargetingNETCoreApp30OrLater)' == 'true'" />

+  <Import Project="Microsoft.NET.Sdk.Razor.Components.Wasm.targets" Condition="'$(_TargetingNET50OrLater)' == 'true' AND '$(UseBlazorWebAssembly)' == 'true'" />


This target gets imported far too late to set up useful defaults such as RuntimeIdentifier or the flag that tells the SDK not to append the rid to the output path. We are able to do other things such as tell it to not reference M.AspNetCore.App or do trimmed publish. One way to remedy the evaluation order issue is to add a blazor-wasm specific SDK.

Pilchie · 2020-06-20T23:10:20Z

👀

pranavkm · 2020-06-22T23:46:27Z

eng/Versions.props

@@ -66,69 +66,69 @@
    <!-- Packages from dotnet/roslyn -->
    <MicrosoftNetCompilersToolsetPackageVersion>3.7.0-4.20319.6</MicrosoftNetCompilersToolsetPackageVersion>
    <!-- Packages from dotnet/runtime -->
-    <MicrosoftExtensionsDependencyModelPackageVersion>5.0.0-preview.7.20321.2</MicrosoftExtensionsDependencyModelPackageVersion>


Please ignore. I'll rebase this once darc updates this file. Using this to verify that the latest build resolves outstanding issues with the wasm runtime.

dougbu · 2020-06-23T00:17:02Z

@pranavkm I got added as a reviewer due to code ownership I believe. And, you're going to revert the changes to the file I kinda own. Is there anything here you'd like me to review?

Curiosity only: Does this help move us any closer to archiving the dotnet/blazor repo?

SteveSandersonMS · 2020-06-23T13:25:57Z

src/Components/WebAssembly/testassets/StandaloneApp/StandaloneApp.csproj

    <FixupWebAssemblyHttpHandlerReference>true</FixupWebAssemblyHttpHandlerReference>
  </PropertyGroup>

  <ItemGroup>
    <Reference Include="Microsoft.AspNetCore.Components.WebAssembly" />
    <Reference Include="System.Net.Http.Json" />
+
+    <FrameworkReference Remove="Microsoft.AspNetCore.App" />


Could you explain what this <FrameworkReference> is for? Since StandaloneApp runs on WebAssembly, I wouldn't expect there to be references to anything from server-side ASP.NET Core.

This could be removed from the project file since the SDK removes this for you. The project targets the WebSDK and net5.0, you get an implicit reference to this framework. I filed #23259 to figure out a better way to do this.

src/Components/test/E2ETest/Tests/WebAssemblyGlobalizationTest.cs

src/Components/test/E2ETest/Tests/WebAssemblyStringComparisonTest.cs

src/Components/test/testassets/BasicTestApp/Program.cs

src/Components/test/testassets/BasicTestApp/wwwroot/js/jsinteroptests.js

src/ProjectTemplates/BlazorTemplates.Tests/BlazorWasmTemplateTest.cs

src/Razor/Microsoft.AspNetCore.Razor.Tools/src/BrotliCompressCommand.cs

SteveSandersonMS · 2020-06-23T13:34:35Z

src/Razor/Microsoft.AspNetCore.Razor.Tools/src/BrotliCompressCommand.cs

+        {
+            Sources = Option("-s", ".cshtml files to compile", CommandOptionType.MultipleValue);
+            Outputs = Option("-o", "Generated output file path", CommandOptionType.MultipleValue);
+            CompressionLevelOption = Option("-c", "Compression level", CommandOptionType.SingleValue);


I don't really feel strongly about removing this, but I'm not aware we have any use case for having an option. We always want to use max AFAIK.

They get turned down in the Razor integration tests. It makes the tests take far too long if they perform maximal compression every time.

SteveSandersonMS · 2020-06-23T13:38:32Z

src/Razor/Microsoft.NET.Sdk.Razor/integrationtests/Wasm/WasmBuildIntegrationTest.cs

+            Assert.Null(bootJsonData.resources.satelliteResources);
+        }
+
+        [Fact(Skip = "https://github.com/dotnet/aspnetcore/issues/22975")]


To be tracked

This one's our issue.

src/Razor/Microsoft.NET.Sdk.Razor/integrationtests/Wasm/WasmBuildIntegrationTest.cs

src/Razor/Microsoft.NET.Sdk.Razor/src/RazorGenerate.cs

SteveSandersonMS · 2020-06-23T13:47:21Z

src/Razor/Microsoft.NET.Sdk.Razor/src/build/netstandard2.0/LinkerWorkaround.xml

@@ -19,6 +13,8 @@
    <type fullname="System.ComponentModel.TimeSpanConverter" />
  </assembly>

+  <assembly fullname="System.Runtime.InteropServices.JavaScript" preserve="all" />
+
  <assembly fullname="System.Text.Json">


We might be able to drop this now (as it was working around an issue in STJ that might be fixed). Perfectly fine if you don't want to mess with extra stuff in this PR of course though!

I filed #23262 to see if we can get these resolved by the BCL.

.../build/netstandard2.0/Microsoft.NET.Sdk.Razor.Components.ServiceWorkerAssetsManifest.targets

SteveSandersonMS · 2020-06-23T13:49:31Z

...osoft.NET.Sdk.Razor/src/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Components.Wasm.targets

@@ -0,0 +1,482 @@
+<!--


Since there's no diff associated with this file, it's pretty hard to CR and reason about what's changing.

Do you think you'd be able to summarize the high-level points about what's being done differently here now?

The "Build" part of the pipeline is pretty close to what we had in Blazor.MonoRuntime.targets. We gather inputs, generate a blazor.boot.json, optionally a server worker manifest, add things to StaticWebAssets, and copy files to $(OutputPath)wwwroot. We no longer run the linker or compression during, so those parts of the pipeline are removed.

When publishing standalone, the linker operates on ResolvedFileToPublish. These files are inputs to compression, published PWA manifest, and the published blazor.boot.json. It's pretty much like build, but with different inputs. To make our lives easier, rather than the targets calculating values (such as BootManifestResourceType), the tasks do it. This saves us from duplicating a lot of MSBuild code between the build and publish targets.

Publishing P2P is a bit more interesting. A dotnet publish on a hosted server project, only results in a dotnet build on the referenced client project. This is how MSBuild works. But we want this to produce a "published" WASM app. There is an escape hatch though. MSBuild also calls GetCopyToPublishDirectoryItems on referenced projects to get files to copy to the server's outputs. We use this to do a publish on the client project and copy them over to the server.

SteveSandersonMS · 2020-06-23T13:51:25Z

src/Razor/Microsoft.NET.Sdk.Razor/test/GenerateBlazorBootJsonTest.cs

    {
-        [Fact]
+        [Fact(Skip = "TODO fix this")]


Not sure if you're planning to handle this as part of the PR or later. If later, could you add it to the list of things to be tracked for follow-up? Presumably we could have one big issue listing them all.

SteveSandersonMS · 2020-06-23T13:52:33Z

src/Razor/test/testassets/Directory.Build.props

@@ -26,6 +27,9 @@

    <!-- Working around an issue in XDT transforms -->
    <AspNetCoreHostingModel>OutOfProcess</AspNetCoreHostingModel>
+
+    <!-- Turn down the compression level for brotli -->
+    <_BlazorBrotliCompressionLevel>NoCompression</_BlazorBrotliCompressionLevel>


Why? To make CI builds faster? Does it make enough of a difference to be worthwhile?

Also, our benchmarking relies on getting realistic brotli-compressed sizes. Just want to check this isn't going to interfere.

This is specifically for the integration tests. It takes kinda long: #21382

SteveSandersonMS

This is looking really great! Thanks for figuring out all these details. It's cool to see things getting simpler as part of this process.

Not sure how close you are to considering this PR done so I'll await a further ping when you're ready for sign-off.

javiercn · 2020-06-23T15:04:06Z

src/Components/WebAssembly/testassets/HostedInAspNet.Client/HostedInAspNet.Client.csproj

-    <!-- Disable compression in this project so that we can validate that it can be disabled -->
-    <BlazorEnableCompression>false</BlazorEnableCompression>
+    <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
+    <RuntimeIdentifier>browser-wasm</RuntimeIdentifier>


Did we get around the publish hosted bit by forcing the RID?

I thought I'd tried this, but there's an issue where dotnet publish -r [rid] doesn't work. The restore that runs as part of publish messes things up. I'll talk to SDK folks about this and see what they recommend doing here.

javiercn · 2020-06-23T15:09:21Z

src/ProjectTemplates/Shared/Project.cs

-            var razorSDKarg = string.IsNullOrEmpty(RazorSdkDirectoryRoot) ? string.Empty : $"/p:RazorSdkDirectoryRoot={RazorSdkDirectoryRoot}";
-
-            using var result = ProcessEx.Run(Output, TemplateOutputDir, DotNetMuxer.MuxerPathOrDefault(), $"build --no-restore -c Debug /bl {razorSDKarg} {additionalArgs}", packageOptions);
+            using var result = ProcessEx.Run(Output, TemplateOutputDir, DotNetMuxer.MuxerPathOrDefault(), $"build --no-restore -c Debug /bl {additionalArgs}", packageOptions);


Why do we need the rid now here?

Further up? WASM produces outputs in a RID based directory. There's a flag to turn it off, but does not work: dotnet/sdk#12114

javiercn · 2020-06-23T15:11:10Z

src/Razor/Microsoft.AspNetCore.Razor.Tools/src/Application.cs

@@ -39,6 +39,7 @@ internal class Application : CommandLineApplication
            Commands.Add(new ShutdownCommand(this));
            Commands.Add(new DiscoverCommand(this));
            Commands.Add(new GenerateCommand(this));
+            Commands.Add(new BrotliCompressCommand(this));


Does this ties compression to the Razor compiler?

To the rzc tool, but it does not remote calls to the server process. It doesn't seem problematic as yet, but we can come back to this if there are side-effects to it.

javiercn · 2020-06-23T15:25:14Z

...osoft.NET.Sdk.Razor/src/build/netstandard2.0/Microsoft.NET.Sdk.Razor.StaticWebAssets.targets

@@ -269,7 +269,7 @@ Copyright (c) .NET Foundation. All rights reserved.
    <ItemGroup>
      <StaticWebAsset
        Include="@(_ReferencedProjectStaticWebAssets)"
-        KeepMetadata="ContentRoot;BasePath;RelativePath;SourceId;SourceType" />
+        KeepMetadata="ContentRoot;BasePath;RelativePath;SourceId;SourceType;CopyToPublishDirectory" />


SWAs should only have ContentRoot;BasePath;RelativePath;SourceId;SourceType I want to avoid them becoming a random bag of properties.

javiercn

Looks great!

I want us to discuss the issue with SWA, but other than that looks fine.

…0622.6 System.ComponentModel.Annotations , System.Diagnostics.DiagnosticSource , System.Diagnostics.EventLog , Microsoft.Extensions.Logging.Configuration , Microsoft.Extensions.Logging.Console , Microsoft.Extensions.Logging.Debug , Microsoft.Extensions.Logging.EventLog , Microsoft.Extensions.Logging.EventSource , Microsoft.Extensions.Logging.TraceSource , Microsoft.Extensions.Options , Microsoft.Extensions.Options.ConfigurationExtensions , Microsoft.Extensions.Options.DataAnnotations , Microsoft.Extensions.Primitives , Microsoft.Extensions.Logging.Abstractions , Microsoft.Extensions.Logging , Microsoft.Extensions.Internal.Transport , Microsoft.Extensions.Http , Microsoft.Extensions.Caching.Abstractions , Microsoft.Extensions.Caching.Memory , Microsoft.Extensions.Configuration , Microsoft.Extensions.Configuration.Abstractions , Microsoft.Extensions.Configuration.Binder , Microsoft.Extensions.Configuration.CommandLine , Microsoft.Extensions.Configuration.EnvironmentVariables , Microsoft.Extensions.Configuration.FileExtensions , Microsoft.Extensions.Configuration.Ini , Microsoft.Extensions.Configuration.Json , Microsoft.Extensions.Configuration.UserSecrets , Microsoft.Extensions.DependencyInjection , Microsoft.Extensions.DependencyInjection.Abstractions , Microsoft.Extensions.DependencyModel , Microsoft.Extensions.FileProviders.Abstractions , Microsoft.Extensions.FileProviders.Composite , Microsoft.Extensions.FileProviders.Physical , Microsoft.Extensions.FileSystemGlobbing , Microsoft.Extensions.HostFactoryResolver.Sources , Microsoft.Extensions.Hosting , Microsoft.Extensions.Hosting.Abstractions , Microsoft.Extensions.Configuration.Xml , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.Platforms , Microsoft.Win32.Registry , Microsoft.Win32.SystemEvents , Microsoft.NETCore.App.Internal , Microsoft.NETCore.App.Ref , System.Drawing.Common , System.ServiceProcess.ServiceController , System.Text.Encodings.Web , System.Text.Json , System.Threading.Channels , System.Windows.Extensions , System.Security.Principal.Windows , System.Security.Permissions , System.Security.Cryptography.Xml , System.IO.Pipelines , System.Net.Http.Json , System.Net.Http.WinHttpHandler , System.Net.WebSockets.WebSocketProtocol , System.Reflection.Metadata , System.Runtime.CompilerServices.Unsafe , System.Security.Cryptography.Cng , System.Security.Cryptography.Pkcs From Version 5.0.0-preview.7.20321.2 -> To Version 5.0.0-preview.7.20322.6

mkArtakMSFT

Let's get this merged so we can unblock our partners.
Please address any follow-up feedback through separate PRs, as this is pretty large already.

pranavkm requested a review from a team June 18, 2020 07:17

captainsafia reviewed Jun 18, 2020

View reviewed changes

src/Components/Components.sln Show resolved Hide resolved

Pilchie added the area-blazor Includes: Blazor, Razor Components label Jun 18, 2020

pranavkm force-pushed the prkrishn/net5-sdk branch from 23f896d to 3caf917 Compare June 19, 2020 20:04

pranavkm requested a review from a team June 19, 2020 20:11

pranavkm commented Jun 19, 2020

View reviewed changes

pranavkm force-pushed the prkrishn/net5-sdk branch from 3caf917 to 5cbfd78 Compare June 19, 2020 23:44

pranavkm marked this pull request as ready for review June 20, 2020 00:06

pranavkm requested a review from SteveSandersonMS as a code owner June 20, 2020 00:06

pranavkm added this to the 5.0.0-preview7 milestone Jun 21, 2020

pranavkm force-pushed the prkrishn/net5-sdk branch from 775b2b2 to f6a0514 Compare June 22, 2020 23:44

pranavkm requested a review from dougbu as a code owner June 22, 2020 23:44

pranavkm changed the base branch from master to release/5.0-preview7 June 22, 2020 23:44

pranavkm commented Jun 22, 2020

View reviewed changes