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

tab: store installed_as_dependency, installed_on_request. #1813

Merged
merged 5 commits into from Jan 18, 2017

Conversation

Projects
None yet
3 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Jan 10, 2017

This is used to determine whether or not a formula’s install was specifically requested by a user or pulled in as a dependency.

Also, use this and set it in various places (including a separate analytics category only for things explicitly requested to be installed).

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:installed_as_dependency branch from 24f512d to bab1589 Jan 11, 2017

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Jan 11, 2017

This is nice — I'd been thinking about something like this.

Would it be worth remembering what it was installed as a dependency for?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 11, 2017

Would it be worth remembering what it was installed as a dependency for?

@alyssais Yes, perhaps! I was thinking the implementation of this would be significantly more involved, though so punted to YAGNI for now.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:installed_as_dependency branch from bab1589 to 1d7b38b Jan 11, 2017

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jan 16, 2017

If you subsequently interact with the formula directly via upgrade or reinstall shouldn't it be promoted to true leaf status?

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jan 16, 2017

Isn't this going to result in false information when brew bundle is used?

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jan 16, 2017

If we were to also record installed-as-leaf it might help to detect bugs.

Also, I think this may result in some pretty bogus data, especially if it's not able to be toggled by the user. We have no way of specifying to which formula a given option pertains unless we install them in separate invocations of brew install but only one of them may be intended as an actual leaf install.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 16, 2017

If you subsequently interact with the formula directly via upgrade or reinstall shouldn't it be promoted to true leaf status?

It will on reinstall, it won't on upgrade. I don't think a manual upgrade should imply that.

Isn't this going to result in false information when brew bundle is used?

Temporarily, yes. This functionality is being added at least in part to mean that brew bundle can dump only the leaves so it will be a time-limited thing.

If we were to also record installed-as-leaf it might help to detect bugs.

I'd rather not store duplicate data that's inferred from the inverse of the existing binary.

Also, I think this may result in some pretty bogus data, especially if it's not able to be toggled by the user.

Toggling by the user will be done at a later stage. Storing the data is the first step along this road.

We have no way of specifying to which formula a given option pertains unless we install them in separate invocations of brew install but only one of them may be intended as an actual leaf install.

I'm not sure I understand this, can you elaborate?

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jan 16, 2017

I'd rather not store duplicate data that's inferred from the inverse of the existing binary.

That means we can't tell the difference between something that was originally a dependency and then got promoted and something that was always a leaf. It's not actually duplicate data or able to be inferred unless you make it non-Boolean.

I'm not sure I understand this, can you elaborate?

x depends on y.

brew install --without-test x

vs.

brew install y --without-test
brew install x

y may not be a leaf at all. I may just want to avoid the test suite in y. There is no way to do that currently without first doing the dependency, and you're falsely inferring I want y as a leaf, which it may not be at all (e.g., openssl has a test suite that has a long run time, but that doesn't actually make it a leaf in any way).

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 16, 2017

Just as it's worth noting: this is broadly the approach apt takes and I think following this prior art is valuable. That said, if there's an approach you suggest taking instead I'm all 👂s.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jan 16, 2017

if there's an approach you suggest taking instead I'm all 👂s.

Non-Boolean or two Boolean values would provide more complete information.

Regarding the problem of specifying options for non-leaves without being forced to promote them to leaves, I recently learned cabal uses : for this: Homebrew/homebrew-core#8397 (comment)

The example from the comment is

cabal install --allow-newer=turtle:directory --constraint 'directory < 1.4' purescript
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 16, 2017

Non-Boolean or two Boolean values would provide more complete information.

It would provide more complete information but I'm struggling to see how that information would be actionable (i.e. how you'd not end up just consistently discarding one value).

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jan 16, 2017

I'd rather have the information than not have it. It seems worthwhile to know that something was always a leaf vs. something was promoted to being a leaf. For example, I may want to "uninstall all formulae that were originally dependencies regardless of whether they were subsequently promoted so long as they are no longer required by anything installed." You're throwing out the data we'd need for that. (For example, brew autoremove --ignore-promotions)

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 16, 2017

@ilovezfs Thanks, good explanation 👍

@MikeMcQuaid MikeMcQuaid changed the title tab: store installed_as_dependency. tab: store installed_as_dependency, installed_on_request. Jan 17, 2017

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:installed_as_dependency branch from 1d7b38b to 89c7a7c Jan 17, 2017

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 17, 2017

Updated to be two variables. Can you double-check my logic here @alyssais and @ilovezfs to make sure it makes sense? Thanks!

next unless f.any_version_installed?
keg = Keg.new(f.prefix)
tab = Tab.for_keg(keg)
tab.installed_as_dependency = false

This comment has been minimized.

@ilovezfs

ilovezfs Jan 17, 2017

Contributor

probably should leave any preexisting value of installed_as_dependency intact

@@ -250,6 +253,12 @@ def install
category = "install"
action = ([formula.full_name] + options).join(" ")
Utils::Analytics.report_event(category, action)

unless installed_on_request

This comment has been minimized.

@ilovezfs

ilovezfs Jan 17, 2017

Contributor

not if?

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:installed_as_dependency branch from 89c7a7c to e1c6bea Jan 17, 2017

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 17, 2017

@ilovezfs Thanks, updated again.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:installed_as_dependency branch 2 times, most recently from 8afc243 to 3db68b5 Jan 18, 2017

MikeMcQuaid added some commits Jan 9, 2017

tab: store installed_{as_dependency,on_request}.
These are used to determine whether or not a formula’s install was
specifically requested by a user and/or pulled in as a dependency.
formula: installed_{as_dependency,on_request} hash
Which, in turn, provides them for `brew info --json=v1` so other tools
such as e.g. `brew bundle` can make use of this information.
formula_installer: use installed_* variables.
Also, report formulae installed on request. This is useful in
differentiating between those formulae that are popular because they
are widely requested and those that are popular because they are widely
depended on.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:installed_as_dependency branch from 3db68b5 to 89d8864 Jan 18, 2017

@MikeMcQuaid MikeMcQuaid merged commit aa926a1 into Homebrew:master Jan 18, 2017

3 checks passed

codecov/patch 71.42% of diff hit (target 63.11%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +8.31% compared to 22e8ddc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:installed_as_dependency branch Jan 18, 2017

@alyssais alyssais referenced this pull request Jan 19, 2017

Closed

`brew orphaned` #1881

5 of 5 tasks complete

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