[build] Create Microsoft.iOS.Windows.Sdk workload pack #11251
Conversation
| $(foreach platform,$(DOTNET_PLATFORMS),$(eval $(call WorkloadTargets,$(platform),$(shell echo $(platform) | tr A-Z a-z),$($(platform)_NUGET_VERSION_NO_METADATA), \ | ||
| $(if $(findstring $(platform),$(DOTNET_WINDOWS_PLATFORMS)),targets/WorkloadManifest.windows.template.json,targets/WorkloadManifest.template.json)))) |
There was a problem hiding this comment.
I think this will stop generating workloads for platforms included in DOTNET_WINDOWS_PLATFORMS, are you sure you don't want to add a line instead:
| $(foreach platform,$(DOTNET_PLATFORMS),$(eval $(call WorkloadTargets,$(platform),$(shell echo $(platform) | tr A-Z a-z),$($(platform)_NUGET_VERSION_NO_METADATA), \ | |
| $(if $(findstring $(platform),$(DOTNET_WINDOWS_PLATFORMS)),targets/WorkloadManifest.windows.template.json,targets/WorkloadManifest.template.json)))) | |
| $(foreach platform,$(DOTNET_PLATFORMS),$(eval $(call WorkloadTargets,$(platform),$(shell echo $(platform) | tr A-Z a-z),$($(platform)_NUGET_VERSION_NO_METADATA,targets/WorkloadManifest.template.json)))) | |
| $(foreach platform,$(DOTNET_WINDOWS_PLATFORMS),$(eval $(call WorkloadTargets,$(platform),$(shell echo $(platform) | tr A-Z a-z),$($(platform)_NUGET_VERSION_NO_METADATA,targets/WorkloadManifest.windows.template.json)))) |
There was a problem hiding this comment.
I want to use the new "windows" template file for all workloads in $(DOTNET_PLATFORMS) that include a windows SDK. These changes should result in the Microsoft.NET.Workload.iOS workload declaring this new Windows only pack, however since it's conditionally imported in WorkloadManifest.targets it will never need to be installed on macOS. This all seemed to work locally but it looks like CI didn't run for this yesterday so I'm still waiting to check an official build. I don't want to introduce new Windows specific workloads (just one new workload pack). We should be able to continue to ship the same Microsoft.NET.Workload.iOS workload on both macOS and Windows.
pjcollins
force-pushed
the
windows-workload-packs
branch
from
7fa2a18 to
d6543fb
Compare
❌ Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffTest results1 tests failed, 98 tests passed.Failed tests
|
✅ Tests passed on Build. ✅Tests passed on Build. API diff✅ API Diff from stable View API diff🎉 All 102 tests passed 🎉 |
| @@ -0,0 +1,32 @@ | |||
| { | |||
| "version": 5, | |||
| "workloads": { | |||
There was a problem hiding this comment.
I'm confused by this file, do we need two WorkloadManifest.json files?
We also have:
If we need Microsoft.@PLATFORM@.Windows.Sdk to only get included on Windows, I think we can do that in a single file with platforms. They have some examples in this one:
I think we could have:
"Microsoft.@PLATFORM@.Windows.Sdk": {
"kind": "sdk",
"version": "@VERSION@",
"platforms": [ "win-x86", "win-x64" ]
},There was a problem hiding this comment.
The reasoning behind the two files is that WorkloadManifest.template.json is used for all other platforms (mac, watch, tv), and these workloads don't (and some will never?) have a *.Windows.Sdk entry.
Rather than adding more complex string manipulation to dotnet/Makefile to conditionally add this windows entry to only the iOS manifest, I added a new template for workloads that should include a "Windows" Sdk. I suppose we could always have a "Microsoft.@PLATFORM@.Windows.Sdk" entry in those other workloads that ultimately mapped to nothing, but it seems uglier.
We may want add the "platforms": [ "win-x86", "win-x64" ] declaration here, though the conditional import in the iOS WorkloadManifest.targets file allows the new manifest which declares a windows specific pack to work on macOS even when the pack is not installed.
There was a problem hiding this comment.
Ok makes sense 👍
I think we need to put platforms so that the future dotnet workload install commands won't install these Windows packs on Mac.
There was a problem hiding this comment.
It looks like this feature may not work when declared directly on a pack entry, I'm going to revert this portion for now dotnet/sdk#17151
❌ Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffTest results26 tests failed, 78 tests passed.Failed tests
|
✅ Tests passed on Build. ✅Tests passed on Build. API diff✅ API Diff from stable View API diff🎉 All 104 tests passed 🎉 |
|
Waiting on review from @emaf to merge. |
|
/sudo backport d16-10 |
|
Backport Job to branch d16-10 Created! The magic is happening here |
|
Oh no! Backport failed! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4804552 for more details. |
Converts the Microsoft.iOS.Windows.Sdk NuGet package into a proper
workload SDK pack. The entry point for this pack has been changed,
and it is now imported through the
WorkloadManifest.targetsfileincluded in
Microsoft.NET.Workload.iOS, rather than being importeddirectly from
Microsoft.iOS.Sdk.Import ordering has otherwise changed slightly. The following files are
now imported before the majority of the
Microsoft.iOS.Sdk(and themajority of the .NET SDK targets):
After this the majority of the .NET SDK targets will load, followed by
the
Microsoft.iOS.Sdktargets. Finally, everything declared in the<AfterMicrosoftNETSdkTargets/>hook loads, which consists of: