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

uninstall: refuse when dependents still installed #1082

Merged
merged 32 commits into from Nov 11, 2016

Conversation

Projects
None yet
8 participants
@alyssais
Copy link
Contributor

alyssais commented Sep 22, 2016

  • 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 alyssais changed the title uninstall: warn when dependants still installed uninstall: refuse when dependants still installed Sep 22, 2016

@alyssais alyssais force-pushed the alyssais:uninstall_dependancy_error branch from cdb1e6f to 01422c3 Sep 22, 2016

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Sep 22, 2016

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

This comment has been minimized.

Copy link
Contributor

vladshablinsky commented Sep 22, 2016

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

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}`."

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 23, 2016

Member

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 23, 2016

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

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Sep 23, 2016

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 23, 2016

@penman 👏

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 23, 2016

@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

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 23, 2016

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

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Sep 23, 2016

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

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 23, 2016

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

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 23, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 24, 2016

--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.

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 24, 2016

@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

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 24, 2016

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 24, 2016

@zmwangx interesting.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 24, 2016

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

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 24, 2016

Yup, reproduced what @zmwangx described.

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Oct 25, 2016

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 🤔.)

@alyssais alyssais force-pushed the alyssais:uninstall_dependancy_error branch from 7eaa7bb to bb30b01 Oct 25, 2016

alyssais added some commits Oct 25, 2016

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 26, 2016

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

This comment has been minimized.

Copy link
Contributor

alyssais commented Oct 26, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 26, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 26, 2016

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

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Oct 30, 2016

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`.
#: 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.

This comment has been minimized.

@bfontaine

bfontaine Oct 30, 2016

Member

Typo: seperated ➡️ separated

alyssais added some commits Oct 30, 2016

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Nov 1, 2016

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Nov 1, 2016

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

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Nov 1, 2016

No problem, just making sure. :)

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Nov 10, 2016

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

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Nov 10, 2016

Member

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".

This comment has been minimized.

@alyssais

alyssais Nov 10, 2016

Contributor

Good catch. I'll do a separate PR.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Nov 12, 2016

Member

@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!

This comment has been minimized.

@alyssais

alyssais Nov 12, 2016

Contributor

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Nov 12, 2016

Member

Great, looking forward to it!

This comment has been minimized.

@alyssais

alyssais Nov 13, 2016

Contributor

@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 MikeMcQuaid merged commit 2ce17a1 into Homebrew:master Nov 11, 2016

4 checks passed

codecov/patch 62.92% of diff hit (target 60.82%)
Details
codecov/project Absolute coverage decreased by -0.04% but relative coverage increased by +2.09% compared to 84d1661
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Nov 11, 2016

🎉 Great work @alyssais!

@alyssais alyssais deleted the alyssais:uninstall_dependancy_error branch Nov 11, 2016

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.