Skip to content

Additional fixes for workloads/multitargeting#10488

Merged
mmitche merged 4 commits intodotnet:release/6.0from
joeloff:workload-dependencies
Aug 18, 2022
Merged

Additional fixes for workloads/multitargeting#10488
mmitche merged 4 commits intodotnet:release/6.0from
joeloff:workload-dependencies

Conversation

@joeloff
Copy link
Member

@joeloff joeloff commented Aug 18, 2022

Number of fixes

  1. Fix the ManifestMsiVersion parameter requirement on CreateVisualStudioWorkload
  2. Add metadata to SWIX output items that indicate the type of VS package it produces. This will make it easier to create batches, e.g. separating packages and components and publishing them separately to different VSDrop locations.
  3. Fix the package dependencies being generated for workloads in VS and add a unit test to run the task end-to-end. This requires using an actual set of packages. For now, we're using EMSDK because it's the smallest workload and relies on using aliasing.

@joeloff
Copy link
Member Author

joeloff commented Aug 18, 2022

@pjcollins ping me if you want a private copy of the task DLL to validate so you're not blocked waiting on the official Arcade build.

@pjcollins
Copy link
Member

pjcollins commented Aug 18, 2022

@joeloff thanks! I built this locally and it fixes my two issues with inconsistent pack alias usage and the ManifestMsiVersion param.

pjcollins
pjcollins previously approved these changes Aug 18, 2022
View reviewed changes
dsplaisted
dsplaisted previously approved these changes Aug 18, 2022
View reviewed changes
...osoft.DotNet.Build.Tasks.Workloads.Tests/Microsoft.DotNet.Build.Tasks.Workloads.Tests.csproj
Comment on lines +26 to +29
<PackageReference Include="Microsoft.NET.Workload.Emscripten.Manifest-6.0.200" Version="6.0.4" GeneratePathProperty="true" />
<PackageReference Include="Microsoft.NET.Runtime.Emscripten.2.0.23.Node.win-x64" Version="6.0.4" GeneratePathProperty="true" />
<PackageReference Include="Microsoft.NET.Runtime.Emscripten.2.0.23.Python.win-x64" Version="6.0.4" GeneratePathProperty="true" />
<PackageReference Include="Microsoft.NET.Runtime.Emscripten.2.0.23.Sdk.win-x64" Version="6.0.4" GeneratePathProperty="true" />
Copy link
Member

@dsplaisted dsplaisted Aug 18, 2022

Choose a reason for hiding this comment

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

Seems like PackageDownload would be more appropriate here, since we're just trying to download assets. Does PackageDownload support GeneratePathProperty?

src/Microsoft.DotNet.Build.Tasks.Workloads.Tests/testassets/AliasedPacks.json
@@ -0,0 +1,27 @@
{
"version": "33.0.0-rc.1.132",
Copy link
Member

@dsplaisted dsplaisted Aug 18, 2022

Choose a reason for hiding this comment

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

I don't see this file being used at all, is it supposed to be here?

Copy link
Member

@mmitche mmitche Aug 18, 2022

Choose a reason for hiding this comment

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

It looks to be used in the workloads test csproj

src/Microsoft.DotNet.Build.Tasks.Workloads/src/WorkloadPackPackage.wix.cs Outdated
Platforms = platforms;
MsiVersion = Version;

// Override the SWIX ID for MSI packages to use the shortened, non-aliased ID with the pack version
Copy link
Member

@dsplaisted dsplaisted Aug 18, 2022

Choose a reason for hiding this comment

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

Is there a wording that makes this more clear? I'm never sure which ID is the "aliased" one.

Copy link
Member Author

@joeloff joeloff Aug 18, 2022

Choose a reason for hiding this comment

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

Yup, I'll clarify

@joeloff joeloff dismissed stale reviews from dsplaisted and pjcollins via cad93cd August 18, 2022 17:20
@marcpopMSFT marcpopMSFT mentioned this pull request Aug 18, 2022
Closed
pjcollins
pjcollins approved these changes Aug 18, 2022
View reviewed changes
Copy link
Member

@pjcollins pjcollins left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Latest changes LGTM, manifest component IDs no longer also include full version strings:
"id": "Android.Manifest-7.0.100-rc.1"

@mmitche mmitche enabled auto-merge (squash) August 18, 2022 18:38
@mmitche mmitche merged commit b851fe8 into dotnet:release/6.0 Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsplaisted dsplaisted dsplaisted approved these changes

@pjcollins pjcollins pjcollins approved these changes

@mmitche mmitche mmitche approved these changes

@hoyosjs hoyosjs Awaiting requested review from hoyosjs

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@joeloff @pjcollins @dsplaisted @mmitche