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

Undeclared dependencies in runtime_dependencies #3789

Merged
merged 15 commits into from Apr 25, 2018

Conversation

Projects
None yet
4 participants
@alyssais
Copy link
Contributor

alyssais commented Feb 11, 2018

  • 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 style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Fixes #2173.

I've just noticed #3785. That probably interplays with this change in some way, but I haven't had a chance to have a proper look at it yet.

@alyssais alyssais requested review from MikeMcQuaid and ilovezfs Feb 11, 2018


keg = Keg.new(opt_prefix)
linkage_checker = LinkageChecker.new(keg, self)
dylib_formula_names = linkage_checker.brewed_dylibs.keys

This comment has been minimized.

@ilovezfs

ilovezfs Feb 11, 2018

Contributor

This includes the declared ones.

This comment has been minimized.

@alyssais

alyssais Feb 11, 2018

Contributor

Ah, so it does. This didn't matter when I had this all in one method because we took the union, but it does now that it's in a method called undeclared_runtime_dependencies.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Feb 11, 2018

This PR does not save the undeclared ones to the INSTALL_RECEIPT.json and so unsafe removal of opportunistically linked deps is still allowed.

Add undeclared dependencies to Tab when installing
An installed formula doesn't get optlinked until _after_ it's installed,
meaning that we can't rely on `opt_prefix` to get the right keg.

So, if not optlinked, fall back to the formula's prefix, which will be
that of the current installation.
@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Feb 11, 2018

This PR does not save the undeclared ones to the INSTALL_RECEIPT.json and so unsafe removal of opportunistically linked deps is still allowed.

Ah, I'd assumed Tab.create would pick them up, but it doesn't because the formula doesn't get linked into opt until after the tab is created. We can work around that by falling back to the formula's prefix if it isn't optlinked.

@@ -13,6 +13,7 @@
require "tap"
require "keg"
require "migrator"
require "os/mac/linkage_checker"

This comment has been minimized.

@alyssais

alyssais Feb 11, 2018

Contributor

Will fix this to properly use the OS layer on the next pass.

@alyssais alyssais force-pushed the alyssais:undeclared_runtime_dependencies branch from ca1c389 to b5be5c7 Feb 15, 2018

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Feb 15, 2018

how is this not also a problem for Linux?

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Feb 15, 2018

It is a problem on Linux, but we don't have a LinkageChecker there

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Feb 15, 2018

brew linkage also doesn't work on Linux

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Feb 15, 2018

Oh joy

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Feb 15, 2018

I think there was a WIP implementation for Linuxbrew at some point, but not sure what happened to it.

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Feb 18, 2018

brew linkage works on Linuxbrew.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 18, 2018

@sjackman any chance of getting it upstreamed in the near future for this PR?

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Feb 18, 2018

Done! I'm that good. (It works already too.) =p

$ brew linkage testbottest
System libraries:
  /lib/x86_64-linux-gnu/libc.so.6
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 26, 2018

@alyssais What's the latest here?

linkage_checker = LinkageChecker.new(keg, self)
linkage_checker.undeclared_deps.map { |n| Dependency.new(n) }
end
end

This comment has been minimized.

@sjackman

sjackman Feb 26, 2018

Contributor

Why is undeclared_runtime_dependencies undefined for Linux?

This comment has been minimized.

@alyssais

alyssais Feb 28, 2018

Contributor

Because I didn't realise we had working linkage on Linux :P

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Feb 28, 2018

Oh, I see. os/mac/linkage_checker is used on Linux even though it's in os/mac? I'll update the PR accordingly.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Feb 28, 2018

@alyssais Also, have you determined what impact this has on brew deps? Note that #3785 was merged.

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Feb 28, 2018

Oh, I see. os/mac/linkage_checker is used on Linux even though it's in os/mac? I'll update the PR accordingly.

Hmm, how sketchy. I'll submit a PR to move linkage_checker.rb to generic.

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Feb 28, 2018

PR #3852 moves linkage_checker.rb to generic. Thanks for pointing out this peculiarity, Alyssa!

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Mar 1, 2018

Also, have you determined what impact this has on brew deps?

AFAICT brew deps doesn't call runtime_dependencies, so it should have no impact.

Note that #3785 was merged.

#3785 will stop indirect dependencies suddenly showing up as runtime dependencies, which would have happened otherwise, I think.

alyssais added some commits Feb 15, 2018

Only check undeclared dependencies on macOS
We don't currently have a LinkageChecker on Linux, so can't do this.

@alyssais alyssais force-pushed the alyssais:undeclared_runtime_dependencies branch from b5be5c7 to 46c85ae Mar 1, 2018

@alyssais alyssais force-pushed the alyssais:undeclared_runtime_dependencies branch from 46c85ae to af682d2 Mar 1, 2018

alyssais added some commits Mar 12, 2018

@@ -1487,12 +1488,29 @@ def recursive_requirements(&block)
# Returns a list of Dependency objects that are required at runtime.
# @private
def runtime_dependencies

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 12, 2018

Member

I was thinking: given this is going to change the output of runtime_dependencies again quite drastically: I think it would be worth having the runtime_dependencies in the Tab be versioned. This means we don't need to do something like parsed_homebrew_version < "1.1.6" (in tab.rb) again in order to make sure these tabs have what we expect/need to rely on them.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 25, 2018

Member

I've changed my mind on this given #3979 so: ignore it.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Mar 25, 2018

This PR will be affected by #3979 (which I think should probably go in first).

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Mar 30, 2018

@alyssais What's the latest here? Thanks!


def undeclared_runtime_dependencies
if optlinked?
keg = Keg.new(opt_prefix)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 30, 2018

Member

What's the advantage of using the opt_prefix when it's optlinked??

This comment has been minimized.

@alyssais

alyssais Apr 9, 2018

Contributor

Uses the currently active version of a formula rather than the latest. Can happen with brew switch or even just if brew upgrade hasn't been run yet.

@@ -63,7 +63,7 @@ def check_undeclared_deps
formula.build.without?(dep)
end
declared_deps = formula.deps.reject { |dep| filter_out.call(dep) }.map(&:name)
runtime_deps = keg.to_formula.runtime_dependencies(read_from_tab: false)
runtime_deps = keg.to_formula.declared_runtime_dependencies

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 30, 2018

Member

This method doesn't seem to be defined.

This comment has been minimized.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 8, 2018

Member

Fixed now.

alyssais added some commits Apr 7, 2018

Restore Formula#declared_runtime_dependencies
This got lost in a merge.
@@ -63,7 +63,7 @@ def check_undeclared_deps
formula.build.without?(dep)
end
declared_deps = formula.deps.reject { |dep| filter_out.call(dep) }.map(&:name)
runtime_deps = keg.to_formula.runtime_dependencies(read_from_tab: false)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 8, 2018

Member

Maybe the read_from_tab argument can go away entirely? The only other caller would be in tab.rb

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 25, 2018

Member

Changed my mind on this: ignore!

compute_runtime_dependencies
end

def compute_runtime_dependencies

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 9, 2018

Member

Sorry to be a pain but I think this is probably worse than the read_from_tab argument was 😭. It's not clear when you need to call runtime_dependencies vs. compute_runtime_dependencies. Could maybe fiddle with naming options but I'm tempted to just say put it back to how it was. Sorry.

@MikeMcQuaid MikeMcQuaid force-pushed the alyssais:undeclared_runtime_dependencies branch from 46dee2b to c167614 Apr 25, 2018

MikeMcQuaid added some commits Apr 25, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 25, 2018

@ilovezfs Mind kicking this around a bit and seeing if it does what you expect/want before I merge? Thanks ❤️

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 25, 2018

It seems to work.

iMac-TMP:freeciv joe$ diff before after
1c1
< {"homebrew_version":"1.6.2-14-g29f7d3d"
---
> {"homebrew_version":"1.6.2-29-g8035afc"
12c12
< "time":1524657205
---
> "time":1524657606
14c14
< "HEAD":"29f7d3d7ef5b2b939021ac247ef042228b8cc8a2"
---
> "HEAD":"8035afcc36ac45ebd127f670b13ab32414c310fa"
73c73,75
< "version":"2.24.32"}]
---
> "version":"2.24.32"}
> {"full_name":"xz"
> "version":"5.2.3"}]

for freeciv opportunistically linked to xz.

Do you want to bump any sort of INSTALL_RECEIPT version here since it changes the meaning/reliability of that data?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 25, 2018

It seems to work.

🎉

Do you want to bump any sort of INSTALL_RECEIPT version here since it changes the meaning/reliability of that data?

I reckon we can leave it as-is because this is "value add" rather than "broken without".

@MikeMcQuaid MikeMcQuaid merged commit 0fd2db3 into Homebrew:master Apr 25, 2018

3 checks passed

codecov/patch 87.5% of diff hit (target 70.05%)
Details
codecov/project Absolute coverage decreased by -7.37% but relative coverage increased by +17.44% compared to 29f7d3d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 25, 2018

Thanks @alyssais!

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 25, 2018

Thanks @alyssais and @MikeMcQuaid!

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 27, 2018

This PR broke upgrades when there are formulae with the same name in two different taps even if the formula dependency is fully qualified. 8386e6a is the breaking commit.

iMac-TMP:Homebrew joe$ git rev-parse @
27ec9dae5fa4c2c5b19f8bfafaefbdbb3e9a9c8c
iMac-TMP:Homebrew joe$ brew upgrade
==> Upgrading 2 outdated packages, with result:
pothosware/pothos/pothoscomms 0.2.2 -> 0.3.1, pothosware/pothos/pothosplotters 0.3.1 -> 0.4.1
Error: Formulae found in multiple taps: 
       * dholm/sdr/spuce
       * pothosware/pothos/spuce

Please use the fully-qualified name e.g. dholm/sdr/spuce to refer the formula.
iMac-TMP:Homebrew joe$ 

with debug

==> Upgrading 2 outdated packages, with result:
pothosware/pothos/pothoscomms 0.2.2 -> 0.3.1, pothosware/pothos/pothosplotters 0.3.1 -> 0.4.1
Error: Formulae found in multiple taps: 
       * dholm/sdr/spuce
       * pothosware/pothos/spuce

Please use the fully-qualified name e.g. dholm/sdr/spuce to refer the formula.
/usr/local/Homebrew/Library/Homebrew/formulary.rb:377:in `loader_for'
/usr/local/Homebrew/Library/Homebrew/formulary.rb:280:in `factory'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1444:in `[]'
/usr/local/Homebrew/Library/Homebrew/linkage_checker.rb:141:in `block in check_undeclared_deps'
/usr/local/Homebrew/Library/Homebrew/linkage_checker.rb:139:in `reject'
/usr/local/Homebrew/Library/Homebrew/linkage_checker.rb:139:in `check_undeclared_deps'
/usr/local/Homebrew/Library/Homebrew/linkage_checker.rb:104:in `check_dylibs'
/usr/local/Homebrew/Library/Homebrew/linkage_checker.rb:19:in `initialize'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1530:in `new'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1530:in `undeclared_runtime_dependencies'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1511:in `runtime_dependencies'
/usr/local/Homebrew/Library/Homebrew/formula_installer.rb:424:in `runtime_requirements'
/usr/local/Homebrew/Library/Homebrew/formula_installer.rb:437:in `expand_requirements'
/usr/local/Homebrew/Library/Homebrew/formula_installer.rb:381:in `compute_dependencies'
/usr/local/Homebrew/Library/Homebrew/formula_installer.rb:136:in `verify_deps_exist'
/usr/local/Homebrew/Library/Homebrew/formula_installer.rb:129:in `prelude'
/usr/local/Homebrew/Library/Homebrew/cmd/upgrade.rb:139:in `upgrade_formula'
/usr/local/Homebrew/Library/Homebrew/cmd/upgrade.rb:95:in `block in upgrade'
/usr/local/Homebrew/Library/Homebrew/cmd/upgrade.rb:92:in `each'
/usr/local/Homebrew/Library/Homebrew/cmd/upgrade.rb:92:in `upgrade'
/usr/local/Homebrew/Library/Homebrew/brew.rb:101:in `<main>'

MikeMcQuaid added a commit to MikeMcQuaid/brew that referenced this pull request Apr 28, 2018

linkage_checker: correclty handle multiple taps.
Fix upgrades of formulae with the same name in multiple taps.

As reported in:
Homebrew#3789 (comment)

MikeMcQuaid added a commit to MikeMcQuaid/brew that referenced this pull request Apr 28, 2018

linkage_checker: correctly handle multiple taps.
Fix upgrades of formulae with the same name in multiple taps.

As reported in:
Homebrew#3789 (comment)

@lock lock bot added the outdated label May 27, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 27, 2018

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