sdk icon indicating copy to clipboard operation
sdk copied to clipboard

Add Install Records for VS Managed Workloads During 'Update'

Open nagilson opened this issue 1 year ago • 2 comments

Why are we doing this

This is an attempt to fix the following: https://github.com/dotnet/sdk/issues/21811 and https://github.com/dotnet/aspire/issues/1559

If The .NET SDK CLI updates a workload, it will then update the manifests. VS will also begin using the updated manifests as it shares the workload resolver with the CLI. But Visual Studio will not have updated its workloads, and the workload resolver does not have logic to use the old manifests for VS.

Then, Visual Studio will reference to nonexistent packs because its workloads were not updated even though the manifest points to newer packs. So, by writing the install record, we hope to make the rest of the update command add back in the packs, so VS would work fine. And if we uninstall the standalone SDK, then the old packs still have a VS reference, and the newer manifest should be removed, so it should be fine.

What does this PR do

Makes the CLI update command create SDK install records for VS managed workloads that don't already have SDK managed records. This is to cause the downstream logic to install the packs for VS to use.

Tests

Lots of manual testing. Which requires many hours of setup, don't recommend. This is hard to write tests for -- maybe once VM changes are merged we can add tests. For evidence of behavior, see screenshots below.

Before

image

Running workload update from this state does not create an install record on the SDK side.

Also, after running update:

dotnet publish now becomes broken for VS workload apps, such as wasm apps, or for aspire apps.

image

Also, installing any other workload via CLI (in this example, MAUI), the VS workload(s) (wasm-tools in this case) are no longer found and you are in a broken state. Running workload install again does not fix the problem.

Without my fix and Phil's fix in Visual Studio, templates are also missing from VS Workloads after installing a different workload in the CLI. image

After 😮

When running dotnet workload update, install records are created for VS workloads in the CLI side. image

After installing MAUI or another workload, wasm-tools is still found. image

dotnet publish succeeds for apps with workloads that were previously installed by VS and got updated by the CLI. image

Templates appear in VS even after installing or updating workloads via the CLI. Screenshot_19

When running install, a message appears indicating workloads from VS had install records added to them. Example: image

nagilson avatar Jan 29 '24 21:01 nagilson

/azp run

nagilson avatar Feb 12 '24 18:02 nagilson

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Feb 12 '24 18:02 azure-pipelines[bot]

@joeloff Thank you for your review! Are those things we can test before getting an official build just via patching a standalone sdk? It seems like all of those I should be able to test before merging, except for the 8.0.3xx one but I might be wrong.

nagilson avatar Mar 13 '24 16:03 nagilson

No, at the moment you need to get an official build once your SDK changes flow to installer. This is another good example where merging the repos has real benefits

joeloff avatar Mar 13 '24 16:03 joeloff

Ok, thanks :) In that case, I suppose we will merge it!

nagilson avatar Mar 13 '24 16:03 nagilson

No, at the moment you need to get an official build once your SDK changes flow to installer. This is another good example where merging the repos has real benefits

I think you could test this without waiting for flow into installer by copying the stage 2 implementation over the installed SDK folder. I showed Noah how to do this manually, and the VM installation tests automate this.

dsplaisted avatar Mar 13 '24 23:03 dsplaisted

No, at the moment you need to get an official build once your SDK changes flow to installer. This is another good example where merging the repos has real benefits

I think you could test this without waiting for flow into installer by copying the stage 2 implementation over the installed SDK folder. I showed Noah how to do this manually, and the VM installation tests automate this.

It won't work for this specific case. You want to verify that running the bundle cleanly removes everything. Installing an MSI and then modifying the files won't let the MSI remove the files. They'll get flagged as user-modified and left behind.

You could maybe push your branch to internal, grab the toolset zip, then modify the installer repo to use that zip and build a more official bundle.

joeloff avatar Mar 14 '24 00:03 joeloff

It looks like it got in 2 hours after this PR, https://github.com/dotnet/installer/pull/19037 so the nightly build should have it. Let me try it out.

nagilson avatar Mar 14 '24 16:03 nagilson

Doing the testing now. One thing I noticed is these messages side by side might confuse people but I think it's ok, since the 2nd message is prefixed with 'VS' workloads. image

nagilson avatar Mar 14 '24 17:03 nagilson

  1. Install VS + wasm tools
  2. Install the SDK and run dotnet workload update to add the CLI references
  3. Uninstall VS
  4. Uninstall the SDK
  5. Verify that the SDK removes the WASM workload installations

This works, the entire dotnet folder is gone. Also if I reinstall stuff afterwards it seems to work. image

nagilson avatar Mar 14 '24 17:03 nagilson

Variation on this is to alter the later steps 3. Remove the SDK (this should remove the CLI dependents) 4. Remove VS 5. Verify that VS was able to remove WASM tools (because step 3 should have removed the dependents that would otherwise have blocked the removal)

This also works. Only dotnet.exe remains. image

It also worked in between the state of after uninstalling the standalone SDK. The VS workloads were still in a good state. image

nagilson avatar Mar 14 '24 17:03 nagilson

Another variation is still install multiple SDKs after VS, different feature bands, like 8.0.3xx and eventually 8.0.4xx

I tried using 8.0.202, mixing operations between 8.0.100 and 8.0.300, and VS, and pinning to it and doing various things and it seemed to work.

image

nagilson avatar Mar 14 '24 18:03 nagilson

Doing the testing now. One thing I noticed is these messages side by side might confuse people but I think it's ok, since the 2nd message is prefixed with 'VS' workloads.

I think we should update this so it doesn't print the "no workloads installed" message if there are any VS workloads installed.

dsplaisted avatar Mar 15 '24 00:03 dsplaisted