Add workload fallback to previous feature band #19545
Conversation
|
@dsplaisted the issue mentions that we want to add the ability to depreciate workloads when we fallback, do you have any thoughts on that implementation? I haven't included it here. |
|
The fallback should be per manifest, instead of for the top-level manifest folder. IE there might be a 6.0.200 version of Microsoft.NET.Workload.Mono.ToolChain and Microsoft.NET.Workload.Emscripten, but not 6.0.200 versions of Microsoft.NET.Sdk.Android or Microsoft.NET.Sdk.iOS. In that case it should fall back to the 6.0.100 folder for iOS and Android, but not for the Mono toolchain or Emscripten workload manifests. This also should be based on a list of known manifests that are included in the SDK. That's the way to deprecate workloads too, if we don't include them in the SDK, then it wouldn't fall back to previous folders for that workload manifest. We probably want to generate the list of known workload manifests in the installer repo, and put that list in a data file in the SDK directory for the workload resolver logic to pick up. |
sfoslund
force-pushed
the
WorkloadPrevFallback
branch
from
88c1d02
to
ef81e90
Compare
|
@dsplaisted okay I've updated the implementation to fallback per manifest and when a serialized list of known manifests is located at |
src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs
Outdated
Show resolved
Hide resolved
src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs
Show resolved
Hide resolved
src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs
Outdated
Show resolved
Hide resolved
src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/Strings.resx
Outdated
Show resolved
Hide resolved
.../Microsoft.NET.Sdk.WorkloadManifestReader.Tests/SdkDirectoryWorkloadManifestProviderTests.cs
Show resolved
Hide resolved
src/Tests/dotnet-workload-install.Tests/GivenDotnetWorkloadInstall.cs
Outdated
Show resolved
Hide resolved
|
FYI @wli3 since I think you had some concerns about this feature, do you want to review too? |
|
checking... |
| @@ -80,6 +97,7 @@ public IEnumerable<(string manifestId, string? informationalPath, Func<Stream> o | |||
|
|
|||
| public IEnumerable<string> GetManifestDirectories() | |||
There was a problem hiding this comment.
implementation side, we just need to make sure GetManifestDirectories and GetManifests are the only methods used to search for manifest. Or other callers might get the old behavior
There was a problem hiding this comment.
Agreed, I think that's the intention regardless of this change.
To resolve a workload, the current version band is used to look in `sdk-manifests` directory. But when in development, and bumping to a newer version band, not all the manifests might have packages for that. This shows up when bumping the band to `6.0.200`, where we generate the corresponding packages for wasm-tools. But the Emscripten packages are still on `6.0.100`. SDK [added](dotnet/sdk#19545) support for falling back to older bands in that case. The `InstallWorkloadFromArtifacts` task emits a warning about not being able to find the emscripten manifest package. But this becomes an error on CI due to warnaserror=true. So, log a message instead of a warning. But this is only done for the *dependent* manifest.
* Bump SdkVersionForWorkloadTesting to use a 200 band SDK #62787 bumped the manifest id. This change bumps the prop for testing. * Download dotnet-install script for installing workloads Instead of trying to use the script from `.dotnet`, download the script. `.dotnet` might not exist, for example, when the `global.json` version matches the system installed one. * Workloads: fix CI build when bumping to a newer version band To resolve a workload, the current version band is used to look in `sdk-manifests` directory. But when in development, and bumping to a newer version band, not all the manifests might have packages for that. This shows up when bumping the band to `6.0.200`, where we generate the corresponding packages for wasm-tools. But the Emscripten packages are still on `6.0.100`. SDK [added](dotnet/sdk#19545) support for falling back to older bands in that case. The `InstallWorkloadFromArtifacts` task emits a warning about not being able to find the emscripten manifest package. But this becomes an error on CI due to warnaserror=true. So, log a message instead of a warning. But this is only done for the *dependent* manifest. Co-authored-by: Steve Pfister <[email protected]> Co-authored-by: Ankit Jain <[email protected]>
Fixes #18489