dom96 / choosenim

Tool for easily installing and managing multiple versions of the Nim programming language.
BSD 3-Clause "New" or "Revised" License
680 stars 64 forks source link

Nimarchive cannot overwrite existing extraction? #199

Closed dom96 closed 4 years ago

dom96 commented 4 years ago

Getting this error which makes choosenim unusable:

PS C:\Users\morfe\projects\stardust> choosenim 1.2.0
      Info: C:\Users\morfe\.choosenim\downloads\dlls.zip already downloaded
 Extracting dlls.zip
     Error: Unable to extract. Error was 'Cannot create a file when that file already exists.
        ... '.

The extraction is into nimble bin dir (the same as returned by choosenim --getNimbleBin). So it fails because it's trying to extract DLLs that are already there...

Steps to repro:

You will see that error.

dom96 commented 4 years ago

In my scenario, I wouldn't actually want choosenim to overwrite the files. I think choosenim should write a file that signifies that the DLLs have been installed, and then whatever the user does to the DLLs later choosenim should leave them alone. i.e. choosenim should only install the DLLs once on a system (we can have a big warning about this)

genotrance commented 4 years ago

I don't see a problem with overwriting a partial list of DLLs since it means that the last time they were downloaded and extracted was months if not years ago.

It's likely that the new zip might have updated versions and the issue will only recur when we update the choosenim code again to include some more DLLs in the check, which means they should be redownloaded anyway.

genotrance commented 4 years ago

nimarchive calls moveFile() which is documented with If dest already exists, it will be overwritten.

If you see the code for Windows however, it fails if file already exists. Per the docs, MoveFileExW requires the MOVEFILE_REPLACE_EXISTING flag to overwrite.

I can change nimarchive to delete before writing but this really needs to be fixed in the stdlib.

cc @vegansk @yglukhov @timotheecour

timotheecour commented 4 years ago

Please also file bug in nim repo!

genotrance commented 4 years ago

Meanwhile, setting the MOVEFILE_REPLACE_EXISTING flag results in an access denied error since choosenim is using libeay64.dll so it cannot be overwritten. Even if this is fixed in the stdlib, we still need some changes.

nimarchive is doing the extraction and simply fails when a file copy fails, no way to handle this scenario. So proposal is for choosenim to extract to another directory and then copy each dll and handle errors.

timotheecour commented 4 years ago

So proposal is for choosenim to extract to another directory and then copy each dll and handle errors.

that should definitely be done (likewise with nimble if it doesn't do so, although I don't remember running into this). That's what hoembrew does too

code is here https://github.com/Homebrew/brew/blob/master/Library/Homebrew/cmd/upgrade.rb but it's enough to look at logs: brew upgrade --verbose grpc => see log here https://gist.github.com/timotheecour/12606fb89a278a39c5aeddeb4f37de69

In my scenario, I wouldn't actually want choosenim to overwrite the files. I think choosenim should write a file that signifies that the DLLs have been installed, and then whatever the user does to the DLLs later choosenim should leave them alone. i.e. choosenim should only install the DLLs once on a system (we can have a big warning about this)

that's a recipe for inconsistent state. Again, homebrew handles this scenario well and doesn't result in inconsisent state; I can provide more details if needed.