Linkage checker: flag unrequested dependencies in --test

[mistydemeo]

• 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?
• Have you successfully run brew man locally and committed any changes?

---

@claui noticed when running brew test-bot locally that we don't flag unrequested dependencies when building bottles, even though we flag broken linkage. This wasn't what we expected from brew linkage --test, which we thought would flag these kinds of problems. This adds unrequested dependencies to the list of things being tested by brew linkage --test.

For example, I have a fish bottle with an unrequested gettext dependency. Before:

$ brew linkage fish --test
No broken library linkage detected
$ echo $?
0
$ brew linkage fish
System libraries:
  /usr/lib/libSystem.B.dylib
  /usr/lib/libc++.1.dylib
  /usr/lib/libncurses.5.4.dylib
Homebrew libraries:
  /opt/homebrew/opt/gettext/lib/libintl.8.dylib (gettext)
Undeclared dependencies with linkage:
  gettext

After:

$ brew linkage fish --test
No broken library linkage detected
Undeclared dependencies detected
$ echo $?
1

/cc @MikeMcQuaid

[MikeMcQuaid]

Would like to bring in @Homebrew/core here to check that this is desirable as it's likely to cause a bunch of CI failures whenever opportunistic linking happens.

[MikeMcQuaid]

Suggested change

	puts "Undeclared dependencies detected" if undeclared_dependencies?
	puts "Undeclared dependencies detected" if undeclared_dependencies.present?

[MikeMcQuaid]

Suggested change

	def undeclared_dependencies?
	!undeclared_deps.empty?
	end

[claui]

it's likely to cause a bunch of CI failures whenever opportunistic linking happens.

@MikeMcQuaid We could try and see if it’s too much to handle. We can always revert.

[claui]

Looks good, factoring in @MikeMcQuaid’s suggestions.
Awaiting more feedback from fellow maintainers.

[SMillerDev]

it's likely to cause a bunch of CI failures whenever opportunistic linking happens.

As far as I know that's the point of the audit so that seems fine to me. I'll be here to complain if it becomes too much 😄

[iMichka]

Same here, I think having this audit will save us issues and bugs in the long run, so I am definitively in favour of it.

[MikeMcQuaid]

I remember a previous maintainer had complained about this but I can't remember who and it may have been someone who left the project. Let's give them 24h from my comment and merge if no-one has objections after that.

[fxcoudert]

As far as I understand, unless CI is fixed to avoid creating the undesirable opportunistic linkages, this will flag them (which is good) but provide no way to fix them (which is bad). I'm trying to explain with a hypothetical below.

---

Suppose we have 3 formulas: A, B, and C.

• Both B and C depend on A.
• B also depends on W.
• C can link against W, but in Homebrew we decided not to go that way, and C does not depend on W.

We create a PR to update A, and because it's a major update, this requires revision bumps to B and C. The test-bot will rebuild A, then install W to rebuild B, then rebuild C without removing W. Thus, C is built with opportunistic linkage to W. Right now, this is already a problem. With this PR, it will still be a problem, and we will get an error, but it still does not help us fix the problem.

For some formulas, we can build C with a suitable option (--without-W). But that's not always possible. Also, it is tedious and annoying to build and manually disable every other possibility we think of.

[claui]

@fxcoudert If our CI catches C opportunistically linking to W, why not make it a policy to resolve this by just letting C have its way and always adding the dependency it wants so badly?

[fxcoudert]

@claui we have to weigh the pros and cons of providing more features and adding more dependencies, because they come at a cost for the user (disk space, download, etc). And a cost for us, because it means more revision bumps, more rebuilds.

Also: in some cases if we add all optional features we'd have a circular dependency graph, which isn't possible. There are already a couple of cases in homebrew-core where we disable features explicitly to avoid that.

[claui]

@fxcoudert Makes sense. Are we talking about 5 such cases a year or more like 50?
(I mean the expected number of extra CI failures, not the circular dependencies case)

[gromgit]

Third-party POV: I was personally involved in one such case (Homebrew/homebrew-core#64654). The initial build went perfectly, with neither ffmpeg@2.8 or libav picking up any opportunistic X11 linkages. Then both were rebuilt within a few days for reasons unknown, and in CI contexts that included libx11, so both inadvertently picked it up. This was only discovered later, when ffmpeg@2.8 failed when testing one of its bumped dependencies in a non-X11 context (Homebrew/homebrew-core#64941).

In the end, this wasn't a hard puzzle to solve, but stuff like this is guaranteed to "explode" at unexpected moments. Extraneous linkages in a formula never cause problems during the build/test on itself, but always further down the line in a different context, and when building/testing something else. Worse, each rebuild of a complex formula in a different CI context may end up picking up a different set of extra linkages from the last time, leading to different complaints of missing libraries that seem to come out of nowhere.

Ideally, each formula should be built in a cleaned-out environment even when multiple formulae are involved in a single PR, but I imagine that gets impractical pretty quickly. However, as long as the declared dependencies remain unchanged, every build in any context should result in zero unexpected linkages even if extraneous formulae happen to be present, and any deviation should be flagged during the build, so that it can be addressed before a problematic bottle is released for general consumption.

[MikeMcQuaid]

@fxcoudert If our CI catches C opportunistically linking to W, why not make it a policy to resolve this by just letting C have its way and always adding the dependency it wants so badly?

We do this automatically (sort of). Formula#runtime_dependencies (which we try to use anywhere we output or check dependencies for an installed formula) is based on the actual runtime linkage that we detected rather than what's declared in the formula. As a result, opportunistic linkage is (much) less of a problem since we added that.

[claui]

We do this automatically (sort of). Formula#runtime_dependencies (which we try to use anywhere we output or check dependencies for an installed formula) is based on the actual runtime linkage that we detected rather than what's declared in the formula. As a result, opportunistic linkage is (much) less of a problem since we added that.

Ok, given that the check is already active in several places, I’d expect the number of additional cases introduced by this PR to be pretty low. Low enough so we could just make it a habit to promote the unexpected dependency to a declared dependency and move on, correct?

[MikeMcQuaid]

Low enough so we could just make it a habit to promote the unexpected dependency to a declared dependency and move on, correct?

I defer to @fxcoudert on that. It sounds like he doesn't agree and if he doesn't: I don't either.

[claui]

It sounds like he doesn't agree and if he doesn't: I don't either.

@MikeMcQuaid Apologies if my words came across as a lobbying attempt. Didn’t mean to.
Just trying to be helpful and Miss Marple-ing together the anecdata.

This bug doesn’t even affect me. I work around it by brew rm-ing all the kegs.

[sjackman]

If our CI catches C opportunistically linking to W, why not make it a policy to resolve this by just letting C have its way and always adding the dependency it wants so badly?

I agree with Claudia that if there is a de facto dependency of C on W, then it ought to be added as an explicit dependency in the formula. I comment could explain why. Or alternatively add configure --without-W to C.

[sjackman]

If this discussion is contentious (say more than two maintainers on each side), then it'd be a great topic to refer to the @Homebrew/tsc.

[fxcoudert]

Note: I don't object to this change, but I think it should be accompanied by an improvement of test-bot to be better at uninstalling dependencies when several formulas are built in the same PR. Otherwise, I worry a. we'll have a lot of failures, b. to solve those we'll create a lot of new dependencies that we don't actually need, and bloat everything.

[MikeMcQuaid]

If this discussion is contentious (say more than two maintainers on each side), then it'd be a great topic to refer to the @Homebrew/tsc.

@sjackman I'd like to gently push back on this and request we hold referring (more) things to the TSC until we're blocked completely here.

Note: I don't object to this change, but I think it should be accompanied by an improvement of test-bot to be better at uninstalling dependencies when several formulas are built in the same PR. Otherwise, I worry a. we'll have a lot of failures, b. to solve those we'll create a lot of new dependencies that we don't actually need, and bloat everything.

I would also welcome this.

[sjackman]

I'd like to gently push back on this and request we hold referring (more) things to the TSC until we're blocked completely here.

For sure. It's preferable that the maintainers come to a consensus on their own if possible.

[mistydemeo]

Note: I don't object to this change, but I think it should be accompanied by an improvement of test-bot to be better at uninstalling dependencies when several formulas are built in the same PR.

This seems reasonable and we can make sure that happens.

The ultimate goal of this PR was to prevent broken bottles from shipping to our users - which is its own kind of friction. We encountered multiple cases of this in building initial ARM bottles, and only caught it in manual testing.

[MikeMcQuaid]

This seems reasonable and we can make sure that happens.

I hope I'm not being too cynical or pedantic when I say: if we have multiple people agreed that this is important then I'd suggest the test-bot PR be merged before or alongside this one.

[fxcoudert]

The ultimate goal of this PR was to prevent broken bottles from shipping to our users - which is its own kind of friction

Agreed, of course. But not all bottles are created equal, and while it's good to catch all such issues (and hopefully, with both this and the testbot change we will reach that point), having a big PR blocked because of low-usage bottle opportunistic linkage would make our life much harder that at current.

I'd suggest the test-bot PR be merged before or alongside this one

I agree. And I apologize for not being able to really provide a basis for that PR: I've looked at that code and between its complexity and my Ruby ignorance, it's a bit too difficult for me to draft something.

[mistydemeo]

I'd suggest the test-bot PR be merged before or alongside this one.

I'll tackle a PR for that and try to get something written up soon.

[BrewTestBot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.