test-bot: check all dependents for broken dylibs #107

apjanke · 2016-04-18T01:35:23Z

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. Answer: No. This is mostly in test-bot so we don't have a harness to test it. And brew linkage depends on having a formula installed, so we can't test that reproducibly without interacting with the Homebrew installation.
Have you successfully ran brew tests with your changes locally?

Goes with Homebrew/homebrew-dev-tools/pull/5.
Addresses #60

Description

Has test-bot check all dependents of changed formulae for dylib link breakage, independent of whether they have a test do block.

Pulls 'brew linkage' in to main brew repo as a dev-cmd, and has test-bot use it to detect dylib breakage, which usually means a revision bump is needed. Checks all dependents, not just those with a 'test do' block defined, since we can do this without formula support.

This follows up #60 ("Preventing formulae updates from breaking their dependencies") and partially addresses its concerns. Doesn't fully fix them, because the decoupling between repos means that test-bot still won't be able to validate cross-repo library dependencies. It will take manual work on the PR author's part to make sure separate PRs are made for revision bumps dependent repos and that all formulae are included.

Testing

You can see an example of the new output at https://gist.github.com/apjanke/77f7e593c922a58408bdc1f92964bcec.

This was produced with brew test-bot --keep-logs --junit --local --verbose --skip-homebrew --skip-setup icu4c | tee test-bot-with-linkage.txt run against the current repo states.

You can see it catching dylib breakage that's still in some of icu4c's dependents. Some of these were not caught by the existing tests, because they don't define a test do block, so they're not "testable". This would show up in Jenkins as a failed test step.


==> brew linkage --test gptfdisk
No broken dylib links
==> brew linkage --test libfolia
Broken dylib links:
  /usr/local/opt/icu4c/lib/libicudata.56.1.dylib
  /usr/local/opt/icu4c/lib/libicui18n.56.dylib
  /usr/local/opt/icu4c/lib/libicuio.56.dylib
  /usr/local/opt/icu4c/lib/libicuuc.56.dylib
==> FAILED
==> brew linkage --test ucto
Broken dylib links:
  /usr/local/opt/icu4c/lib/libicudata.56.1.dylib
  /usr/local/opt/icu4c/lib/libicui18n.56.dylib
  /usr/local/opt/icu4c/lib/libicuio.56.dylib
  /usr/local/opt/icu4c/lib/libicuuc.56.dylib
==> FAILED
==> brew linkage --test vislcg3
Broken dylib links:
  /usr/local/opt/icu4c/lib/libicui18n.56.dylib
  /usr/local/opt/icu4c/lib/libicuio.56.dylib
  /usr/local/opt/icu4c/lib/libicuuc.56.dylib
  @rpath/libcg3.0.dylib

Changes and Compatibility

With this, brew linkage is now a dev-cmd inside main brew, instead of a regular cmd in homebrew/dev-tools. Users will need to have HOMEBREW_DEVELOPER enabled to make it visible. Users who had homebrew/dev-tools tapped but didn't have HOMEBREW_DEVELOPER set will lose it.

I chose to put it in dev-cmd because the move was done for test-bot's use, and it was originally a "dev" tool. I think it'd be fine to just have in cmd, too, to avoid breaking things for anybody, if other maintainers think that would be more appropriate.

Any other taps which define a linkage command will now have it masked when HOMEBREW_DEVELOPER is enabled. (Or always, if we choose to make it a regular cmd.)

UniqMartin · 2016-04-18T05:37:57Z

==> brew linkage --test vislcg3
Broken dylib links:
  /usr/local/opt/icu4c/lib/libicui18n.56.dylib
  /usr/local/opt/icu4c/lib/libicuio.56.dylib
  /usr/local/opt/icu4c/lib/libicuuc.56.dylib
  @rpath/libcg3.0.dylib

To avoid getting false positives like in the quoted example, we either need to teach brew linkage how to properly resolve the special linker path prefixes @executable_path/, @loader_path/, and @rpath/ (resolving all of them correctly is non-trivial and sometimes even impossible) or we need to ignore those entries as far as our check for brokenness is concerned (we're building most things with absolute paths anyway, so we're not missing much). I'm strongly suggesting the latter option.

Otherwise generally 👍 on this.

UniqMartin · 2016-04-18T06:00:29Z

Library/Homebrew/cmd/test-bot.rb

@@ -583,7 +583,7 @@ def formula(formula_name)
          end
        end
        test "brew", "test", "--verbose", formula_name if formula.test_defined?
-        testable_dependents.each do |dependent|
+        dependents.each do |dependent|


Unlike the name suggests, testable_dependents are not merely dependents with a test, but dependents that have a test and are bottled (see a bit further up). Switching to dependents potentially means that unbottled dependents will be built from source, just for the sake of checking their linkage, which is likely to be alright due to the from-source build.

Good point. Yeah, testing linkage on non-bottled formulae is probably pointless. I'll modify it to do only bottled ones.

It'd be useful but it's too slow.

Switched to doing just the bottled dependents.

MikeMcQuaid · 2016-04-18T07:10:30Z

Seems good, thanks for this 👍

apjanke · 2016-04-18T07:12:53Z

Switched to testing bottled dependents only.

I modified it to treat install paths starting with @ variables as a different category, and to not consider them as broken links. New output:

$ brew linkage vislcg3
System libraries:
  /usr/lib/libSystem.B.dylib
  /usr/lib/libc++.1.dylib
Variable-referenced libraries:
  @rpath/libcg3.0.dylib
Missing libraries:
  /usr/local/opt/icu4c/lib/libicui18n.56.dylib
  /usr/local/opt/icu4c/lib/libicuio.56.dylib
  /usr/local/opt/icu4c/lib/libicuuc.56.dylib
$ brew linkage --test vislcg3
Broken dylib links:
  /usr/local/opt/icu4c/lib/libicui18n.56.dylib
  /usr/local/opt/icu4c/lib/libicuio.56.dylib
  /usr/local/opt/icu4c/lib/libicuuc.56.dylib
[✘ /usr/local on ⇄ detect-broken-dylibs ±]

I read a couple articles on the @xxx/... install path form, including Mike Ash's Q&A, man dyld, and man install_name_tool. I think validating these is impossible for us, since they are relative paths that are evaluated, at least in part, relative to the program or framework that is linking to the library, which we don't know when we're examining the library on its own. And per the docs, an @rpath that does not resolve is not an error.

(To be strict, maybe we should be checking for unresolved symbols before complaining about referenced library paths that don't resolve. But I think 99% of the time they'll be an error for us regardless, so we probably don't have to check the symbols, too.)

UniqMartin · 2016-04-18T07:24:00Z

Library/Homebrew/dev-cmd/linkage.rb

+      @keg.find do |file|
+        next unless file.dylib? || file.mach_o_executable? || file.mach_o_bundle?
+        file.dynamically_linked_libraries.each do |dylib|
+          if !dylib.empty? && dylib[0] == "@"


I find dylib.start_with?("@") easier to parse.

apjanke · 2016-04-18T14:29:39Z

@MikeMcQuaid You're welcome!

Pulls 'brew linkage' in to main brew repo as a dev-cmd, and has test-bot use it to detect dylib breakage, which usually means a revision bump is needed. Checks all dependents, not just those with a 'test do' block defined, since we can do this without formula support.

apjanke · 2016-04-18T18:16:13Z

And here's what the results look like, from Homebrew/homebrew-core#427.

Gonna go make a revision-bump PR now... :)

apjanke · 2016-04-26T18:36:28Z

It's working! We've caught dylib breakage in PRs a couple times this last week. Example: Homebrew/homebrew-core#621

ahundt · 2016-05-02T21:13:00Z

cool! so is this functionality integrated or not merged? confused by the red closing of the pull request.

apjanke · 2016-05-02T22:47:20Z

Thanks! It's integrated and running now (at least for in-tap dependencies), and we've already caught and fixed a couple broken bottles with it.

Most of our closed pull requests for things that get accepted are still red "closed" instead of purple "merged" because we usually use brew pull instead of GitHub's "merge" button to do the merging. This allows us to edit commit messages, link commit messages back to PRs, include test-bot-generated bottle pulls, squash commits, do local testing, and so on.

If the message says something with a commit identifier like "apjanke closed this in da34fba 14 days ago", that means it was accepted and merged. If the message just says "apjanke closed this 14 days ago" that means it was probably rejected or superseded, and there should be an explicit comment in the discussion indicating why it was rejected.

Sorry if this is confusing.

apjanke · 2016-05-02T22:48:14Z

Maybe GitHub could make a special "Mark as manually merged" button. But maybe that would break their model with tight coupling between commit history and issues.

ahundt · 2016-05-02T23:40:29Z

ah ok I see now, thanks for the clarification! It would be nice if github allowed things to be made clearer more easily... oh well now I know!

apjanke added the features New features label Apr 18, 2016

apjanke mentioned this pull request Apr 18, 2016

Feature Request: Preventing formulae updates from breaking their dependencies #60

Closed

18 tasks

UniqMartin reviewed Apr 18, 2016
View reviewed changes

apjanke closed this in da34fba Apr 18, 2016

apjanke mentioned this pull request Apr 18, 2016

icu4c: dummy revision bump Homebrew/homebrew-core#427

Closed

apjanke deleted the detect-broken-dylibs branch April 18, 2016 18:23

Homebrew locked and limited conversation to collaborators May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test-bot: check all dependents for broken dylibs #107

test-bot: check all dependents for broken dylibs #107

apjanke commented Apr 18, 2016 •

edited

Loading

UniqMartin commented Apr 18, 2016

UniqMartin Apr 18, 2016

apjanke Apr 18, 2016

MikeMcQuaid Apr 18, 2016

apjanke Apr 18, 2016

MikeMcQuaid commented Apr 18, 2016

apjanke commented Apr 18, 2016

UniqMartin Apr 18, 2016

apjanke Apr 18, 2016

apjanke commented Apr 18, 2016

apjanke commented Apr 18, 2016

apjanke commented Apr 26, 2016

ahundt commented May 2, 2016

apjanke commented May 2, 2016

apjanke commented May 2, 2016

ahundt commented May 2, 2016 •

edited

Loading

test-bot: check all dependents for broken dylibs #107

test-bot: check all dependents for broken dylibs #107

Conversation

apjanke commented Apr 18, 2016 • edited Loading

Description

Testing

Changes and Compatibility

UniqMartin commented Apr 18, 2016

UniqMartin Apr 18, 2016

Choose a reason for hiding this comment

apjanke Apr 18, 2016

Choose a reason for hiding this comment

MikeMcQuaid Apr 18, 2016

Choose a reason for hiding this comment

apjanke Apr 18, 2016

Choose a reason for hiding this comment

MikeMcQuaid commented Apr 18, 2016

apjanke commented Apr 18, 2016

UniqMartin Apr 18, 2016

Choose a reason for hiding this comment

apjanke Apr 18, 2016

Choose a reason for hiding this comment

apjanke commented Apr 18, 2016

apjanke commented Apr 18, 2016

apjanke commented Apr 26, 2016

ahundt commented May 2, 2016

apjanke commented May 2, 2016

apjanke commented May 2, 2016

ahundt commented May 2, 2016 • edited Loading

apjanke commented Apr 18, 2016 •

edited

Loading

ahundt commented May 2, 2016 •

edited

Loading