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

(#508) Fix some files not being removed during uninstall/upgrade #3151

Merged
merged 5 commits into from
May 10, 2023
Merged

(#508) Fix some files not being removed during uninstall/upgrade #3151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
20d6dc1
(#508) Update snapshot service to respect version normalization
AdmiringWorm May 10, 2023
797cdf9
(#508) Remove nupkg file on partial file removals
AdmiringWorm May 10, 2023
8d1c7e2
(test) Remove foss only tags on fixed tests
AdmiringWorm May 10, 2023
a17a33d
(maint) Move GetStorePath before deprecations
corbob May 10, 2023
7cb721a
(test) Use no-reduce instead of reduce-nupkg-only
corbob May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 51 additions & 3 deletions src/chocolatey/infrastructure.app/services/ChocolateyPackageInformationService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ public ChocolateyPackageInformation Get(IPackageMetadata package)
return packageInformation;
}

var pkgStorePath = _fileSystem.CombinePaths(ApplicationParameters.ChocolateyPackageInfoStoreLocation, "{0}.{1}".FormatWith(package.Id, package.Version.ToStringSafe()));
var pkgStorePath = GetStorePath(_fileSystem, package.Id, package.Version);

if (!_fileSystem.DirectoryExists(pkgStorePath))
{
return packageInformation;
Expand Down Expand Up @@ -176,7 +177,8 @@ public void Save(ChocolateyPackageInformation packageInformation)
return;
}

var pkgStorePath = _fileSystem.CombinePaths(ApplicationParameters.ChocolateyPackageInfoStoreLocation, "{0}.{1}".FormatWith(packageInformation.Package.Id, packageInformation.Package.Version.ToStringSafe()));
var pkgStorePath = GetStorePath(_fileSystem, packageInformation.Package.Id, packageInformation.Package.Version);

_fileSystem.EnsureDirectoryExists(pkgStorePath);

if (packageInformation.RegistrySnapshot != null)
Expand Down Expand Up @@ -250,11 +252,57 @@ public void Save(ChocolateyPackageInformation packageInformation)

public void Remove(IPackageMetadata package)
{
var pkgStorePath = _fileSystem.CombinePaths(ApplicationParameters.ChocolateyPackageInfoStoreLocation, "{0}.{1}".FormatWith(package.Id, package.Version.ToStringSafe()));
var pkgStorePath = GetStorePath(_fileSystem, package.Id, package.Version);
if (_config.RegularOutput) this.Log().Info("Removing Package Information for {0}".FormatWith(pkgStorePath));
_fileSystem.DeleteDirectoryChecked(pkgStorePath, recursive: true);
}

private static string GetStorePath(IFileSystem fileSystem, string id, NuGetVersion version)
corbob marked this conversation as resolved.
Show resolved Hide resolved
{
var preferredStorePath = fileSystem.CombinePaths(ApplicationParameters.ChocolateyPackageInfoStoreLocation, "{0}.{1}".FormatWith(id, version.ToNormalizedStringChecked()));

if (fileSystem.DirectoryExists(preferredStorePath))
{
return preferredStorePath;
}

var pkgStorePath = fileSystem.CombinePaths(ApplicationParameters.ChocolateyPackageInfoStoreLocation, "{0}.{1}".FormatWith(id, version.ToStringSafe()));

if (fileSystem.DirectoryExists(pkgStorePath))
{
return pkgStorePath;
}

// Legacy handling for package version that was installed originally as 4.0.0.0

var versionFull = version.IsPrerelease
? "{0}-{1}".FormatWith(version.Version.ToStringSafe(), version.Release)
: version.Version.ToStringSafe();
pkgStorePath = fileSystem.CombinePaths(ApplicationParameters.ChocolateyPackageInfoStoreLocation, "{0}.{1}".FormatWith(id, versionFull));

if (fileSystem.DirectoryExists(pkgStorePath))
{
return pkgStorePath;
}

// Legacy handling for package versions that was installed originally as "4.2"

if (version.Version.Revision == 0 && version.Version.Build == 0)
{
versionFull = version.IsPrerelease
? "{0}.{1}-{2}".FormatWith(version.Major, version.Minor, version.Release)
: "{0}.{1}".FormatWith(version.Major, version.Minor);
pkgStorePath = fileSystem.CombinePaths(ApplicationParameters.ChocolateyPackageInfoStoreLocation, "{0}.{1}".FormatWith(id, versionFull));

if (fileSystem.DirectoryExists(pkgStorePath))
{
return pkgStorePath;
}
}

return preferredStorePath;
}

#pragma warning disable IDE1006
[Obsolete("This overload is deprecated and will be removed in v3.")]
public ChocolateyPackageInformation get_package_information(IPackageMetadata package)
Expand Down
15 changes: 13 additions & 2 deletions src/chocolatey/infrastructure.app/services/NugetService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1389,6 +1389,10 @@ public virtual ConcurrentDictionary<string, PackageResult> Upgrade(ChocolateyCon

continue;
}

// We need to remove the nupkg file explicitly in case it has
// been modified.
EnsureNupkgRemoved(oldPkgInfo.Package);
}

var endpoint = NuGetEndpointResources.GetResourcesBySource(packageDependencyInfo.Source);
Expand Down Expand Up @@ -2244,6 +2248,12 @@ public virtual ConcurrentDictionary<string, PackageResult> Uninstall(ChocolateyC
this.Log().Warn(logMessage);
packageResult.Messages.Add(new ResultMessage(ResultType.Error, errorlogMessage));

// As the only reason we failed the uninstall is due to left over files, let us
// be good citizens and ensure that the nupkg file is removed so the package will
// not be listed as installed. This is needed to do manually as the package may
// have been optimized by licensed edition of Chocolatey CLI.
EnsureNupkgRemoved(packageToUninstall.PackageMetadata, throwError: false);

// Do not call continueAction again here as it has already been called once.
//if (continueAction != null) continueAction.Invoke(packageResult, config);
continue;
Expand Down Expand Up @@ -2418,7 +2428,8 @@ protected void BackupCurrentPackageFiles(ChocolateyConfiguration config, IPackag
/// Ensure that the package is deleted or throw an error.
/// </summary>
/// <param name="removedPackage">The installed package.</param>
private void EnsureNupkgRemoved(IPackageMetadata removedPackage)
/// <param name="throwError">Whether failing to remove the nuget package should throw an exception or not.</param>
private void EnsureNupkgRemoved(IPackageMetadata removedPackage, bool throwError = true)
{
this.Log().Debug(ChocolateyLoggers.Verbose, "Removing nupkg if it still exists.");

Expand All @@ -2431,7 +2442,7 @@ private void EnsureNupkgRemoved(IPackageMetadata removedPackage)
FaultTolerance.TryCatchWithLoggingException(
() => _fileSystem.DeleteFile(nupkg),
"Error deleting nupkg file",
throwError: true);
throwError: throwError);
}

protected void NormalizeNuspecCasing(IPackageSearchMetadata packageMetadata, string packageLocation)
Expand Down
70 changes: 13 additions & 57 deletions tests/chocolatey-tests/commands/choco-uninstall.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,7 @@ Describe "choco uninstall" -Tag Chocolatey, UninstallCommand {
$Output.ExitCode | Should -Be 0 -Because $Output.String
}

# This does not work as expected when runnning in test kitchen and with Chocolatey Licensed Extension installed.
# It is believed to be a problem with test kitchen, or the VM that we are using that is causing the issue.
# The result is that versioned backup files of each file in the package is created, instead of the package being
# removed. Consider the test partially broken.
It "Should have removed lib package directory" -Tag FossOnly {
It "Should have removed lib package directory" {
"$env:ChocolateyInstall\lib\upgradepackage" | Should -Not -Exist
}

Expand Down Expand Up @@ -131,11 +127,7 @@ Describe "choco uninstall" -Tag Chocolatey, UninstallCommand {
"$env:ChocolateyInstall\lib\installpackage\a-locked-file.txt" | Should -Exist
}

# This does not work as expected when runnning in test kitchen and with Chocolatey Licensed Extension installed.
# It is believed to be a problem with test kitchen, or the VM that we are using that is causing the issue.
# The result is that versioned backup files of each file in the package is created, instead of the package being
# removed. Consider the test partially broken.
It "Should have removed file '<_>' in lib directory" -ForEach @('installpackage.nupkg', 'installpackage.nuspec', 'tools\casemismatch.exe', 'tools\Casemismatch.exe.ignore', 'tools\chocolateyBeforeModify.ps1', 'tools\chocolateyinstall.ps1', 'tools\chocolateyuninstall.ps1', 'tools\console.exe', 'tools\graphical.exe', 'tools\graphical.exe.gui', 'tools\not.installed.exe', 'tools\not.installed.exe.ignore', 'tools\simplefile.txt') -Tag FossOnly {
It "Should have removed file '<_>' in lib directory" -ForEach @('installpackage.nupkg', 'installpackage.nuspec', 'tools\casemismatch.exe', 'tools\Casemismatch.exe.ignore', 'tools\chocolateyBeforeModify.ps1', 'tools\chocolateyinstall.ps1', 'tools\chocolateyuninstall.ps1', 'tools\console.exe', 'tools\graphical.exe', 'tools\graphical.exe.gui', 'tools\not.installed.exe', 'tools\not.installed.exe.ignore', 'tools\simplefile.txt') {
"$env:ChocolateyInstall\lib\installpackage\$_" | Should -Not -Exist
}

Expand Down Expand Up @@ -172,23 +164,15 @@ Describe "choco uninstall" -Tag Chocolatey, UninstallCommand {
$LockedFile.Dispose()
}

It "Exits with Failure (1)" -Tag FossOnly {
It "Exits with Failure (1)" {
$Output.ExitCode | Should -Be 1 -Because $Output.String
}

It "Exits with Success (0)" -Tag Licensed {
$Output.ExitCode | Should -Be 0 -Because $Output.String
}

It "Should have kept locked file in lib directory" {
"$env:ChocolateyInstall\lib\installpackage\a-locked-file.txt" | Should -Exist
}

# This does not work as expected when runnning in test kitchen and with Chocolatey Licensed Extension installed.
# It is believed to be a problem with test kitchen, or the VM that we are using that is causing the issue.
# The result is that versioned backup files of each file in the package is created, instead of the package being
# removed. Consider the test partially broken.
It "Should have removed file '<_>' in lib directory" -ForEach @('installpackage.nupkg', 'installpackage.nuspec', 'tools\casemismatch.exe', 'tools\Casemismatch.exe.ignore', 'tools\chocolateyBeforeModify.ps1', 'tools\chocolateyinstall.ps1', 'tools\chocolateyuninstall.ps1', 'tools\console.exe', 'tools\graphical.exe', 'tools\graphical.exe.gui', 'tools\not.installed.exe', 'tools\not.installed.exe.ignore', 'tools\simplefile.txt') -Tag FossOnly {
It "Should have removed file '<_>' in lib directory" -ForEach @('installpackage.nupkg', 'installpackage.nuspec', 'tools\casemismatch.exe', 'tools\Casemismatch.exe.ignore', 'tools\chocolateyBeforeModify.ps1', 'tools\chocolateyinstall.ps1', 'tools\chocolateyuninstall.ps1', 'tools\console.exe', 'tools\graphical.exe', 'tools\graphical.exe.gui', 'tools\not.installed.exe', 'tools\not.installed.exe.ignore', 'tools\simplefile.txt') {
"$env:ChocolateyInstall\lib\installpackage\$_" | Should -Not -Exist
}

Expand All @@ -200,11 +184,11 @@ Describe "choco uninstall" -Tag Chocolatey, UninstallCommand {
"$env:ChocolateyInstall\lib-bkp\upgradepackage" | Should -Not -Exist
}

It "Reports no package uninstalled" -Tag FossOnly {
It "Reports no package uninstalled" {
$Output.Lines | Should -Contain "Chocolatey uninstalled 0/1 packages. 1 packages failed."
}

It "Outputs not able to remove all package files" -Tag FossOnly {
It "Outputs not able to remove all package files" {
$Output.String | Should -Match "installpackage - Unable to delete all existing package files. There will be leftover files requiring manual cleanup"
}
}
Expand All @@ -222,19 +206,15 @@ Describe "choco uninstall" -Tag Chocolatey, UninstallCommand {
$Output.ExitCode | Should -Be 0 -Because $Output.String
}

# When a file exists before initial installation, it will be considered as part of the
# package files. This is NuGet behavior. This happens during existing files for upgrades as well.
# We might want to rollback files in this case, but it is not possible as the backup has been removed before
# any locked files are being tried to be removed.
It "Should have removed <_>" -ForEach @('isdependency', 'hasdependency') -Tag FossOnly {
It "Should have removed <_>" -ForEach @('isdependency', 'hasdependency') {
"$env:ChocolateyInstall\lib\$_" | Should -Not -Exist -Because $Output.String
}

It "Should not have removed isexactversiondependency" {
"$env:ChocolateyInstall\lib\isexactversiondependency" | Should -Exist -Because $Output.String
}

It "Outputs <_> was succcesfully uninstalled" -ForEach @('isdependency', 'hasdependency') -Tag FossOnly {
It "Outputs <_> was succcesfully uninstalled" -ForEach @('isdependency', 'hasdependency') {
$Output.Lines | Should -Contain "$_ has been successfully uninstalled." -Because $Output.String
}

Expand All @@ -260,11 +240,7 @@ Describe "choco uninstall" -Tag Chocolatey, UninstallCommand {
$Output.ExitCode | Should -Be 0 -Because $Output.String
}

# This does not work as expected when runnning in test kitchen and with Chocolatey Licensed Extension installed.
# It is believed to be a problem with test kitchen, or the VM that we are using that is causing the issue.
# The result is that versioned backup files of each file in the package is created, instead of the package being
# removed. Consider the test partially broken.
It "Should have removed <_>" -ForEach @('isdependency', 'hasdependency', 'isexactversiondependency') -Tag FossOnly {
It "Should have removed <_>" -ForEach @('isdependency', 'hasdependency', 'isexactversiondependency') {
"$env:ChocolateyInstall\lib\$_" | Should -Not -Exist -Because $Output.String
}

Expand Down Expand Up @@ -299,19 +275,11 @@ Describe "choco uninstall" -Tag Chocolatey, UninstallCommand {
$Output.String | Should -Match "- uninstallfailure \(exited -1\) - Error while running"
}

# This does not work as expected when runnning in test kitchen and with Chocolatey Licensed Extension installed.
# It is believed to be a problem with test kitchen, or the VM that we are using that is causing the issue.
# The result is that versioned backup files of each file in the package is created, instead of the package being
# removed. Consider the test partially broken.
It "Should have uninstall package installpackage" -Tag FossOnly {
It "Should have uninstall package installpackage" {
"$env:ChocolateyInstall\lib\installpackage" | Should -Not -Exist -Because $Output.String
}

# This does not work as expected when runnning in test kitchen and with Chocolatey Licensed Extension installed.
# It is believed to be a problem with test kitchen, or the VM that we are using that is causing the issue.
# The result is that versioned backup files of each file in the package is created, instead of the package being
# removed. Consider the test partially broken.
It "Outputs successful uninstall of installpackage" -Tag FossOnly {
It "Outputs successful uninstall of installpackage" {
$Output.Lines | Should -Contain "installpackage has been successfully uninstalled." -Because $Output.String
}

Expand All @@ -333,11 +301,7 @@ Describe "choco uninstall" -Tag Chocolatey, UninstallCommand {
$Output.ExitCode | Should -Be 1 -Because $Output.String
}

# This does not work as expected when runnning in test kitchen and with Chocolatey Licensed Extension installed.
# It is believed to be a problem with test kitchen, or the VM that we are using that is causing the issue.
# The result is that versioned backup files of each file in the package is created, instead of the package being
# removed. Consider the test partially broken.
It "Should have removed package toplevelwithnesteddependencies" -Tag FossOnly {
It "Should have removed package toplevelwithnesteddependencies" {
"$env:ChocolateyInstall\lib\toplevelwithnesteddependencies" | Should -Not -Exist -Because $Output.String
}

Expand Down Expand Up @@ -379,11 +343,7 @@ Describe "choco uninstall" -Tag Chocolatey, UninstallCommand {
$Output.ExitCode | Should -Be 0 -Because $Output.String
}

# This does not work as expected when runnning in test kitchen and with Chocolatey Licensed Extension installed.
# It is believed to be a problem with test kitchen, or the VM that we are using that is causing the issue.
# The result is that versioned backup files of each file in the package is created, instead of the package being
# removed. Consider the test partially broken.
It "Should have removed <_>" -ForEach @('hasdependency', 'hasnesteddependency', 'isdependency', 'isexactversiondependency', 'toplevelhasexactversiondependency', 'toplevelwithnesteddependencies') -Tag FossOnly {
It "Should have removed <_>" -ForEach @('hasdependency', 'hasnesteddependency', 'isdependency', 'isexactversiondependency', 'toplevelhasexactversiondependency', 'toplevelwithnesteddependencies') {
"$env:ChocolateyInstall\lib\$_" | Should -Not -Exist -Because $Output.String
}

Expand Down Expand Up @@ -418,10 +378,6 @@ Describe "choco uninstall" -Tag Chocolatey, UninstallCommand {
$Output.ExitCode | Should -Be 0 -Because $Output.String
}

# This does not work as expected when runnning in test kitchen and with Chocolatey Licensed Extension installed.
# It is believed to be a problem with test kitchen, or the VM that we are using that is causing the issue.
# The result is that versioned backup files of each file in the package is created, instead of the package being
# removed. Consider the test partially broken.
It "Should have removed <_>" -ForEach @('hasdependency', 'isdependency') {
"$env:ChocolateyInstall\lib\$_" | Should -Not -Exist -Because $Output.String
}
Expand Down
Loading