Permit workload restore to work with new global.json #42606
Conversation
Specifying a workload set version in the global.json file is a supported scenario. If that workload set has not yet been installed, and the user tries to take an action that relies on workloads, we should error and indicate that they should make sure that workload set is installed first via update, install, or restore. We do that correctly, but when the user subsequently restores, we do not escape from the error case and instead direct the user to run restore. We should handle it properly, essentially installing the workload set before we discover which workloads the user needs and installing those.
dotnet-issue-labeler
bot
added
Area-Workloads
untriaged
There was a problem hiding this comment.
We definitely need to fix this (so this should be retargeted to release/9.0.1xx now), but I wonder if there's a way to do it without so much code change. We are going to update the workloads anyway after we discover which ones are needed, so would it make sense to just run an update command before doing the discovery? That seems like it would be a lot simpler.
Also, for testing, consider writing a VM/MSI installation test for this. I think it would work well :-)
src/Cli/dotnet/commands/dotnet-workload/restore/WorkloadRestoreCommand.cs
Outdated
Show resolved
Hide resolved
That's what it does? And note that most of the code changes are just taking a bunch of code out of one place in update/install and putting it in a helper method instead so I can call it in two places depending on whether I want to capture the result of the command in a workload history record.
Yeah; that should be doable. |
Specifying a workload set version in the global.json file is a supported scenario. If that workload set has not yet been installed, and the user tries to take an action that relies on workloads, we should error and indicate that they should make sure that workload set is installed first via update, install, or restore. We do that correctly, but when the user subsequently restores, we do not escape from the error case and instead direct the user to run restore. We should handle it properly, essentially installing the workload set before we discover which workloads the user needs and installing those.
Forgind
force-pushed
the
fix-restore-error
branch
from
e963dec
to
8e820df
Compare
This looks similar to the failure on #42978, though it's a different test that's timing out. |
| { | ||
| InstallSdk(); |
There was a problem hiding this comment.
I think you need to remove the call to InstallSdk from SetupWorkloadSetInGlobalJson, and add it back in UseGlobalJsonToSpecifyWorkloadSet.
|
/backport to release/8.0.4xx |
|
Started backporting to release/8.0.4xx: https://github.com/dotnet/sdk/actions/runs/10726264895 |
|
@marcpopMSFT backporting to release/8.0.4xx failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Permit workload restore to work with new global.json
Applying: move up workload history recorder
error: sha1 information is lacking or useless (src/Cli/dotnet/commands/dotnet-workload/restore/WorkloadRestoreCommand.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 move up workload history recorder
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@marcpopMSFT an error occurred while backporting to release/8.0.4xx, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
|
@dsplaisted looks like I couldn't just backport this as too much has changed. Let's sync tomorrow in standup and decide if we want to take this or not for servicing. CC @baronfel |
|
Per offline chat, the owrkaround is to run a workload update first (confirmed). We'll document this for now and revisit if we get feedback: dotnet/core#9491 |
Fixes #42582
Specifying a workload set version in the global.json file is a supported scenario. If that workload set has not yet been installed, and the user tries to take an action that relies on workloads, we should error and indicate that they should make sure that workload set is installed first via update, install, or restore. We do that correctly, but when the user subsequently restores, we do not escape from the error case and instead direct the user to run restore. We should handle it properly, essentially installing the workload set before we discover which workloads the user needs and installing those.
I haven't tested this properly yet.