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.

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

Uninstallation does not delete files extracted to package directory by chocolateyInstall.ps1 #121

Closed
jberezanski opened this issue Feb 25, 2015 · 18 comments

Comments

@jberezanski
Copy link

When a package is uninstalled, only files contained in the nupkg are removed from the package directory. If the package install script put extra files there (e.g. using the common pattern for zip packages - extract the downloaded zip to package directory), those files are left behind when the package is uninstalled and the package directory is not deleted.

In PS Choco, the package directory was unconditionally deleted.

Is the change intentional? If yes, will the auto uninstaller service eventually handle removing such files or will package maintainers have to explicitly code it in chocolateyUninstall.ps1?

@ferventcoder
Copy link
Member

choco clean will handle additional cleanup.

@ferventcoder
Copy link
Member

And there is choco uninstall --force

@ferventcoder
Copy link
Member

If you want, we can call this a documentation issue?

@dhilgarth
Copy link

Would you encourage adding an uninstall script that removes those files via Uninstall-ChocolateyZipPackage? Or would you discourage it? Or don't you have a preference? Looking for best practices here.

@ferventcoder
Copy link
Member

I think I misunderstood this issue when I first read it. I think we'll want to automatically look for those unzipped files (based on that file that is created) and remove them as part of uninstall.
And any downloaded files.

@dhilgarth
Copy link

Great, I totally agree! So, the manual call to Uninstall-ChocolateyZipPackage would be unnecessary?

@ferventcoder
Copy link
Member

Yes

On Thursday, February 26, 2015, Daniel Hilgarth notifications@github.com
wrote:

Great, I totally agree! So, the manual call to
Uninstall-ChocolateyZipPackage would be unnecessary?


Reply to this email directly or view it on GitHub
#121 (comment).

Rob
"Be passionate in all you do"

http://devlicio.us/blogs/rob_reynolds
http://ferventcoder.com
http://twitter.com/ferventcoder

@rismoney
Copy link
Contributor

love it.

@jberezanski
Copy link
Author

Great! I was concerned about the increased workload for maintainers of zip-based packages and the need to modify all such existing packages to retain current uninstall behavior. Besides, it seemed contrary to the goal of eliminating the need for chocolateyUninstall.ps1 in common cases.

@ferventcoder
Copy link
Member

@jberezanski Yeah sorry I misunderstood what you were talking about before...

@bc3tech
Copy link
Contributor

bc3tech commented Mar 28, 2015

I have a pull request prepped that improves Uninstall-ChocolateyZipPackage to work more robustly. I found in the case of one package that uses ZIP install, the call to Uninstall-ChocolateyZipPackage would hang on specifiers in the .zip.txt file that were directory names and not files. My improvement solves this and successfully deletes all files denoted in the .zip.txt file contained in the package's lib directory.

@ferventcoder: You want I should submit the pull request?

@ferventcoder
Copy link
Member

@bc3tech Yeah we can improve uninstall-chocolateyzippackage for now.

@bc3tech
Copy link
Contributor

bc3tech commented Mar 29, 2015

@ferventcoder pull request submitted. Thanks! hopefully it's not too crazy.

@ferventcoder ferventcoder modified the milestones: 0.9.9.5, 0.9.10 Apr 2, 2015
@ferventcoder ferventcoder modified the milestones: 0.9.9.5, 0.9.9.6 Apr 20, 2015
@AnthonyMastrean
Copy link

To be clear, the Uninstall-ChocolateyZipPackage helper doesn't work at all, as-is. It hangs waiting for confirmation (I recreated the relevant chunk in my shell)...

PS> $zipContentFile = 'C:\ProgramData\chocolatey\lib\tinycc\tinyccInstall.zip.txt'
PS> $zipContents=get-content $zipContentFile
>>> foreach ($fileInZip in $zipContents) {
>>>   remove-item "$fileInZip" -ErrorAction SilentlyContinue
>>> }

Confirm
The item at C:\ProgramData\chocolatey\lib\tinycc\tools\tcc has children and the Recurse parameter was not
specified. If you continue, all children will be removed with the item. Are you sure you want to continue?
[Y] Yes  [A] Yes to All  [N] No  [L] No to All  [S] Suspend  [?] Help (default is "Y"):

You need, at least, the -Force and -Recurse flags to ignore all prompts.

Then you should consider that the zip contents text file starts with the top-most directory.

C:\ProgramData\chocolatey\lib\tinycc\tools\tcc
C:\ProgramData\chocolatey\lib\tinycc\tools\tcc\libtcc
C:\ProgramData\chocolatey\lib\tinycc\tools\tcc\libtcc.dll
C:\ProgramData\chocolatey\lib\tinycc\tools\tcc\tcc.exe
C:\ProgramData\chocolatey\lib\tinycc\tools\tcc\tiny_impdef.exe
C:\ProgramData\chocolatey\lib\tinycc\tools\tcc\tiny_libmaker.exe

The Remove-Item loop will recursively delete the top-level folder, and fail to delete all the children. For example, without the Error Action specified...

Remove-Item : Cannot find path 'C:\ProgramData\chocolatey\lib\tinycc\tools\tcc\libtcc.dll' because it does not exist.
At line:1 char:65
+ Get-Content C:\ProgramData\chocolatey\lib\tinycc\tinyccInstall.zip.txt | Remove-Item -For ...
+                                                                 ~~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (C:\ProgramData\...libtcc\libtcc.dll:String) [Remove-Item], ItemNotFoundException
    + FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.RemoveItemCommand

The -ErrorAction SilentlyContinue parameter will suppress that error. Or, you could add a Test-Path into the pipeline. I'd rewrite that whole loop in one of two ways

Get-Content $zipContentFile | ?{ Test-Path $_ } | Remove-Item -Force -Recurse

Or you might still want to silently continue past all other possible errors (which are?)

Get-Content $zipContentFile | Remove-Item -Force -Recurse -ErrorAction SilentlyContinue

In either case, what is the recommendation for package authors until this bug is fixed? Do we allow the zip files and, for packages that unzip into the install directory, the sparsely deleted package directory to stick around?

ferventcoder pushed a commit to ferventcoder/choco that referenced this issue Apr 20, 2015
…when deleting files that were copied during installation of the Zip package
ferventcoder added a commit to ferventcoder/choco that referenced this issue Apr 20, 2015
* patch-1:
  (chocolateyGH-121) Making Uninstall-ChocolateyZipPackage more robust when deleting files that were copied during installation of the Zip package
ferventcoder added a commit that referenced this issue Apr 20, 2015
* stable: (22 commits)
  (GH-121) Making Uninstall-ChocolateyZipPackage more robust when
deleting files that were copied during installation of the Zip package
  (doc) update changelog/nuspec
  (GH-238) ApiKey source matching intuitive
  (maint) formatting
  (GH-240) Set CredentialProvider for NuGet
  (GH-240) ChocolateyNugetCredentialProvider
  (GH-240) Add default sources to machine sources
  (maint) Only warn subcommand list if not empty
  (GH-171) Use RedirectedHttpClient
  (GH-240) pass credentials at runtime
  (GH-240)(config) Add machine sources
  (doc) how to quote values
  (GH-230) Export all functions and aliases imported
  (GH-230) Fix Issues with Generate/Remove BinFile
  (GH-185) Remove console prompt default choices
  (GH-186) Uninstall - no prompt for one version
  (GH-182) Ask before printing ps1 scripts
  (GH-187) Show log file path in messages.
  (maint) formatting
  (GH-169) Do not resolve disabled sources
  ...

Conflicts:
	src/chocolatey/infrastructure.app/commands/ChocolateySourceCommand.cs
	src/chocolatey/infrastructure.app/runners/GenericRunner.cs
@ferventcoder
Copy link
Member

So the fixes from @bc3tech went into 0.9.9.5. So there is that path forward for now.

@ferventcoder
Copy link
Member

This remains open as the ultimate fix here is to capture what is in the folder at install and clean it up if it hasn't changed.

@AnthonyMastrean
Copy link

Awesome :)

In the past, I leaned heavily on Chocolatey removing the entire install directory, especially for locally extracted zip packages.

ferventcoder added a commit to ferventcoder/choco that referenced this issue May 4, 2015
Define the package files classes and the package information
file they will produce.
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 4, 2015
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 4, 2015
Add ability to read and save package file information.
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 4, 2015
Add the interaction to the package information service to read the
files file and save to the files file.
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 4, 2015
Allow IFileSystem to read a file and return the file bytes as an array.
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 4, 2015
To compute hashes in files, add a hash provider that can determine
hashes of files.
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 4, 2015
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 4, 2015
Capture the package hashes after everything, including the
install script, has had a chance to run.
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 4, 2015
- Rename Md5HashProvider to CryptoHashProvider and provide enumeration
   for HashAlgorithm Crytpo providers.
- Implement adapter for HashAlgorithm.
- Update specs for CryptoHashProvider
- Specs for FilesService
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 13, 2015
When the files that were installed during install are unchanged, they
should be removed from the package directory. Any files that have
changed should stick around.
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 13, 2015
While adding scenarios it was discovered that the empty directories
were not being removed, so the service will ensure those are removed
upon removing a package with no changed files in it.
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 15, 2015
The entire package checksum is not needed at this time so remove it.
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 15, 2015
ferventcoder added a commit to ferventcoder/choco that referenced this issue May 15, 2015
When files have changed in a package, back those files up prior to
upgrade.
ferventcoder added a commit that referenced this issue May 15, 2015
…napshot

(GH-121) Capture And Remove Files Related to Package Installation
@ferventcoder
Copy link
Member

This is fixed in 3bb8421

@ferventcoder ferventcoder self-assigned this May 15, 2015
ferventcoder added a commit that referenced this issue May 15, 2015
* stable: (27 commits)
  (maint) formatting
  (GH-170) Add outdated command
  (maint) move magic string to constants
  (maint) Command specs for new options
  (maint) harden removing install files
  (GH-121) backup changed files
  (GH-121) IFilesService capture files by directory
  (GH-121) Remove package checksum
  (maint) renaming results to appropriate singular
  (GH-121) Remove empty directories / scenarios
  (maint) comment
  (maint) prefix "[NuGet]" on Nuget logger
  (GH-121) Remove unchanged files on uninstall
  (GH-121) Specs and Files Service hardening
  (GH-121) Capture file hashes
  (GH-121) specs for Md5HashProvider
  (GH-121) Add hash provider
  (GH-121) IFileSystem.read_file_bytes
  (GH-121) PkgInfoService files service interaction
  (GH-121) Add files service
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants