Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[dotnet-core-uninstall] Allow preview version removal #269

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[dotnet-core-uninstall] Allow preview version removal #269

wants to merge 1 commit into from

Conversation

pjcollins
Copy link
Member

@pjcollins pjcollins commented Oct 11, 2023 •
edited
Loading

Context: #265

Updates the uninstall tool to ignore the upper version check and
uninstallation restriction when the --force parameter is provided.

Local testing output:

% sudo dotnet artifacts/bin/dotnet-core-uninstall/Debug/net8.0/dotnet-core-uninstall.dll list                   

This tool cannot uninstall versions of the runtime or SDK that are installed using zip/scripts. The versions that can be uninstalled with this tool are:

.NET Core SDKs:
  9.0.100-preview.2.24157.14  (arm64)  [Cannot uninstall version 9.0.0 and above. Use —-force to remove]             
  8.0.101                     (arm64)  [Used by Visual Studio for Mac. Specify individually or use —-force to remove]
                                                                           

.NET Core Runtimes:
  9.0.0-preview.2.24128.5  (arm64)  [Used by Visual Studio for Mac or SDKs. Specify individually or use —-force to remove]
  9.0.0-preview.2.24128.5  (x64)                                                                                          
  9.0.0-preview.2.24128.4  (arm64)                                                                                        
  9.0.0-preview.2.24128.4  (x64)  
  8.0.1   (arm64)  [Used by Visual Studio for Mac or SDKs. Specify individually or use —-force to remove]
  8.0.1   (x64)                                                                                          

% sudo dotnet artifacts/bin/dotnet-core-uninstall/Debug/net8.0/dotnet-core-uninstall.dll remove --sdk 9.0.100-preview.2.24157.14
Uninstallation not allowed. This tool cannot uninstall .NET Core SDKs with version 9.0.0 or above. Use --force to ignore this restriction.

% sudo dotnet artifacts/bin/dotnet-core-uninstall/Debug/net8.0/dotnet-core-uninstall.dll remove --sdk 9.0.100-preview.2.24157.14 --force
The following items will be removed:
  Microsoft .NET Core SDK 9.0.100-preview.2.24157.14 (x64)

To avoid breaking Visual Studio for Mac or other problems, read https://aka.ms/dotnet-core-uninstall-docs.

Do you want to continue? [y/n] y
Uninstalling: Microsoft .NET Core SDK 9.0.100-preview.2.24157.14 (x64).

@pjcollins pjcollins mentioned this pull request Oct 11, 2023
Open
pjcollins
pjcollins commented Oct 12, 2023
View reviewed changes
src/dotnet-core-uninstall/Shared/Configs/CommandLineConfigs.cs Outdated
@@ -133,6 +133,10 @@ internal static class CommandLineConfigs
RuntimeInfo.RunningOnWindows ? LocalizableStrings.ForceOptionDescriptionWindows
: LocalizableStrings.ForceOptionDescriptionMac);

public static readonly Option IgnoreUpperLimitOption = new Option(
"--ignore-upper-limit",
Copy link
Member Author

Choose a reason for hiding this comment

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

@joeloff @baronfel I was looking at improving preview uninstall support for future versions. My initial thought was to add this --ignore-upper-limit parameter, but after thinking a bit more I think the UX may be better if we overload --force to also handle this case instead?

@joeloff
Copy link
Member

joeloff commented Oct 25, 2023

cc @Forgind

@marcpopMSFT
Copy link
Member

@baronfel per our chat earlier about removing the upper limit. This PR provides an alternative option for customers though is not exacty what we were talking about.

@pjcollins pjcollins force-pushed the skip-upper branch 4 times, most recently from cf82982 to 91342c4 Compare March 20, 2024 18:58
@pjcollins pjcollins marked this pull request as ready for review March 20, 2024 19:10
@pjcollins
Copy link
Member Author

I reworked this to use the --force argument to uninstall versions over the upper limit, rather than adding a new parameter.

@pjcollins pjcollins changed the title [dotnet-core-uninstall] Allow upper version bypass [dotnet-core-uninstall] Allow preview version removal Mar 20, 2024
Forgind
Forgind reviewed Mar 20, 2024
View reviewed changes
src/dotnet-core-uninstall/Shared/VSVersioning/VisualStudioSafeVersionsExtractor.cs Outdated
@@ -103,7 +104,9 @@ public static IEnumerable<Bundle> GetUninstallableBundles(IEnumerable<Bundle> bu
{
var (bundlesByDivisions, remainingBundles) = ApplyVersionDivisions(allBundles);

var bundlesAboveUpperLimit = remainingBundles.Where(bundle => bundle.Version.SemVer >= UpperLimit);
var bundlesAboveUpperLimit = CommandLineConfigs.CommandLineParseResult.ForceArgumentProvided()
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this does what you might expect? I think this just changes a string, which shouldn't really matter for uninstallation purposes. If the user specifies force (or however we decide they should be able to request uninstalling versions above the upper limit), I think we should instead add an extra version division [UpperLimit, infinity) and treat it the same as the rest.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, this was left over from some of my initial changes and I forgot to back it out.

With respect to the extra version division suggestion, are you suggesting that we remove the "force argument provided" check around CommandBundleFilter.FilterRequiredBundles and instead check if this param is provided and update the version groups created in VisualStudioSafeVersionsExtractor.ApplyWindowsVersionDivisions / VisualStudioSafeVersionsExtractor.ApplyMacVersionDivisions accordingly?

Copy link
Member

Choose a reason for hiding this comment

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

Yes; that's what I was thinking. I haven't spent that much time with cli-lab, though, so it would probably be good to get signoff from someone else who's commented on this PR.

@pjcollins
[dotnet-core-uninstall] Allow upper version bypass
887873a 
Updates the uninstall tool to ignore the upper version check and
uninstallation restriction when the `--force` parameter is provided.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Forgind Forgind Forgind left review comments

At least 1 approving review is required to merge this pull request.

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
@pjcollins @joeloff @marcpopMSFT @Forgind