uninstall: refuse when dependents still installed #1082

alyssais · 2016-09-22T12:31:30Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew tests with your changes locally?

Makes brew uninstall foo (but not brew uninstall --force foo) refuse to uninstall foo if kegs that depend on it are still installed.

Closes #934.

alyssais · 2016-09-22T12:46:00Z

Here's what I've tested with:

brew install feedgnuplot
brew uninstall gnuplot # check it refuses
brew uninstall --force gnuplot # check it uninstalls
brew uninstall feedgnuplot # check it uninstalls (because nothing depends on it)

vladshablinsky · 2016-09-22T19:11:40Z

Does it make sense to rename Keg#formula to Keg#to_formula similar to Dependency#to_formula method?

MikeMcQuaid · 2016-09-23T07:37:25Z

Library/Homebrew/cmd/uninstall.rb

+          dependants_output = dependants.map { |k| "#{k.name} #{k.version}" }.join(", ")
+          conjugation = dependants.count == 1 ? "is" : "are"
+          ofail "Refusing to uninstall #{keg} because it is required by #{dependants_output}, which #{conjugation} currently installed."
+          puts "Remove it anyway with `brew uninstall --force #{keg.name}`."


Maybe You can override this and force removal with brew uninstall --force ...

MikeMcQuaid · 2016-09-23T07:38:08Z

@penman From my reading this just handles direct and not recursive dependencies. What do you think about handling this recursively?

alyssais · 2016-09-23T08:03:51Z

@MikeMcQuaid it should handle recursive dependencies! It uses the list of dependencies from the tab, which is based on the recursive dependency list.

MikeMcQuaid · 2016-09-23T08:10:58Z

@penman 👏

MikeMcQuaid · 2016-09-23T08:33:26Z

@penman @ilovezfs made a good point to me privately: this probably shouldn't overload --force (although I realise that was my idea originally) so instead should be something like --ignore-dependents or something?

ilovezfs · 2016-09-23T08:56:34Z

I think the brew code mostly uses "dependent" not "dependant"

alyssais · 2016-09-23T09:21:40Z

@MikeMcQuaid @ilovezfs so brew uninstall --force would also refuse to uninstall a keg with dependents without --ignore-dependents?

ilovezfs · 2016-09-23T09:24:45Z

Right, I think the two options shouldn't directly interact with each other.

DomT4 · 2016-09-23T20:22:09Z

I wonder if this PR breaks brew bundle as-is, given brew bundle IIRC doesn't try to remove things in order of leaves but simply alphabetically.

--ignore-dependents or something?

I'm quite keen to avoid dependents or dependants because people will absolutely & regularly mistype that flag. It should be something fairly simple, and less likely to be bashed wrongly into the keyboard.

MikeMcQuaid · 2016-09-24T07:45:20Z

I'm quite keen to avoid dependents or dependants because people will absolutely & regularly mistype that flag. It should be something fairly simple, and less likely to be bashed wrongly into the keyboard.

Was thinking the same (but we probably don't really want people typing it anyway...).

ilovezfs · 2016-09-24T08:48:27Z

--ignore-dependents and --ignore-dependencies (anyone want to volunteer to write a paragraph of documentation for the man page explaining the difference) seem like developer language spilling over into the CLI.

As for whether we want people typing it, if you need to uninstall and reinstall something (for example: we didn't bump the revision but someone needs the change), they're going to have to type it, unless we want to force them to uninstall every dependent, which would almost certainly be unnecessary since otherwise there would have been a revision bump. If this PR is merged, that option will very much so be exposed to users whether we like it or not.

MikeMcQuaid · 2016-09-24T15:00:11Z

IMO this is definitely going to be more trouble than its worth

This is the default behaviour of pretty much every other package manager. What makes them different to us?

MikeMcQuaid · 2016-09-24T15:03:08Z

The difference is users are accustomed to this behavior and you're going to break user scripts

If the status quo is the only justification I don't think it's good enough to not implement the functionality.

ilovezfs · 2016-09-24T15:05:43Z

This is the default behaviour of pretty much every other package manager. What makes them different to us?

We're not managing system components. I'd start there.

MikeMcQuaid · 2016-09-24T15:20:56Z

@zmwangx Many of our users would disagree that we don't have anything mission critical. I'm open to different approaches to this functionality but needs to go in and be on by default to be useful.

ilovezfs · 2016-09-24T15:31:41Z

When was the last time someone filed an issue due to uninstalling something they shouldn't have? I don't think we actually see that user problem in the wild much since people tend to install things and then leave them alone.

If anything this PR will probably inadvertently cause more such issues by sending people on cleaning sprees since uninstall is "safe" now, when in fact due to undeclared dependencies, dependencies hidden behindif build.with? options, and anomalies of the requirement system, it won't actually be safe.

ilovezfs · 2016-09-24T15:38:13Z

@zmwangx but I bet it fails if you try to uninstall rlwrap, yes?

MikeMcQuaid · 2016-09-24T15:40:22Z

If anything this PR will probably inadvertently cause more such issues by sending people on cleaning sprees since uninstall is "safe" now, when in fact due to undeclared dependencies, dependencies hidden behindif build.with? options, and anomalies of the requirement system, it won't actually be safe.

That doesn't matter because we track the actual used dependencies.

MikeMcQuaid · 2016-09-24T15:40:52Z

I don't think we actually see that user problem in the wild much since people tend to install things and then leave them alone.

We regularly see issues with brew missing output from brew doctor.

ilovezfs · 2016-09-24T15:41:47Z

@zmwangx interesting.

ilovezfs · 2016-09-24T15:42:37Z

We regularly see issues with brew missing output from brew doctor.

doctor complaining about that is not the same as it causing a user reported issue :)

MikeMcQuaid · 2016-09-24T15:44:26Z

I've seen enough of them. I'm also fed up of people complaining in person and on the internets about this being another example of our dependency system being shitty.

MikeMcQuaid · 2016-09-24T15:44:43Z

And if in doubt: we should do what other package managers do unless there's a really good reason.

ilovezfs · 2016-09-24T15:45:39Z

And if in doubt: we should do what other package managers do unless there's a really good reason.

Be careful what you wish for :)

ilovezfs · 2016-09-24T15:51:12Z

Yup, reproduced what @zmwangx described.

alyssais · 2016-10-25T21:45:11Z

Additionally, I think all the new tests in this file should be made into non-integration tests instead; the integration tests are meant just as an overall smoke test and are too slow to be used for more complex cases like this.

Agree in principle. I might leave one or two for now just to make sure that the options are understood correctly, and move the rest somewhere else. (Although, it sure would be nice if they were just faster 🤔.)

This was moved to Keg, but looks like I forgot to get rid of it here.

MikeMcQuaid · 2016-10-26T13:50:37Z

Agree in principle. I might leave one or two for now just to make sure that the options are understood correctly, and move the rest somewhere else. (Although, it sure would be nice if they were just faster 🤔.)

I think it's probably still worth just removing them. Agreed about them being faster but integration tests are always going to be significantly slower so I think we really only want one per command.

alyssais · 2016-10-26T14:05:39Z

Okay, I'll remove the two extra integration tests. (I'm just desperate to get this merged, heh.)

However, I'm still very wary of leaving Homebrew with no tests ensuring that it doesn't inadvertently block uninstalling things that shouldn't be blocked.

MikeMcQuaid · 2016-10-26T14:21:04Z

However, I'm still very wary of leaving Homebrew with no tests ensuring that it doesn't inadvertently block uninstalling things that shouldn't be blocked.

I appreciate the concern ❤️ but we'll do some manual testing and you've added some new unit tests which should help here.

MikeMcQuaid · 2016-10-26T14:21:47Z

Something else that could be useful is writing out manual tests cases you've done so I can evaluate and rerun them.

alyssais · 2016-10-30T12:56:35Z

I totally missed these comments. Oops!

This should be a pretty complete test:

# These probably should all be run with HOMEBREW_BUILD_FROM_SOURCE set.
# There should be no formulae currently installed that depend on ant or rlwrap.

# Run the follow tests as a developer

brew reinstall abcl
brew uninstall ant # should succeed

# Run the following tests as a non-developer

brew reinstall abcl
brew uninstall ant # should fail
brew uninstall --force ant # should fail
brew uninstall --ignore-dependencies ant # should work

brew reinstall abcl
brew uninstall abcl # should work

brew reinstall abcl
brew uninstall ant abcl # should work

brew reinstall
brew uninstall abcl ant # should work

brew reinstall abcl
brew uninstall rlwrap # should work

# To test the fallback for formulae installed before runtime
# dependencies were recorded, delete the runtime_dependencies
# array from the abcl INSTALL_RECEIPT after every `reinstall`.

bfontaine · 2016-10-30T13:12:45Z

Library/Homebrew/cmd/missing.rb

 #:    Check the given <formulae> for missing dependencies. If no <formulae> are
 #:    given, check all installed brews.
+#:
+#:    If `--hide=`<hidden> is passed, act as if none of <hidden> are installed.
+#:    <hidden> should be a comma-seperated list of formulae.


Typo: seperated ➡️ separated

alyssais · 2016-11-01T20:58:36Z

Anything else I should be doing that I've missed?

MikeMcQuaid · 2016-11-01T21:03:29Z

Sorry, on vacation so will be a bit before I can test/merge this.

alyssais · 2016-11-01T21:04:53Z

No problem, just making sure. :)

MikeMcQuaid · 2016-11-10T21:30:19Z

Great work @alyssais! My testing is all good and I'll merge this tomorrow.

MikeMcQuaid · 2016-11-10T21:32:06Z

Library/Homebrew/cmd/uninstall.rb

+    msg << " required by #{dependents.join(", ")}, which "
+    msg << (dependents.count == 1 ? "is" : "are")
+    msg << " currently installed."
+    ofail msg


It can be in another PR but I reckon a non-fatal version of this message should be printed in the developer case as a "Warning" rather than "Error".

Good catch. I'll do a separate PR.

@alyssais Was thinking of releasing a new tag with this in it on Mondayish? Any chance you'll be able to do this over the weekend? If not, no worries and I'll jump in on this before then. Thanks again!

I'm mostly done already! Just need to fix a test failure.

Great, looking forward to it!

@MikeMcQuaid in doing this, I think I've identified an (probably very rare) edge case where uninstalling can now result in a crash. Shouldn't take much to fix, but you may want to avoid releasing this until has been. As soon as I've nailed down exactly how to reproduce (should be imminent) I'll open an issue explaining it.

MikeMcQuaid · 2016-11-11T08:24:04Z

🎉 Great work @alyssais!

alyssais changed the title ~~uninstall: warn when dependants still installed~~ uninstall: refuse when dependants still installed Sep 22, 2016

alyssais force-pushed the uninstall_dependancy_error branch from cdb1e6f to 01422c3 Compare September 22, 2016 12:32

MikeMcQuaid reviewed Sep 23, 2016

View reviewed changes

ilovezfs changed the title ~~uninstall: refuse when dependants still installed~~ uninstall: refuse when dependents still installed Sep 24, 2016

missing: document --hide

aff5f42

alyssais added 4 commits October 25, 2016 23:48

uninstall: remove duplicated method

d0ad097

This was moved to Keg, but looks like I forgot to get rid of it here.

uninstall, keg: update style

5a3d6c4

uninstall: test should_check_for_dependents?

3702e56

uninstall: go easier on the integration tests

bb30b01

alyssais force-pushed the uninstall_dependancy_error branch from 7eaa7bb to bb30b01 Compare October 25, 2016 22:56

alyssais added 2 commits October 25, 2016 23:59

uninstall: document --ignore-dependencies

481a097

test_uninstall: style fixes

b71ce88

uninstall: remove more integration tests

ec83d1d

bfontaine reviewed Oct 30, 2016

View reviewed changes

alyssais added 2 commits October 30, 2016 16:30

docs: fix typo for brew missing

702345e

Update man

f352638

MikeMcQuaid approved these changes Nov 10, 2016

View reviewed changes

MikeMcQuaid reviewed Nov 10, 2016

View reviewed changes

MikeMcQuaid merged commit 2ce17a1 into Homebrew:master Nov 11, 2016

alyssais deleted the uninstall_dependancy_error branch November 11, 2016 12:59

This was referenced Nov 13, 2016

brew uninstall can't uninstall formulae that don't exist in any tap #1496

Closed

Warn developers when uninstalling a dependency #1498

Merged

Homebrew locked and limited conversation to collaborators May 3, 2018

uninstall: refuse when dependents still installed #1082

uninstall: refuse when dependents still installed #1082

Conversation

alyssais commented Sep 22, 2016 • edited Loading

alyssais commented Sep 22, 2016

vladshablinsky commented Sep 22, 2016

Choose a reason for hiding this comment

MikeMcQuaid commented Sep 23, 2016

alyssais commented Sep 23, 2016

MikeMcQuaid commented Sep 23, 2016

MikeMcQuaid commented Sep 23, 2016

ilovezfs commented Sep 23, 2016

alyssais commented Sep 23, 2016

ilovezfs commented Sep 23, 2016

DomT4 commented Sep 23, 2016

MikeMcQuaid commented Sep 24, 2016

ilovezfs commented Sep 24, 2016 • edited Loading

MikeMcQuaid commented Sep 24, 2016

MikeMcQuaid commented Sep 24, 2016

ilovezfs commented Sep 24, 2016

MikeMcQuaid commented Sep 24, 2016

ilovezfs commented Sep 24, 2016

ilovezfs commented Sep 24, 2016

MikeMcQuaid commented Sep 24, 2016

MikeMcQuaid commented Sep 24, 2016

ilovezfs commented Sep 24, 2016

ilovezfs commented Sep 24, 2016

MikeMcQuaid commented Sep 24, 2016

MikeMcQuaid commented Sep 24, 2016

ilovezfs commented Sep 24, 2016

ilovezfs commented Sep 24, 2016

alyssais commented Oct 25, 2016

MikeMcQuaid commented Oct 26, 2016

alyssais commented Oct 26, 2016

MikeMcQuaid commented Oct 26, 2016

MikeMcQuaid commented Oct 26, 2016

alyssais commented Oct 30, 2016 • edited Loading

Choose a reason for hiding this comment

alyssais commented Nov 1, 2016

MikeMcQuaid commented Nov 1, 2016

alyssais commented Nov 1, 2016

MikeMcQuaid commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Nov 11, 2016

alyssais commented Sep 22, 2016 •

edited

Loading

ilovezfs commented Sep 24, 2016 •

edited

Loading

alyssais commented Oct 30, 2016 •

edited

Loading