linkage_checker: add a check for extraneous dependencies #3103

maxim-belkin · 2017-08-29T16:16:39Z

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?

Detect extraneous dependencies in formulae. Might be useful for new formulae.

Have not written tests for my changes. Suggestions what tests to write are welcome
brew tests fail for some other tests

MikeMcQuaid · 2017-08-29T16:50:57Z

I'm not sure I'm reading this right but is an extraneous dependency any where there's no linkage? If so, this will need to be an optional flag as there will be a lot of false positives otherwise (i.e. anything that calls an executable from a dependency rather than linking against it, most scripting language dependencies, etc.)

sjackman · 2017-08-29T16:53:43Z

A lot of false positives could be eliminated by ignoring any keg that has a bin directory. Many packages whose primary purpose is to provide a library have include and lib directories, but no bin directory.

maxim-belkin · 2017-08-29T17:43:30Z

I agree: false positives are a possibility.

brew linkage ffmpeg

reports xvid as a possible extraneous dependency

maxim-belkin · 2017-08-29T18:28:15Z

Those interested in testing this, try the following "one-liner" to check your installed formulae:

for formula in $(brew list); do echo "* Formula: $formula"; brew linkage $formula | sed -n '/^Possible/,/^[^ ]/p;'; done

sjackman · 2017-08-29T18:41:11Z

@ilovezfs identified that roughly half of depends_on "boost" dependencies in Homebrew/science use only the headers, and have no run-time linkage to boost, and so should in fact be depends_on "boost" => :build. This PR would help identify those cases. I'm having trouble finding the archived conversation—too many repos and slack channels. I agree entirely that the false positives have to be eliminated for this PR to be useful.

scpeters · 2017-08-29T18:59:47Z

@sjackman there could still be a need for boost to be installed for libraries that expose boost symbols in their own headers. Then anything that wants to build against the library would need the boost headers. It's kinda like the difference between lib* and lib*-dev packages on debian.

scpeters · 2017-08-29T19:39:34Z

I just took a quick look at some of the packages in homebrew/science and found two that use the boost headers and install a library and header files (cantera and xylib). Despite having #include <boost/ statements in the installed header files, both of these formulae declare :build dependencies on boost, possibly because they also install executables and want to reduce the footprint for those users. If you're building from source against a library, they might be knowledgeable to install boost on their own.

Maybe these types of header-only dependencies that the linkage test can't see could be marked with :run like the build dependencies in audit?

https://github.com/Homebrew/brew/blob/master/Library/Homebrew/dev-cmd/audit.rb#L499-L507

MikeMcQuaid · 2017-08-29T20:41:05Z

I agree: false positives are a possibility.

Given we're running this in CI for everything in Homebrew/homebrew-core: we really don't want false positives here.

@ilovezfs identified that roughly half of depends_on "boost" dependencies in Homebrew/science use only the headers, and have no run-time linkage to boost, and so should in fact be depends_on "boost" => :build. This PR would help identify those cases.

I agree. Having this as an optional flag which is always run for new formulae in brew test-botseems like a good idea.

maxim-belkin · 2017-08-29T22:04:49Z

Please suggest test-cases or directions how to improve the code

MikeMcQuaid · 2017-08-30T09:47:36Z

@maxim-belkin Use ARGV.include?("--something") and only enable the current functionality if that is passed.

sjackman · 2017-08-30T14:47:05Z

@MikeMcQuaid The output of Possible undeclared dependencies is informative only. It doesn't affect the exit status of either brew test or brew test --missing. For example brew linkage awk reports Possible undeclared dependencies: gmp. Would that behaviour be an option for this feature as well? Enabled by default, print an informative message, but not affecting exit status. To make this feature useful, it's still important to eliminate as many false positives as possible.

MikeMcQuaid · 2017-08-30T17:03:22Z

It doesn't affect the exit status of either brew test or brew test --missing.

It doesn't affect the exit status or output of https://github.com/Homebrew/homebrew-test-bot/blob/80333911e7130b9542a6fe2b94c1311b52cef319/cmd/brew-test-bot.rb#L664 ? That's what we need to avoid.

sjackman · 2017-08-30T17:15:39Z

Sorry, I wrote too quickly and didn't engage brain. I had meant to say…
Possible undeclared dependencies doesn't affect the output status of brew linkage or brew linkage --test.
If the same were true for this PR Possible unnecessary dependencies would it be okay without an option to enable it?

ilovezfs · 2017-08-30T17:23:40Z

I agree. Having this as an optional flag which is always run for new formulae in brew test-bot seems like a good idea.

Things that are known to give a lot of false positives aren't great even for --new-formula. We keep getting contributors asking, "Will my formula still qualify if those audits are failing?" and it's getting pretty old explaining that yes CI is bright red but no that doesn't matter because it will go away next time after it's merged.

sjackman · 2017-08-30T17:36:35Z

This test will not affect the exit status of brew linkage --test, brew audit --new-formula, nor of CI. It'll only display an informational message, similar to Possible undeclared dependencies.

MikeMcQuaid · 2017-08-30T17:38:49Z

@sjackman in that case: how does anyone notice that output and make changes as a result?

scpeters · 2017-08-30T17:59:54Z

I'm guessing it prints console messages but doesn't affect the exit status, which is what CI cares about.

sjackman · 2017-08-30T20:38:33Z

Same as with the existing Possible undeclared dependencies message. It's only an informational message that does not affect CI status. If you ran brew linkage manually you'd see the message, or if you looked in the CI log, you'd see the message.

maxim-belkin · 2017-08-30T20:43:26Z

Just an FYI: I've prepared a version with --extraneous so... just let me guys know what you decide. Personally, I think it is similar to undeclared dependencies, so I'd enable it by default.
As I mentioned in the OP, this could be used for new formulae: people could be asked to provide output of brew linkage when submitting a PR.

sjackman · 2017-08-30T21:05:31Z

@sjackman in that case: how does anyone notice that output and make changes as a result?

Ah, I had forgotten that test-bot prints only the output of brew linkage --test and not brew linkage. In that case this PR does not affect CI at all. I would suggest modifying test-bot to run brew linkage, which always succeeds, in addition to brew linkage --test, so that the output of Possible undeclared dependencies and the proposed Possible unnecessary dependencies are in the build log.

+      test "brew", "linkage", dependent.name
       test "brew", "linkage", "--test", dependent.name

https://github.com/Homebrew/homebrew-test-bot/blob/master/cmd/brew-test-bot.rb#L664

sjackman · 2017-08-30T21:06:11Z

Or brew linkage --test could be modified to keep its existing exit status behaviour, but print the more verbose message of brew linkage.

maxim-belkin · 2017-08-30T21:12:43Z

Or brew linkage --test could be modified to keep its existing exit status behaviour, but print the more verbose message of brew linkage.

that's a possibility.

sjackman · 2017-08-30T21:59:02Z

The current output of brew linkage and brew linkage --test is…

❯❯❯ brew linkage gawk
System libraries:
  /usr/lib/libSystem.B.dylib
Homebrew libraries:
  /Users/sjackman/.homebrew/opt/gmp/lib/libgmp.10.dylib (gmp)
  /Users/sjackman/.homebrew/opt/mpfr/lib/libmpfr.4.dylib (mpfr)
  /Users/sjackman/.homebrew/opt/readline/lib/libreadline.7.dylib (readline)
Possible undeclared dependencies:
  gmp
❯❯❯ brew linkage --test gawk
No broken dylib links

MikeMcQuaid · 2017-08-31T18:20:32Z

Or brew linkage --test could be modified to keep its existing exit status behaviour, but print the more verbose message of brew linkage.

This seems sensible. That said: how are people going to notice this for formulae in CI or is this intended to just be manually run by maintainers? I'm trying to understand the use case.

sjackman · 2017-08-31T22:46:37Z

Of 47 formulae that depend on boost in Homebrew/science, @ilovezfs identified that 20 of those 47 formulae have no library linkage to boost (that is, they use only the headers), so should in fact be depends_on "boost" => :build rather than depends_on "boost". This PR would allow a developer (maintainer or contributor) to run brew linkage foo, see the message Possible unnecessary dependency: boost, and change the dependency to a :build dependency. I would use this command when creating a new formula, or troubleshooting existing formulae.
https://github.com/Homebrew/homebrew-science/issues/6192#issuecomment-324509708

maxim-belkin · 2017-09-01T12:49:53Z

extraneous or unnecessary?
what should I do?
1. leave the PR "as is" 😊
2. move extraneous/unnecessary functionality under the --test
  1. move undeclared dependencies under --test

MikeMcQuaid · 2017-09-01T13:26:13Z

This PR would allow a developer (maintainer or contributor) to run brew linkage foo, see the message Possible unnecessary dependency: boost, and change the dependency to a :build dependency.

That seems like a good workflow but I guess I'm just wondering if this is reliant on individual developers to manually do this: is it going to be useful? I see no harm in this PR as-is being merged, to be clear, I'm just trying to figure out if there's some better workflow.

MikeMcQuaid · 2017-09-01T13:30:00Z

extraneous or unnecessary?

unnecessary

what should I do?

As long as the exit code isn't modified I'm happy as-is. It does seem like it would be useful to have this under --test so test-bot runs it.

sjackman · 2017-09-02T00:05:55Z

@maxim-belkin I'd suggest Possible build or unnecessary dependencies

sjackman · 2017-09-02T00:09:44Z

I'm just trying to figure out if there's some better workflow.

If it were possible to eliminate all the false positives, then it could affect the brew linkage --test exit status. It may be possible, but it would require significant work, and may in fact be impossible to get 100% perfect. If it got to the point of being mostly correctly (but not entirely), perhaps it could be enabled as a check for new formula only.

We could add a tickbox to .github/PULL_REQUEST_TEMPLATE.md that asks the user to run brew linkage foo and consider whether any of the Possible build or unnecessary dependencies may in fact be a :build dependency or unnecessary.

MikeMcQuaid · 2017-09-04T18:01:37Z

Library/Homebrew/os/mac/linkage_checker.rb

@@ -77,6 +78,11 @@ def check_undeclared_deps
        a <=> b
      end
    end
+    extraneous_deps = declared_dep_names.reject do |full_name|
+      name = full_name.split("/").last
+      Formula[name].bin.directory? || @brewed_dylibs.keys.map { |x| x.split("/").last }.include?(name)


next true if Formula[name].bin.directory? and put the map on a new line so it is a little shorter.

perhaps next true unless ...?

nope, next true if... :)

MikeMcQuaid · 2017-09-04T18:02:15Z

unnecessary

Let's go for this and then we can merge as-is if it's useful to you like that.

sjackman · 2017-09-05T17:08:45Z

I'd suggest also renaming the code extraneous_deps to unnecessary_deps in that case, to match.

maxim-belkin · 2017-09-05T17:11:02Z

Sure, will do once I get to it. Should be today (evening).

maxim-belkin · 2017-09-05T20:34:10Z

In my second implementation of this PR (which I'm abandoning), I had to factor out declared_dep_names method from check_undeclared_deps

I can do that here too, if it seems reasonable/necessary.

maxim-belkin · 2017-09-05T20:40:06Z

I wonder if we should report possible unnecessary dependencies (PUDs) with bin directories as "tier-2" PUDs

maxim-belkin · 2017-09-05T21:06:40Z

I think the linkage_checker can be improved further. Code that detects undeclared/unnecessary dependencies is executed under --test, when, in fact, it should not.

link

maxim-belkin · 2017-09-06T14:52:53Z

can someone reinitiate the build? I think there was an intermittent issue

ilovezfs · 2017-09-06T14:55:03Z

Those errors are currently affecting most things 🙈

maxim-belkin · 2017-09-06T14:57:13Z

💩

maxim-belkin · 2017-09-06T18:29:29Z

Could someone have a look at the following lines of this PR more closely: line 8 and 125 through 127.

ilovezfs · 2017-09-11T14:38:16Z

@maxim-belkin you need to rebase the PR or it will keep failing.

maxim-belkin · 2017-09-11T14:39:56Z

it did not fail - it did not run one of the tests. but I'll rebase the PR.

- rename 'extraneous' to 'unnecessary' - add the report under `linkage --test`

maxim-belkin · 2017-09-12T00:18:54Z

any suggestion how to improve test fix "codecov/patch"?

MikeMcQuaid · 2017-09-18T18:28:16Z

Looks good to me, nice work @maxim-belkin!

maxim-belkin · 2017-09-18T18:29:21Z

Glad to contribute!

MikeMcQuaid reviewed Sep 4, 2017

View reviewed changes

maxim-belkin closed this Sep 8, 2017

maxim-belkin reopened this Sep 8, 2017

maxim-belkin closed this Sep 11, 2017

maxim-belkin reopened this Sep 11, 2017

maxim-belkin added 4 commits September 11, 2017 09:40

linkage_checker: check for extraneous dependencies

6c962c2

linkage_checker: fix style

0037d5e

linkage_checker: avoid some false positives

049a453

linkage_checker: unnecessary dependencies

f2531d1

- rename 'extraneous' to 'unnecessary' - add the report under `linkage --test`

maxim-belkin force-pushed the extraneous_deps branch from 756a3ad to f2531d1 Compare September 11, 2017 14:47

MikeMcQuaid approved these changes Sep 18, 2017

View reviewed changes

MikeMcQuaid merged commit 81d9f71 into Homebrew:master Sep 18, 2017

maxim-belkin deleted the extraneous_deps branch September 18, 2017 18:29

Homebrew locked and limited conversation to collaborators May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

linkage_checker: add a check for extraneous dependencies #3103

linkage_checker: add a check for extraneous dependencies #3103

linkage_checker: add a check for extraneous dependencies #3103

linkage_checker: add a check for extraneous dependencies #3103

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment