Skip to content

[Workloads] Allow missing workload packs#9495

Closed
pjcollins wants to merge 1 commit intodotnet:release/6.0from
pjcollins:published-packs
Closed

[Workloads] Allow missing workload packs#9495
pjcollins wants to merge 1 commit intodotnet:release/6.0from
pjcollins:published-packs

Conversation

@pjcollins
Copy link
Member

@pjcollins pjcollins commented May 27, 2022

Commit f8c0d51 introduced a change that breaks MSI generation for the
Android workload. Previously, workload packs that were declared in the
WorkloadManifest.json but not present on disk would be skipped.

The .NET 7 Android workload manifest now declares SDK packs that
have already shipped in order to support .NET 6 projects. We do not
want to regenerate MSI installers for these packs, and we should be able
to continue shipping them in VS by keeping the .NET 6 workload authoring
in VS alongside new .NET 7 workload authoring.

A new AllowMissingPacks task parameter has been added to let workload
owners skip MSI and setup authoring generation for packs that are not
present when the tooling runs.

@pjcollins pjcollins marked this pull request as draft May 27, 2022 17:15
@pjcollins
Copy link
Member Author

After further discussion with @jonathanpeppers I've realized that we would still have an issue with VS insertions. There is no automatic way to install these SDK packs during a project build, and as a result I think we do need to figure out how to get them into VS in all cases.

These changes may still be useful, but they won't fix everything for the Android scenario at least.

@pjcollins pjcollins marked this pull request as ready for review June 2, 2022 15:36
@pjcollins
Copy link
Member Author

pjcollins commented Jun 2, 2022

I think we solve the VS insertion issue by keeping the existing .NET 6 workload authoring and components in VS when we go to insert .NET 7 workloads, instead of replacing them. I think this will still be a necessary change for us in that case.

jonathanpeppers
jonathanpeppers previously approved these changes Jun 2, 2022
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

@joeloff
Copy link
Member

joeloff commented Jun 2, 2022

I'm good with this in principle since it does restore parity with what we previously had. I am curious though how these packs are being resolved. Are they produced separately from another build?

@pjcollins
Copy link
Member Author

pjcollins commented Jun 2, 2022

Are they produced separately from another build?

These packs were produced by our .NET 6 branch and were already published to VS and NuGet.org. As far as Visual Studio is concerned the packs will already be inserted via https://devdiv.visualstudio.com/DevDiv/_git/VS?path=/.corext/Configs/dotnet-workloads-components.json&version=GBmain&_a=contents.

Once we go to insert .NET 7 packs, I think we'll add a new Microsoft.NET.Sdk.Android.Workload.7.0.vsman file to this list so that we can have .NET 6 and .NET 7 workloads in VS at the same time.

@joeloff
Copy link
Member

joeloff commented Jun 6, 2022

@mmitche as an FYI when we're ready to merge

Commit f8c0d51 introduced a change that breaks MSI generation for the
Android workload.  Previously, workload packs that were declared in the
WorkloadManifest.json but not present on disk would be skipped.

The .NET 7 Android workload manifest [now declares SDK packs][0] that
have already shipped in order to support .NET 6 projects.  We do not
want to regenerate MSI installers for these packs, and we should be able
to continue shipping them in VS by keeping the .NET 6 workload authoring
in VS alongside new .NET 7 workload authoring.

A new `AllowMissingPacks` task parameter has been added to let workload
owners skip MSI and setup authoring generation for packs that are not
present when the tooling runs.

[0]: https://github.com/xamarin/xamarin-android/blob/ac5bb6b910a5d9e72f38b9df4528dd308e5000d6/src/Xamarin.Android.Build.Tasks/Microsoft.NET.Sdk.Android/WorkloadManifest.in.json#L45
@pjcollins
Copy link
Member Author

@dsplaisted @joeloff I just fixed the conflict with the recent workload pack grouping changes. This is still working for me locally against a recent Android build (even with the new pack group parameters set), so I think it should be ready to merge?

@pjcollins
Copy link
Member Author

I'll close this, these changes are now in #9628.

@pjcollins pjcollins closed this Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants