RFC: add cleandeps keyword argument to Pkg.rm

[tkelman]

and rename current Pkg.rm to Pkg.subtract original commit db0cb23 was replaced

should fix #7054, probably a simpler alternative to #11262

I can't really think of a situation where I call Pkg.rm and really strongly want to keep that package around in .trash. If I did this correctly it still does dependency resolution in a way that moves no-longer-required packages to .trash, but actually deletes the specific package you asked to remove when you call Pkg.rm.

[IainNZ]

Interesting. I guess I'd switch PkgEval to Pkg.subtract then

[aviks]

I don't really like this. Is this really necessary?

Pedantically, the opposite of add is remove in practical English. You add water to a bowl, and remove it. Or coins to a jar, or balls to a bag.... you get the idea. You only subtract numbers, not any other object.

If rm semantics are incorrect or confusing, we should fix that. Adding a subtract method will result in one of two situations

1. Nobody discovers subtract and continues to use rm for better or worse
2. Everyone is perpetually confused by the difference between subtract and rm

[kmsquire]

I agree with @aviks that remove is a the common name for this.

A more nuanced approach would be to remove the package if nothing depends on it, and if anything depends on it, print a message like "Package X cannot be fully removed because packages Y and Z depend on X. X will be removed automatically when all packages which depend on it are removed."

[carlobaldassi]

What @kmsquire said. The only tricky part is that e.g. package X (to be removed) could depend on package Y, which is not explicitly required, but is itself required by package Z. It shouldn't be too hard to write though, one just needs to explore the graph outwards until explicitly required packages are found.

One more complete approach for this kind of info message could be:

• check which packages are the most direct ones which require X (as in the above);
• simulate removal of those by calling Pkg.resolve on a reduced, subset (this should be fairly fast);
• iterate until package X is no longer part of the output of Pkg.resolve;
• print the list of packages which had to be removed in order to reach the result.

At this point, one could even add a force keyword argument (or similar name) to actually apply the last step.

[tkelman]

Did you read the discussion in #7054 first? The status quo is broken. subtract is a pun because it's not what users actually want, 99% of the time they call Pkg.rm.

The problem is the semantics don't match user expectations for how this should behave, and without fundamentally rewriting Pkg that's difficult to change. Modifying the REQUIRE file, calling resolve, and moving things to and from .trash are really implementation details that only people who've actually looked at the code are going to have a full grasp of (and even then it's filled with corner cases). Most people expect that when you remove a package, it gets deleted, not moved around to some hidden folder then brought back exactly in the (often broken) state it started from.

If the Pkg code actually had any comments then I'd think about a more dramatic refactoring. As things stand right now, if you call Pkg.rm twice on a registered package that something else depends on, the first time just takes it out of REQUIRE but otherwise does nothing, then the second time moves it to .trash thinking it's unregistered - ref #7757, rather buggy.

If we want Pkg to do real sophisticated dependency resolution we should use a satisfiability solver, probably the one that zypper and dnf use. In the meantime, Pkg.rm should actually rm something.

[lstagner]

Perhaps Pkg.nuke

[StefanKarpinski]

What if when we move things to .trash we cleaned out any generated things in deps? That seems to be the major issue people have with moving things to trash, whereas the main reason for moving things to .trash is to try to avoid ever deleting the user's work.

[tkelman]

Do you have a good heuristic for what makes for a "generated thing" as opposed to "user's work" ?

[carlobaldassi]

If we want Pkg to do real sophisticated dependency resolution we should use a satisfiability solver

We do use a satisfiability solver. (One specialized for package dependencies.) I'm not aware of any problems with it.

I think the problem with .trash of #7054 is orthogonal, and more likely to be resolved along the lines @StefanKarpinski proposes.
As a heuristic, we could wipe things in deps which are not checked in? Or even just move them in a sub-trash of that directory (say, a deps/trash-XYZ where the XYZ part is unique, e.g. a timestamp)?

[tkelman]

Our dependency resolver is a bit slow (worse on first-run, much of it isn't precompiled right now), and it's nowhere near the user-friendliness of what zypper, dnf, apt-get etc will tell you in terms of conflicts. Lack of accessibility or extensibility is a problem. The API's kind of a mess in terms of implementing any of your "more complete approach[es]."

.trash is orthogonal to what? That's what I'm proposing modifying here. So mentioning dependency resolution here is the off-topic part.

The other alternative is saving more state in the system re: #11262, which I haven't implemented a prototype for since it's a more complicated change.

I guess we could git clean -fdx deps before moving to .trash, that might work. Though for the PackageEvaluator use case you'd probably rather leave the built artifacts in place if they were working correctly.

[StefanKarpinski]

Doing a full git clean kind of defeats the purpose. Just cleaning deps would be good.

[carlobaldassi]

Our dependency resolver is a bit slow (worse on first-run, much of it isn't precompiled right now)

Precompilation is easy, anyway this is off-topic here. At any rate, the bottleneck there seems to be the part where the current status is read (Read.available() etc.), the actual dependency resolution is quite fast, I can't even see it in profiling (please open an issue if you find otherwise).
Also, please do open an issue if you have examples in which conflicts are not reported clearly.

.trash is orthogonal to what?

To the API. See Stefan's comments.

So mentioning dependency resolution here is the off-topic part.

Sorry, my first post was mostly a warning about what @kmsquire proposed, and a note for whomever may want to tackle that (possibly myself).

[tkelman]

the actual dependency resolution is quite fast

Ah, good to know, you've profiled this in much more detail than I have. I meant resolve is slow overall, but if most of that is filesystem then #11196 should help.

Also, please do open an issue if you have examples in which conflicts are not reported clearly.

#7757 is certainly a bug. The message from #4108 would be good to bring back to explain why Pkg.rm doesn't always do anything. And the code should generally be commented by someone who knows what's going on - I found myself asking what edit is supposed to do, what makes it return true vs false, etc. It's far from self-documenting, if there even is such a thing.

To the API. See Stefan's comments.

Okay. Stefan didn't say anything about the API. If he or anyone else had suggested cleaning deps anywhere in #7054 then that probably would've been prototyped by now. I'll try that and replace this branch, probably add a keyword argument cleandeps = true so PackageEvaluator or users who know what they're doing can opt in to preserving the build artifacts.

Just cleaning deps would be good.

Yes, quoting git clean --help, "If any optional ... arguments are given, only those paths are affected."

[tkelman]

Okay, take a look at this version. Seems to do the right thing in local tests.

[aviks]

Did you read the discussion in #7054 first?

Apologies for not being clear. I agree rm is broken in various ways, I just wanted to say that subtract is a pun too far. Thanks for tackling this! I don't have anything more substantive to say at the moment, so don't mind me .. :)

[tkelman]

Ah, gotcha. Maybe had you led with "-1 for the subtract pun" I would've gotten your point more directly. Sorry for being snappy, I've been coming across as more negative/hostile than intended lately. Maybe I should start throwing in more emoji 😔

Anyway, if people like the name/behavior of the kwarg then there's just the decision of whether it should default to true or false. IMO we should be changing the default behavior here since so many users have hit BinDeps trouble, tried Pkg.rm; Pkg.add, and gotten stuck. Adding a new function name or a non-default option to do the more reliable thing seems a lot less discoverable to me, we'll have to tell people how to do it. Pkg.rm is usually a pretty good sign that "I don't want this anymore."

[aviks]

Sorry for being snappy....

Don't worry about it. I was imprecise in my comment above. Your response was perfectly understandable.

[wildart]

I have mixed feelings about .trash issue. I say we should not touch .trash until we find solution how to track changes in all local branches of removed packages, so these changes would not disappear upon deletion. Or provide some mechanism of withholding from removal packages which are under the development. As for binary dependencies, I prefer a separate action, clean or reset, which would reset state of the packages to original state. If .trash is a real problem, consider adding something like expunge which would delete everything from .trash (interactively?).

[tkelman]

I'm very strongly against a separate action on the grounds that it's not discoverable enough, unless we go the originally proposed route of making rm do the reliable thing and hide the current rm behavior with moving to .trash under a different name.

The packages get moved to .trash with .git information intact, so they're still local repositories with multiple branches.

The intent here is that running git clean -qdfx deps only happens for the specific package that the user manually calls Pkg.rm on, not any other packages that get moved to .trash by dependency resolution. Correct me if this implementation does not do that.

[tkelman]

How about we use -X instead of -x so that this cleans only files/dirs listed in .gitignore? That makes this a little more opt-in and lower-risk in terms of what it is allowed to delete. Puts more burden on packages to list everything correctly in gitignore, but I think many of them have been pretty good about that.

[tkelman]

cc @carnaval @amitmurthy on that travis failure https://gist.github.com/tkelman/e69eae8a9718952101eb

[tkelman]

Maybe we should just add, and very clearly document and encourage, a Pkg.clean convenience function that runs git clean -fdx on a package for you, but only when you explicitly ask for it.

[KristofferC]

Obsolete with the new package manager, if still applicable needs to be reopened against Pkg.jl