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

Pkg.rm persists (broken) state #7054

Closed
mlubin opened this issue May 30, 2014 · 29 comments
Closed

Pkg.rm persists (broken) state #7054

mlubin opened this issue May 30, 2014 · 29 comments
Labels
packages Package management and loading

Comments

@mlubin
Copy link
Member

mlubin commented May 30, 2014

It has occurred a number of times that, say, Pkg.build() fails for some reason and the package gets into a broken state, where another Pkg.build() isn't sufficient to fix it (BinDeps scripts aren't perfect). The user's response is typically (and reasonably) to do Pkg.rm("foo"); Pkg.add("foo"), which surprisingly does absolutely nothing because the old version is just copied to and from .trash

Could we have an interface in Pkg to completely delete or reset the state of a package? Also, is Pkg.rm a reasonable name for moving a package directory to the recycle bin? It seems surprising for anyone who knows what unix rm does.

@IainNZ
Copy link
Member

IainNZ commented May 31, 2014

I was surprised by this in a happy way because it actually saves quite a bit of time on the PackageEvaluator runs. Keyword argument could be good.

@mlubin
Copy link
Member Author

mlubin commented May 31, 2014

I think both functionalities should be available, but there's a question of naming and which should be the default.

@Keno
Copy link
Member

Keno commented May 31, 2014

Pkg.nuke?

@nalimilan
Copy link
Member

Or Pkg.reinstall, doing both steps in one command.

@pao
Copy link
Member

pao commented Jun 2, 2014

I believe when this was in Pkg1, I had named it Pkg.obliterate. But nuke is certainly also evocative.

@StefanKarpinski
Copy link
Member

How about calling it ;rm -rf $(Pkg.dir(pkg))?

@mlubin
Copy link
Member Author

mlubin commented Jun 20, 2014

Will that work on windows?

@tkelman
Copy link
Contributor

tkelman commented Jun 20, 2014

No, but as of last night we now have rm(Pkg.dir(pkg); recursive=true) without going into shell mode

Edit: correction, it will work now because we are bundling msysgit. After switching to libgit it won't work.

@mlubin
Copy link
Member Author

mlubin commented Apr 10, 2015

Bump. I think this is still very confusing and unintuitive behavior.

@wildart
Copy link
Member

wildart commented May 14, 2015

I think, having .trash is a bad idea. There is already .cache exists to speed up package installation. Moving package in and out from .trash only complicates installation behavior. Current issue is an example. I vote to abandon whole .trash idea.

@StefanKarpinski
Copy link
Member

Scenario: you've done a ton of work on package X but haven't pushed those changes anywhere. Then you remove the requirement for package Y, which happens to cause you to no longer need to have X installed – so the package manager does rm -rf X and your hard work vanishes without a trace.

@StefanKarpinski
Copy link
Member

Regarding names: Pkg.add and Pkg.rm don't manipulate packages, they manipulate requirements – they add and remove entries in the REQUIRE file.

@nolta
Copy link
Member

nolta commented May 14, 2015

I agree with @mlubin -- Pkg.rm should rm.

The current behavior could be called Pkg.disable or Pkg.subtract or something.

@wildart
Copy link
Member

wildart commented May 14, 2015

Inform that package is "dirty" and let user decide if "force" remove package. In case of automatic change resolution, same thing should happen.

@mlubin
Copy link
Member Author

mlubin commented May 14, 2015

Understanding Pkg.rm shouldn't require understanding how Pkg works internally (through the REQUIRE file). That said, Pkg should never run Pkg.rm if not directly asked to by the user. It could still move the package to .trash if it wants to.

@wildart
Copy link
Member

wildart commented May 14, 2015

I would not complicate, package manager with additional functions. I bet an average user (not a package developer) uses only: add, rm, update and 'build` (only because it is prompted). The rest of functionality should be resolved manually.

@wildart
Copy link
Member

wildart commented May 14, 2015

Understanding Pkg.rm shouldn't require understanding how Pkg works internally

Exactly, user sees Pkg.add and first thing that comes to mind - install package, Pkg.rm - uninstall.

@tkelman
Copy link
Contributor

tkelman commented May 14, 2015

Understanding Pkg.rm shouldn't require understanding how Pkg works internally (through the REQUIRE file).

This is key. Dependency resolution is not a concept everyone has an intuitive grasp over, and it's a pretty high bar to put up to suggest people need to understand how all the internals work in order to install and remove packages. My suggestion in #11262 is to at least store the state of whether a package has built properly, and not bother keeping broken packages around when they're removed. The state of whether there have been local modifications made could also be worth looking at, to address the loss-of-work concern.

The much more common use case is a frustrated new user wanting to start over from scratch and try again, but the way things are set up right now requires jumping through some very non-obvious hoops and learning a lot of new things at once to do that properly. I don't usually say this about most things in the language, but for Pkg we should try to make day-to-day use for inexperienced users as easy and intuitive as possible. Things that only matter for people who are developing and making modifications to packages are secondary concerns IMO, we should make them work of course but the tolerance for inelegance should be a little higher in these corners of the system.

@stevengj
Copy link
Member

stevengj commented Jul 8, 2015

I agree; it's so frustrating that Pkg.rm doesn't do what is expected. It should nuke the package, full stop. The internal dependency resolution can call some lower-level function (I like Pkg.subtract: it is a play on add and is also obscure enough to ward off casual use) when a package requirement is removed.

@tkelman
Copy link
Contributor

tkelman commented Jul 8, 2015

I also like the name Pkg.subtract. A more consequential version of Pkg.rm would have to do the same step as Pkg.subtract of removing from REQUIRE first, then hard-delete the package, then call resolve.

@blegat
Copy link
Contributor

blegat commented Aug 24, 2016

I agree that Pkg.rm is extremely confusing. Nothing is more confusing for a user than an unexpected internal state that is kept.

I suggest something like

  • Pkg.add and Pkg.rm would be like git add and git rm, it is the same than adding it by hand or doing the shell rm except that Julia (or git in my comparison) is notified properly. It removes it as the user expect it to be. In case of dirty state, there is an error and there could be a switch (similar to rm -f) Pkg.rm(force=true) to ignore dirty state.
    Of course, it would be disastrous to lose unpushed commit that way. So, internally, we could use a .trash folder but without altering the above behavior so that the user can think that we actually removed it. Only the user having lost his unpushed commit will be relieved to learn that it is actually in a trash.
    It would be something like
if dirty
  if force
    git clean -d -x -f
  else
    error("This is dirty, use force=true or clean it")
  end
end
disable it
move it to .trash

Pkg.add would get it back from the trash and checkout the tag of the latest version in METADATA and Pkg.checkout would get it back from the trash and checkout the master branch. With that behavior, the fact that it is put to the trash does not alter the end result to the user (except that he does not loses his commits) since it had removed the uncommitted work and it checkouts the correct branch.

  • Pkg.enable and Pkg.disable would be like systemctl enableand systemctl disable, it makes the package virtually disappear for julia while the package is still there (not in .trash) for the user because it does not need to be hidden from him, only hidden from Julia. If it is dirty, it stays dirty.
  • Pkg.build(clean=true) would also be nice as this is a so common use case (but this is discussed in Pkg.build() needs a corresponding Pkg.build(clean=true) or Pkg.buildclean() or ... #13892).

What do you think ?

@tkelman
Copy link
Contributor

tkelman commented Aug 24, 2016

you'd need a git clean in there somewhere to remove uncommitted work or things like files downloaded by bindeps, which is often the cause of this issue

@blegat
Copy link
Contributor

blegat commented Aug 24, 2016

Yes that is what I meant by git checkout ., I have editted my post to make this clearer.

@tkelman
Copy link
Contributor

tkelman commented Aug 24, 2016

an issue is that you need clean -x since many of these files are (correctly) gitignored to avoid normal operation resulting in an update-preventing dirty state.

@blegat
Copy link
Contributor

blegat commented Aug 24, 2016

It even seems that I need -d to delete directories and -f

@odysseus9672
Copy link
Contributor

odysseus9672 commented Jul 28, 2017

Agree that having rm not delete anything is confusing behavior when compared to the command line. Since this is about package management, I suggest expanding the name of rm to remove, and adding a function named purge that does delete, consistent with the usage of the aptitude package manager. Suggested behavior of the purge command:

  1. all effects of remove
  2. delete the actual package files
  3. throw an error if other packages depend on the named package, and name the dependent packages
  4. have an optional keyword boolean valued argument that defaults to false for recursive purge.

This is all consistent with the behavior of the aptitude package manager (and similar ones like fink).

@stevengj
Copy link
Member

I still think rm (or remove) should actually remove the package, with a remove(force=true) to do so even if the package is dirty. A lower-level function like Pkg.subtract can do what rm does now.

@tkelman
Copy link
Contributor

tkelman commented Jul 28, 2017

That's pretty much what I tried in db0cb23 (initial version of #12350) but that didn't really go through. Pkg3 will do this differently, hopefully enough that it actually fixes this.

@StefanKarpinski
Copy link
Member

The proposed change as implemented in #7054 will never see the light of day since it's more than a bugfix so it can't be in 0.6.x and the current package manager will be fully replaced in 0.7/1.0. If there's a pure bugfix change that can be done here, that would be a good 0.6 PR to make, but otherwise I'm inclined to close this as "won't fix".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading
Projects
None yet
Development

Successfully merging a pull request may close this issue.