Refactoring workload build tasks by joeloff · Pull Request #8645 · dotnet/arcade

joeloff · 2022-03-18T06:45:55Z

Refactoring workload build tasks

Cleaning up duplicate templates for generating .csproj files to package workload MSIs
Adding support for building multiple manifests and feature bands
Support for publishing SWIX manifests for feature bands and avoid including unreferenced packages.
Better support for building MSIs in parallel to reduce build times.

joeloff · 2022-03-18T06:48:47Z

@rainersigwald tagged you because of the MSBUILD shenanigans in case you have time to take a peek.

I still have to make fixes for the tests, clean up some doc comments and fix some files for source build.

dsplaisted · 2022-03-18T13:46:29Z

src/Microsoft.DotNet.Build.Tasks.Workloads/src/WorkloadManifestPackage.cs

+            MsiVersion = !string.IsNullOrWhiteSpace(package.GetMetadata(Metadata.MsiVersion)) ? new Version(package.GetMetadata(Metadata.MsiVersion))
+                : msiVersion != null ? msiVersion : throw new Exception(string.Format(Strings.NoManifestInstallerVersion, nameof(CreateVisualStudioWorkload),
+                nameof(CreateVisualStudioWorkload.ManifestMsiVersion), nameof(CreateVisualStudioWorkload.WorkloadManifestPackageFiles), Metadata.MsiVersion));


I find multiple ternary operators strung together hard to parse. If/else statements or maybe something with matching would be more readable for me.

dsplaisted · 2022-03-18T13:51:52Z

src/Microsoft.DotNet.Build.Tasks.Workloads/src/WorkloadManifestPackage.cs

+                : msiVersion != null ? msiVersion : throw new Exception(string.Format(Strings.NoManifestInstallerVersion, nameof(CreateVisualStudioWorkload),
+                nameof(CreateVisualStudioWorkload.ManifestMsiVersion), nameof(CreateVisualStudioWorkload.WorkloadManifestPackageFiles), Metadata.MsiVersion));
+
+            SdkFeatureBand = GetSdkFeatureBandVersion(GetSdkVersion(Id));


I may be confused here, but isn't the version that's part of the package ID already the SDK feature band version? For example, for Microsoft.NET.Sdk.Maui.Manifest-6.0.200, the version number returned from GetSdkVersion would already be 6.0.200, and so there's no need for a separate call to GetSdkFeatureBandVersion.

dsplaisted · 2022-03-18T14:26:44Z

src/Microsoft.DotNet.Build.Tasks.Workloads/src/CreateVisualStudioWorkload.cs

+                        {
+                            foreach (WorkloadPackId packId in wd.Packs)
+                            {
+                                WorkloadPack pack = manifest.Packs[packId];


It looks like this would break if a workload pack was referenced from one manifest but defined in another. This is supposed to be valid, although I don't think we ever do so in the current manifests. Instead, we have manifests that extend other ones.

We previously maintained a list of packs that were absent (technically external). I'll return that logic

I went through the old code again. The problem with external packs is that their IDs can be shortened to meet the limits imposed by the VS package cache. The replacement values used to shorten the value reside with the repo building the packs, so we have no way to know which packages will be shortened or how they will be shortened.

dsplaisted · 2022-03-18T15:11:52Z

src/Microsoft.DotNet.Build.Tasks.Workloads/src/WorkloadPackPackage.cs

+                        default:
+                            // Unsupported RID.
+                            continue;


What happens if there's a rid such as "win7-x64" which isn't hard-coded here? How would we catch that?

Ideally maybe we should be walking the RID graph here. However, a much simpler solution would be to simply list all the RIDs we expect, including non-Windows ones, and throw an error if an unexpected one is encountered.

dsplaisted · 2022-03-18T17:56:12Z

src/Microsoft.DotNet.Build.Tasks.Workloads/src/BuildData.cs

+        /// <summary>
+        /// The set of feature bands that include contain a reference to this pack.
+        /// </summary>
+        public Dictionary<string, HashSet<ReleaseVersion>> FeatureBands = new();


Maybe this should be called PlatformFeatureBands to make clearer that the key is a platform?

Definitely, thank you. I couldn't come up with a decent name.

dsplaisted · 2022-03-18T18:10:41Z

src/Microsoft.DotNet.Build.Tasks.Workloads/src/CreateVisualStudioWorkload.wix.cs

+                                {
+                                    if (!File.Exists(sourcePackage))
+                                    {
+                                        throw new FileNotFoundException(message: null, fileName: sourcePackage);


This doesn't seem to account for how we'll need to build manifests in the 7.0 branch that reference packs produced out of 6.0. We won't want to re-package the 6.0 workload packs into MSIs. I think probably we should be able to build and package the manifest without even needing access to the packs it refers to.

dsplaisted · 2022-03-18T18:12:48Z

src/Microsoft.DotNet.Build.Tasks.Workloads/src/CreateVisualStudioWorkload.wix.cs

+                // Generate SWIX projects for the Visual Studio components. These are driven by the manifests, so
+                // they need to be ordered based on feature bands to avoid pulling in unnecessary packs into the drop
+                // artifacts.                


I'm not following this comment. I don't see where the swixs are being ordered, or how that relates to what packages are included.

I'll clarify the comment. At the end of the build you'll get a set of SWIX projects that generate the individual packages wrapping the MSIs, components. The item contains two things: the path to the swixproj and the feature band. The idea being that you can the build these projects, e.g. dotnet build %(SwixProject.Identity) /p:ManifestPath=bin\workloads\%(SdkFeatureBand) ManifestPath in this case refers to the setup manifests, not workloads, but it will help to create artifacts that are grouped by the feature bands since later, all the individual manifests are merged into a .vsman file and that whole folder gets pushed into blob storage. Since one pack can ship in different VS versions, this helps to avoid creating drops containing items that should not be included for a specific VS version. There is a trimming step that removes orphaned packages, but I don't want to rely on something that is outside our control and just make sure the drops have the correct set of files.

src/Microsoft.DotNet.Build.Tasks.Workloads/src/WorkloadPackageBase.cs

pjcollins · 2022-03-31T16:35:34Z

@joeloff @mmitche I think we'll want to apply this to the main branch as well? I'm not sure if there is a specific process for that but want to make sure it's not forgotten.

joeloff · 2022-03-31T16:37:34Z

Yes, Them moment I port this to main though, it's going to set of automatic code flows into emsdk/runtime. I'll send mail about this to the owners so they can be ready to address the breaking changes.

pjcollins · 2022-03-31T16:44:32Z

There is no rush from our side, we can consume the 6.0.0 packages for the time being.

* Refactoring workload build tasks * Fix source build and some random cleanup * Updating tests, code cleanup * Minor fixes, unit test conversion * Mark tests as Windows only, fix missing content for Helix * Hide WiX and test packages from Solution Explorer * Fix duplicate publish items * Fix link target for helix * Fix link metadata for WiX * Pass ICE suppressions to Light, more cleanup * Fix file extraction for packs, add unit test for template pack MSI

* Refactoring workload build tasks (#8645) * Refactoring workload build tasks * Fix source build and some random cleanup * Updating tests, code cleanup * Minor fixes, unit test conversion * Mark tests as Windows only, fix missing content for Helix * Hide WiX and test packages from Solution Explorer * Fix duplicate publish items * Fix link target for helix * Fix link metadata for WiX * Pass ICE suppressions to Light, more cleanup * Fix file extraction for packs, add unit test for template pack MSI * Pass ICE suppressions to Light (#9061) * Create workload pack group installers (#9514) * Remove duplicate PackageReference * Create MSIs for workoad pack groups * Build NuGet wrapper packages for workload pack group MSIs * Generate WorkloadPackGroups.json in manifest MSIs * Add swix authoring for workload pack groups * De-duplicate workload pack group creation * Put braces around ProductCode and UpgradeCode registry values * Write registry keys for pack groups * Fix swix dependencies for pack groups * Use correct GUID format when setting candle variables * Add test for creating pack group dependency in SWR file * Support building with missing workload packs (#9628) * Support building with missing workload packs * Include extracted manifest files in manifest MSI payload nupkg * Fix versioning errors in workloads (#10363) * Fix versioning errors in workloads * Disable TRX tests while reporting to AZDO is broken (#10358) (#10380) Co-authored-by: Matt Galbraith <MattGal@users.noreply.github.com> * clean up, api changes Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com> Co-authored-by: Matt Galbraith <MattGal@users.noreply.github.com>

Refactoring workload build tasks

c5ce9ea

joeloff marked this pull request as draft March 18, 2022 06:46

joeloff requested review from baronfel, dsplaisted, gkulin, marcpopMSFT and rainersigwald March 18, 2022 06:46

Fix source build and some random cleanup

4467261

dsplaisted previously approved these changes Mar 18, 2022

View reviewed changes

Updating tests, code cleanup

6b10bdb

joeloff dismissed dsplaisted’s stale review via 6b10bdb March 22, 2022 22:28

joeloff added 6 commits March 22, 2022 21:44

Minor fixes, unit test conversion

99b9788

Mark tests as Windows only, fix missing content for Helix

1ba0600

Hide WiX and test packages from Solution Explorer

2043b45

Fix duplicate publish items

cc94777

Fix link target for helix

cff992f

Fix link metadata for WiX

b249853

joeloff marked this pull request as ready for review March 24, 2022 00:02

Pass ICE suppressions to Light, more cleanup

98e7148

pjcollins reviewed Mar 24, 2022

View reviewed changes

src/Microsoft.DotNet.Build.Tasks.Workloads/src/WorkloadPackageBase.cs Show resolved Hide resolved

Fix file extraction for packs, add unit test for template pack MSI

030c80c

mmitche approved these changes Mar 28, 2022

View reviewed changes

mmitche merged commit f8c0d51 into dotnet:release/6.0 Mar 28, 2022

wtgodbe mentioned this pull request Jun 22, 2022

Undo revert of Arcade update to ingest Workload Manifest task dotnet/runtime#71150

Closed

Conversation

joeloff commented Mar 18, 2022

Uh oh!

joeloff commented Mar 18, 2022

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!

Uh oh!

pjcollins commented Mar 31, 2022

Uh oh!

joeloff commented Mar 31, 2022

Uh oh!

pjcollins commented Mar 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants