More cleaning up Blazor.Build package #17157

pranavkm · 2019-11-15T22:42:27Z

Clean up MonoRuntime targets
Convert executables in to tasks
Add tests

src/Components/Blazor/Build/src/Microsoft.AspNetCore.Blazor.Build.csproj

pranavkm · 2019-11-15T23:20:12Z

src/Components/Blazor/Build/src/Microsoft.AspNetCore.Blazor.Build.csproj

  </ItemGroup>

+  <Target Name="CopyBuildTask" BeforeTargets="Build" AfterTargets="PrepareForRun">
+    <!-- If you see a build warning here and you mean to update the tasks dll when building in VS, manually stop MSBuild.exe processes. -->


This is unfortunate, but we reference this task from other samples \ apps in this project. MSBuild locks up the file. This "mitigates" it by copying the task to a different location and using that in the reference-from-source. Failures to copy are ignored.

Makes sense.

Does this mean that while MSBuild has it locked, it's possible to build this project, but any changes to the codez won't update the running instance of MSBuild?

Might be better to let the build fail if it's not possible to update, assuming that only happens in the scenario where you did change something.

assuming that only happens in the scenario where you did change something

Unfortunately that's not true. If you build the sln, which causes the tasks to lock up, and then attempt to "Rebuild", you're borked at that point. I generally don't want users to have to deal with killing the MSBuild process unless they actually happen to modifying the tasks (which should be rare).

OK, makes sense. It would still be great to expand the comment in the sources a bit more to capture more of the reasoning here, otherwise I think we're going to keep asking why this works like it does.

src/Components/Blazor/Build/src/ReferenceBlazorBuildFromSource.props

src/Components/Blazor/Build/src/targets/Blazor.MonoRuntime.targets

pranavkm · 2019-11-15T23:33:30Z

src/Components/Blazor/Build/test/BuildIntegrationTests/BuildIntegrationTest.cs

+    public class BuildIntegrationTest
+    {
+        [Fact]
+        public async Task Build_WithDefaultSettings_Works()


src/Components/Blazor/Build/test/BuildIntegrationTests/MSBuildProcessManager.cs

src/Components/Blazor/Build/test/BuildIntegrationTests/ProjectDirectory.cs

pranavkm · 2019-11-15T23:34:13Z

src/Components/Blazor/Build/test/BuildIntegrationTests/PublishIntegrationTest.cs

+    public class PublishIntegrationTest
+    {
+        [Fact]
+        public async Task Publish_WithDefaultSettings_Works()


I ran out of emoticons to express my appreciation for this improvement.

pranavkm · 2019-11-15T23:36:19Z

src/Components/Blazor/Build/testassets/standalone/Program.cs

+{
+    public class Program
+    {
+        public static void Main(string[] args)


Note that this app isn't meant to run. It's got enough things in here (targeting the 3.0 release) to build and publish. But that's enough to verify that the build targets execute and do linking etc.

rynowak · 2019-11-16T00:55:19Z

src/Components/Blazor/Build/src/Tasks/GenerateBlazorBootJson.cs


 namespace Microsoft.AspNetCore.Blazor.Build
 {
-    internal class BootJsonWriter
+    public class GenerateBlazorBootJson : Task


👏 👏 👏 👏 👏

Do we XML-comment this kind of public class?

I don't think so.

rynowak · 2019-11-16T00:55:49Z

src/Components/Blazor/Build/src/Tasks/GenerateBlazorBootJson.cs

        {
+            var assemblyReferences = References.Select(c => Path.GetFileName(c.ItemSpec)).ToArray();


doesn't Msbuild give you this already for ITaskItem ? Isn't that better to use?

We get an ItemSpec which in our case is a path to the assembly. We could query using GetMetadata to get the file name and extension which is what we want here, but this is just as effective.

rynowak · 2019-11-16T00:56:11Z

src/Components/Blazor/Build/src/Tasks/GenerateBlazorBootJson.cs

+                LinkerEnabled);
+
+            var bootJsonText = JsonSerializer.Serialize(data, JsonSerializerOptionsProvider.Options);
+            File.WriteAllText(OutputPath, bootJsonText);


Yeah, we'll have to figure this part out 😢

Could we simply "emit" the json by hand? Given the context this is used in, it seems reasonable (specially if kept simple)

Switched this to DataContractJsonSerializer. My concern with handwriting this is that we now have to worry about the risk of producing bad json (due to escaping etc).

SteveSandersonMS · 2019-11-18T20:56:09Z

src/Components/Blazor/Build/src/Microsoft.AspNetCore.Blazor.Build.csproj


  <PropertyGroup>
-    <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
+    <TargetFramework>netstandard2.0</TargetFramework>
+    <TargetName>Microsoft.AspNetCore.Blazor.Build.Tasks</TargetName>


Is it OK that the assembly name and project/dir name don't match? I mean, I understand it works technically, but this might confuse people as it doesn't match our normal conventions. Could we rename the dir and project to Microsoft.AspNetCore.Blazor.Build.Tasks?

Update: never mind, I realise TargetName is not the assembly name now.

We do something similar with the Razor.SDK. We want to keep the package name Microsoft.AspNetCore.Blazor.Build, but by convention most task assemblies are named 'Something.Tasks`. The task assembly is generally not referenced by any code so it might not be incredibly confusing. Let me know if you feel strongly and we can figure out other options to organize this.

I'm fine with whatever here

src/Components/Blazor/Build/src/Microsoft.AspNetCore.Blazor.Build.csproj

src/Components/Blazor/Build/src/Tasks/ResolveBlazorRuntimeDependencies.cs

SteveSandersonMS · 2019-11-18T21:15:16Z

src/Components/Blazor/Build/src/targets/All.targets

@@ -6,8 +6,8 @@
  </PropertyGroup>

  <PropertyGroup>
-    <BlazorToolsDir Condition="'$(BlazorToolsDir)' == ''">$(MSBuildThisFileDirectory)../tools/</BlazorToolsDir>
-    <BlazorBuildExe>dotnet &quot;$(BlazorToolsDir)Microsoft.AspNetCore.Blazor.Build.dll&quot;</BlazorBuildExe>
+    <BlazorToolsDir Condition="'$(BlazorToolsDir)' == ''">$(MSBuildThisFileDirectory)..\tools\</BlazorToolsDir>


There is some possibility we needed the / due to passing the path into some Mono tooling that only understood those characters. Hopefully that's not the case and the MSBuild standardization is OK.

But if the build starts failing with this change, I hope this is a useful clue why!

So far things look fine. 🤷‍♀

SteveSandersonMS · 2019-11-18T21:18:54Z

src/Components/Blazor/Build/src/targets/Blazor.MonoRuntime.props

    <BlazorBootJsonName>blazor.boot.json</BlazorBootJsonName>
-    <BlazorBootJsonOutputPath>$(BaseBlazorRuntimeOutputPath)$(BlazorBootJsonName)</BlazorBootJsonOutputPath>
+    <_BlazorBuiltInBclLinkerDescriptor>$(MSBuildThisFileDirectory)BuiltInBclLinkerDescriptor.xml</_BlazorBuiltInBclLinkerDescriptor>


Feel free to say you don't want to deal with this question, since it's a bit tangential, but I'll ask now since you're thinking about stuff in this area.

We've had a request from Mono to make it easier to build Blazor apps against a Mono build in a custom directory, instead of the Mono build bundled into our Microsoft.AspNetCore.Blazor.Mono.nupkg.

Given an artifact like https://jenkins.mono-project.com/job/test-mono-mainline-wasm/4237/label=ubuntu-1804-amd64/Azure/processDownloadRequest/4237/ubuntu-1804-amd64/sdks/wasm/mono-wasm-b8fe43da3ed.zip extracted into a folder, could a third-party add some properties to their Blazor application (not in our repo) to set paths that would cause the build to pull in mono.js/wasm and the Mono BCL from that external location?

They want this because it will make it far easier for them to test the effects of Mono changes on Blazor, without having to build Blazor itself from sources.

The ultimate ideal would be adding something like this to your csproj:

<PropertyGroup> <DotNetWebAssemblySDKRoot>c:\me\blah\mono-wasm-b8fe43da3ed\</DotNetWebAssemblySDKRoot> </PropertyGroup>

... and having that alone be sufficient to use this custom build of Mono WebAssembly.

Do you think that's a separate future feature, or is it easy to inline with the work you're doing here?

Named it DotNetWebAssemblyArtifactsRoot. But yes that's pretty easy to do.

SteveSandersonMS · 2019-11-18T21:21:58Z

src/Components/Blazor/Build/src/targets/Publish.targets

-      <_BlazorGCTPDIDistFiles Include="@(BlazorItemOutput->'%(TargetOutputPath)')" />
-      <_BlazorGCTPDI Include="@(_BlazorGCTPDIDistFiles)">
-        <TargetPath>$(BlazorPublishDistDir)$([MSBuild]::MakeRelative('$(TargetDir)dist\', %(Identity)))</TargetPath>
+      <_BlazorGCTPDI Include="%(BlazorOutputWithTargetPath.Identity)">


_BlazorGCTPDI is an extremely obscure name. Could this be renamed to something meaningful?

I believe he is following an obscure convention to improve msbuild performance, its Get Copy To Publish Directory Item which is the item without the associated metadata.

Yup, as @javiercn pointed out, these are named so in the .NET SDK and we're following their poorly thought out naming convention.

src/Components/Blazor/Build/testassets/standalone/App.razor

SteveSandersonMS · 2019-11-18T21:30:00Z

src/Components/Blazor/Build/testassets/standalone/standalone.csproj

+    <!-- We'll only reference a subset of publicly released package versions that are enough to link, build, and publish . -->
+    <PackageReference Include="Microsoft.AspNetCore.Components" Version="3.0.0" />
+    <PackageReference Include="Microsoft.AspNetCore.Components.Web" Version="3.0.0" />
+    <PackageReference Include="Microsoft.AspNetCore.Blazor.Mono" Version="3.1.0-preview3.19531.1" />


Surely we don't want to hardcode any of these version numbers here, do we?

The Blazor.Mono package can be changed to use the version used by the rest of the repository. I'm not sure about the two Components references. We want to avoid referencing the actual projects both in blazor-wasm and in the 5.0 branch to avoid involving arcade's build requirements in our test projects.

In RazorSDK's tests, we use a mix of referencing shipped packages for ones that are already shipped or referencing a runtime shim.

SteveSandersonMS

Overall awesome! Just added some thoughts about removing some of the testasset duplication.

* Clean up MonoRuntime targets * Convert executables in to tasks * Add tests

src/Components/Blazor/Build/testassets/Directory.Build.props

javiercn

Looks great!

pranavkm requested a review from SteveSandersonMS as a code owner November 15, 2019 22:42

pranavkm requested a review from javiercn November 15, 2019 22:42

pranavkm mentioned this pull request Nov 15, 2019

Use System.Reflection.Metadata to generate BootConfig #17156

Merged

pranavkm added the area-blazor Includes: Blazor, Razor Components label Nov 15, 2019

pranavkm requested a review from rynowak November 15, 2019 22:52

pranavkm force-pushed the prkrishn/turn-into-task branch from f6a3578 to ae63f2b Compare November 15, 2019 22:56

pranavkm commented Nov 15, 2019

View reviewed changes

rynowak reviewed Nov 16, 2019

View reviewed changes

SteveSandersonMS reviewed Nov 18, 2019

View reviewed changes

src/Components/Blazor/Build/src/Microsoft.AspNetCore.Blazor.Build.csproj Show resolved Hide resolved

SteveSandersonMS reviewed Nov 18, 2019

View reviewed changes

src/Components/Blazor/Build/src/Tasks/ResolveBlazorRuntimeDependencies.cs Outdated Show resolved Hide resolved

SteveSandersonMS reviewed Nov 18, 2019

View reviewed changes

src/Components/Blazor/Build/testassets/standalone/App.razor Show resolved Hide resolved

SteveSandersonMS reviewed Nov 18, 2019

View reviewed changes

pranavkm force-pushed the prkrishn/more-cleanup branch from f8dd670 to 632f8f3 Compare November 19, 2019 01:03

More cleaning up Blazor.Build package

517a1cb

* Clean up MonoRuntime targets * Convert executables in to tasks * Add tests

pranavkm force-pushed the prkrishn/turn-into-task branch from ae63f2b to 517a1cb Compare November 20, 2019 19:34

pranavkm requested a review from a team as a code owner November 20, 2019 19:34

pranavkm changed the base branch from prkrishn/more-cleanup to blazor-wasm November 20, 2019 19:35

pranavkm added 2 commits November 20, 2019 14:16

Cross-compile the tasks dll

5ff734d

Fixup build failures

c1cc586

pranavkm force-pushed the prkrishn/turn-into-task branch from bb9338f to c1cc586 Compare November 21, 2019 00:51

pranavkm added 2 commits November 20, 2019 22:14

Fixup file casing

589e693

Use configuration

9fb10c6

pranavkm force-pushed the prkrishn/turn-into-task branch from c6a2b0c to 9fb10c6 Compare November 21, 2019 06:37

Use the same toolset that the rest of the projects use

a47d0cf

SteveSandersonMS reviewed Nov 21, 2019

View reviewed changes

src/Components/Blazor/Build/testassets/Directory.Build.props Show resolved Hide resolved

pranavkm force-pushed the prkrishn/turn-into-task branch from b83bd2d to a47d0cf Compare November 21, 2019 18:17

pranavkm added 7 commits November 21, 2019 10:45

Make builds work when rid is specified

561cc63

More fixups!

a80370a

Workaround SDK issue

845f671

Turn off more features

755ebac

Fixup casing

1924d9b

Add some logging

c861818

Build ordering

80b3ef5

javiercn approved these changes Nov 22, 2019

View reviewed changes

pranavkm merged commit d6c88a3 into blazor-wasm Nov 22, 2019

dougbu deleted the prkrishn/turn-into-task branch May 18, 2020 19:46

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

		{
		var assemblyReferences = References.Select(c => Path.GetFileName(c.ItemSpec)).ToArray();

More cleaning up Blazor.Build package #17157

More cleaning up Blazor.Build package #17157

Uh oh!

Conversation

pranavkm commented Nov 15, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

SteveSandersonMS Nov 18, 2019 •

edited

Loading

SteveSandersonMS Nov 18, 2019 •

edited

Loading