[release/6.0-preview7] Build VS installers for WebAssembly and Mobile workloads (#55769) #56127

steveisok · 2021-07-22T02:31:55Z

Backport of #55769 to release/6.0-preview7.

Customer Impact

Without this PR, customers will not be able to install mono workloads (wasm, iOS/tvOS/MacCatalyst, and Android) workloads into Visual Studio

Testing

We validated the packages produced are satisfactory to be inserted into VS.

Risk

Very low.

In order to support generating installers, this change adds the mono.workloads subset and the associated yml.

ghost · 2021-07-22T02:32:02Z

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #55769 to release/6.0-preview7.

Customer Impact

Without this PR, customers will not be able to install mono workloads (wasm, iOS/tvOS/MacCatalyst, and Android) workloads into Visual Studio

Testing

We validated the packages produced are satisfactory to be inserted into VS.

Risk

Very low.

Author:	steveisok
Assignees:	-
Labels:	`area-Infrastructure-mono`
Milestone:	-

ViktorHofer · 2021-07-22T05:47:49Z

eng/Versions.props

+    <MicrosoftDotNetBuildTasksInstallersPackageVersion>6.0.0-beta.21369.3</MicrosoftDotNetBuildTasksInstallersPackageVersion>
    <MicrosoftDotNetBuildTasksInstallersVersion>6.0.0-beta.21364.3</MicrosoftDotNetBuildTasksInstallersVersion>


Why does this property now exist twice once with the PackageVersion suffix (which is the pattern we don't use in this repo) and once with the Version suffix?

ViktorHofer · 2021-07-22T05:49:37Z

src/workloads/VSSetup.props

@@ -0,0 +1,42 @@
+<?xml version="1.0" encoding="utf-8"?>


You can remove this xml section as it's not needed by msbuild and we usually don't have it in our msbuild files in the repo.

ViktorHofer · 2021-07-22T05:49:54Z

src/workloads/VSSetup.props

@@ -0,0 +1,42 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">


You can also remove the ToolsVersion and the xmlns attributes.

ViktorHofer · 2021-07-22T05:50:31Z

src/workloads/VSSetup.props

+
+    <PropertyGroup>
+        <ManifestTeamProject Condition="'$(ManifestTeamProject)' == ''">dotnet</ManifestTeamProject>
+        <ManifestRepositoryName Condition="'$(ManifestRepositoryName)' == ''">installer</ManifestRepositoryName>


Shouldn't the name of the repo be runtime?

ViktorHofer · 2021-07-22T05:51:00Z

src/workloads/VSSetup.targets

+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">


Same comment here about the xml line and the attributes.

ViktorHofer · 2021-07-22T05:52:20Z

src/workloads/VSSetup.targets

@@ -0,0 +1,49 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+    <Import Project="VSSetup.props" Condition="'$(VSSetupProps)' != '1'"/>


Why do you need the condition? Does anyone else import this file?

Why does the targets file import the props file? Usually the project imports the props and the targets file.

ViktorHofer · 2021-07-22T05:52:55Z

src/workloads/readme.md

@@ -0,0 +1,2 @@
+# Building
+The workloads project can only be built using .NET Framework msbuild. To build locally, run ```build -project src\workloads\workloads.csproj -msbuildEngine vs```


You could also mention that alternatively you can use the Developer Command Prompt shell on Windows.

ViktorHofer · 2021-07-22T05:54:32Z

src/workloads/workloads.csproj

@@ -0,0 +1,185 @@
+<Project DefaultTargets="Restore;Build">


Restore and Build targets should never run in the same node and share their evaluation as restore impacts properties and items in the Build target's evaluation.

I don't see why this line at all is necessary, I would just remove the DefaultTargets.

When an msbuild project doesn't compile any C# code it shouldn't be named .csproj. Instead we use the .proj suffix for such.

Also I recommend to use the Microsoft.Build.NoTargets Sdk here instead which allows to override the Build target in a less hacky way.

ViktorHofer · 2021-07-22T05:58:28Z

src/workloads/workloads.csproj

+  <PropertyGroup>
+    <MicrosoftDotNetBuildTasksInstallersTaskTargetFramework Condition="'$(MSBuildRuntimeType)' == 'Core'">netcoreapp3.1</MicrosoftDotNetBuildTasksInstallersTaskTargetFramework>
+    <MicrosoftDotNetBuildTasksInstallersTaskTargetFramework Condition="'$(DotNetBuildFromSource)' == 'true'">net5.0</MicrosoftDotNetBuildTasksInstallersTaskTargetFramework>
+    <MicrosoftDotNetBuildTasksInstallersTaskTargetFramework Condition="'$(MSBuildRuntimeType)' != 'Core'">net472</MicrosoftDotNetBuildTasksInstallersTaskTargetFramework>
+    <MicrosoftDotNetBuildTasksInstallersTaskAssembly>$(NuGetPackageRoot)microsoft.dotnet.build.tasks.installers\$(MicrosoftDotNetBuildTasksInstallersPackageVersion)\tools\$(MicrosoftDotNetBuildTasksInstallersTaskTargetFramework)\Microsoft.DotNet.Build.Tasks.Installers.dll</MicrosoftDotNetBuildTasksInstallersTaskAssembly>
+  </PropertyGroup>


Don't hardcode the path to the installers task assembly as that's subject to change, i.e. when the TargetFrameworks are changed.

Instead I would just use the MicrosoftDotNetBuildTasksInstallersTaskAssembly property which is already exposed by the package here: https://github.com/dotnet/arcade/blob/828107daa2dd84171c328b9d2b58009984ee4c39/src/Microsoft.DotNet.Build.Tasks.Installers/build/Microsoft.DotNet.Build.Tasks.Installers.props#L3-L4

ViktorHofer · 2021-07-22T05:59:11Z

src/workloads/workloads.csproj

+    <MicrosoftDotNetBuildTasksInstallersTaskAssembly>$(NuGetPackageRoot)microsoft.dotnet.build.tasks.installers\$(MicrosoftDotNetBuildTasksInstallersPackageVersion)\tools\$(MicrosoftDotNetBuildTasksInstallersTaskTargetFramework)\Microsoft.DotNet.Build.Tasks.Installers.dll</MicrosoftDotNetBuildTasksInstallersTaskAssembly>
+  </PropertyGroup>
+
+  <UsingTask AssemblyFile="$(PkgMicrosoft_DotNet_Build_Tasks_Workloads)\tools\net472\Microsoft.DotNet.Build.Tasks.Workloads.dll" TaskName="GenerateVisualStudioWorkload" />


Same here. The Workloads package in Arcade should expose a property that points to its task assembly and not the consumer.

ViktorHofer · 2021-07-22T06:01:32Z

eng/pipelines/mono/templates/workloads-build.yml

+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.android-*.nupkg
+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.browser-wasm*.nupkg
+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NETCore.App.Runtime.Mono.android-*.nupkg
+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NETCore.App.Runtime.Mono.browser-wasm*.nupkg
+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NETCore.App.Runtime.Mono.ios-*.nupkg
+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NETCore.App.Runtime.Mono.iossimulator-*.nupkg
+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NETCore.App.Runtime.Mono.maccatalyst-*.nupkg
+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NETCore.App.Runtime.Mono.tvos-*.nupkg
+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NETCore.App.Runtime.Mono.tvossimulator-*.nupkg
+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NET.Workload.Mono.ToolChain.Manifest*.nupkg
+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NET.Runtime.MonoTargets.Sdk*.nupkg
+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NET.Runtime.MonoAOTCompiler.Task*.nupkg
+          IntermediateArtifacts/MonoRuntimePacks/Shipping/Microsoft.NET.Runtime.WebAssembly.Sdk*.nupkg


Can you think of a way to avoid hardcoding all these packages? Would creating a new intermediate artifact only for the mono runtime packs make sense?

The purpose of the hardcoded list is to speed up builds, by only downloading the required artifacts out of the build. Just grabbing the whole MonoRuntimePacks folder weighs in at around 4GiB

steveisok · 2021-07-22T14:59:43Z

This has been approved by Steve over teams/email.

steveisok · 2021-07-22T15:47:29Z

@ViktorHofer Appreciate the review. Generating the vs installer files is pretty time sensitive, so I think all your items can be taken care of in a follow up.

steveisok · 2021-07-22T15:58:27Z

official build is running at https://dev.azure.com/dnceng/internal/_build/results?buildId=1253166&view=results

Anipik · 2021-07-22T19:40:24Z

The offcial build is green and follow up will be in another email as mentioned.

Build VS installers for WebAssembly and Mobile workloads (dotnet#55769)

bb36bc8

In order to support generating installers, this change adds the mono.workloads subset and the associated yml.

ghost added the area-Infrastructure-mono label Jul 22, 2021

steveisok added the Servicing-consider Issue for next servicing release review label Jul 22, 2021

steveisok requested review from Anipik, joeloff, lewing and marek-safar July 22, 2021 02:33

steveisok added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jul 22, 2021

ViktorHofer reviewed Jul 22, 2021

View reviewed changes

Anipik merged commit d17b813 into dotnet:release/6.0-preview7 Jul 22, 2021

ghost locked as resolved and limited conversation to collaborators Aug 21, 2021

		<MicrosoftDotNetBuildTasksInstallersPackageVersion>6.0.0-beta.21369.3</MicrosoftDotNetBuildTasksInstallersPackageVersion>
		<MicrosoftDotNetBuildTasksInstallersVersion>6.0.0-beta.21364.3</MicrosoftDotNetBuildTasksInstallersVersion>

		@@ -0,0 +1,42 @@
		<?xml version="1.0" encoding="utf-8"?>
		<Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

		@@ -0,0 +1,2 @@
		# Building
		The workloads project can only be built using .NET Framework msbuild. To build locally, run ```build -project src\workloads\workloads.csproj -msbuildEngine vs```

[release/6.0-preview7] Build VS installers for WebAssembly and Mobile workloads (#55769) #56127

[release/6.0-preview7] Build VS installers for WebAssembly and Mobile workloads (#55769) #56127

Uh oh!

Conversation

steveisok commented Jul 22, 2021

Customer Impact

Testing

Risk

Uh oh!

ghost commented Jul 22, 2021

Customer Impact

Testing

Risk

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!

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!

steveisok commented Jul 22, 2021

Uh oh!

steveisok commented Jul 22, 2021

Uh oh!

steveisok commented Jul 22, 2021

Uh oh!

Anipik commented Jul 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants