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

CKAN should not remove ship folders if empty on uninstall #1191

Closed
pjf opened this issue Jun 26, 2015 · 5 comments
Closed

CKAN should not remove ship folders if empty on uninstall #1191

pjf opened this issue Jun 26, 2015 · 5 comments
Labels
Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Easy This is easy to fix
Milestone
User Visible Issues

Comments

@pjf
Copy link
Member

pjf commented Jun 26, 2015

As reported by @SkyMarhsal in #1189, the CKAN will remove subdirectories
(VAB, SPH, etc) if a mod that includes craft files (eg: FAR) is uninstalled
and the directory is then empty.

The CKAN should never be deleting these directories, and realistically we
should probably be creating the system Ships subdirectories if they
don't exist.

Hopefully this is a simple patch to ModuleInstaller. :)
@pjf pjf added Bug Something is not working as intended Easy This is easy to fix Core (ckan.dll) Issues affecting the core part of CKAN ★★☆ labels Jun 26, 2015
@pjf pjf added this to the User Visible Issues milestone Jun 26, 2015
This was referenced Jun 27, 2015
Closed
Closed
@RichardLake
Copy link
Contributor

Just to be clear is it removing ships/VAB or ships/@thumbs/ and subdirectories? The former should be impossible - the code only calls directory.delete in one place and that has a guard for the directories. However it was never updated with 1.0.0 to include @thumbs and its subdirectories ...

@martinnj
Copy link
Contributor

The addition is based on pjf comment "The CKAN should never be deleting these directories, and realistically we should probably be creating the system Ships subdirectories if they don't exist." which I took to mean if the directories are missing for whatever reason, just recreate them, even if they we're manually deleted. If I'm wrong I can close the RP.

@RichardLake
Copy link
Contributor

Just spent some time checking - I can get ckan to remove @thumbs/VAB, @thumbs/SPH and @thumbs but not VAB or SPH. If I am correct the "proper" fix is to add the three directories to the guard on line 773 of ModuleInstaller.cs

Even with a fix it will still be worthwhile recreating them but doing so in uninstall seems a odd location. Having them missing will cause issues when installing modules, or while playing KSP, not while uninstalling.

@martinnj
Copy link
Contributor

Right, I'll take another gander at it. Should I let the PR stay open or just close it and create another one once it's fixed?

@RichardLake
Copy link
Contributor

As far as I know either way would be fine - it isn't like there is discussion on the PR which would be lost if you made a new one.

@Postremus Postremus mentioned this issue Aug 22, 2015
Merged
pjf added a commit that referenced this issue Sep 1, 2015
@pjf
Merge #1405: Don't remove empty ship folders on uninstall
e6c26db 
* Postremus/#1191:
  protect the @thumbs folder as well
  Don't remove empty ship folders on uninstall also check for severall other directories which should not be removed fixes #1191
  Normalize every path before returning them otherwise this could lead to issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Easy This is easy to fix
Projects
None yet
Milestone
User Visible Issues
Development

Successfully merging a pull request may close this issue.

Recreates ship folders if missing.
4 participants
@pjf @martinnj @Postremus @RichardLake