Allow missing libraries #7847

maxim-belkin · 2020-06-28T04:43:59Z

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?

This enables formula creators/editors to say that the formula can have broken linkage by adding allow_missing_libs to the formula definition.
Things to consider:

Shall this be a bit more precise and affect broken_dylibs only?
Shall this be always false on a Mac and define allow_missing_libs on Linux only (under extend/os/linux/linkage_checker)?

CC @sjackman @iMichka

maxim-belkin · 2020-06-28T04:47:04Z

Testing this on root formula (Homebrew on Linux Docker container):

$ brew linkage --test root
Missing libraries:
  libASImage.so
  libCling.so
  libCore.so
  libEG.so
  libFTGL.so
  libFoam.so
  libGLEW.so
  libGX11.so
  libGed.so
  libGeom.so
  libGpad.so
  libGraf.so
  libGraf3d.so
  libGui.so
  libHist.so
  libHistFactory.so
  libImt.so
  libMLP.so
  libMathCore.so
  libMathMore.so
  libMatrix.so
  libMinuit.so
  libMultiProc.so
  libNet.so
  libNew.so
  libPhysics.so
  libProof.so
  libProofPlayer.so
  libRCsg.so
  libRGL.so
  libRHTTP.so
  libRIO.so
  libROOTBrowserv7.so
  libROOTDataFrame.so
  libROOTGpadv7.so
  libROOTHist.so
  libROOTHistDraw.so
  libROOTNTuple.so
  libROOTVecOps.so
  libROOTWebDisplay.so
  libRint.so
  libRooFit.so
  libRooFitCore.so
  libRooStats.so
  libTMVA.so
  libThread.so
  libTree.so
  libTreePlayer.so
  libTreeViewer.so
  libWebGui6.so
  libXMLIO.so
  libXMLParser.so
  libXrdProofd.so
  libvdt.so
Broken library linkage ignored

[linuxbrew@docker] [Sun Jun 28 04:45:46] [Last Exit Code: 0]

iMichka · 2020-06-28T19:43:00Z

Can we have a mechanism for macOS/Linux? I do not know any of these cases on macOS, and a few on Linux, so this is mosty a Linux-only "issue". The idea would be to have two different lists/functions for both OSes.

maxim-belkin · 2020-06-28T20:17:42Z

I'm not sure I understand the question/request. The way I implemented it here is that addition of allow_missing_libs line to formulae that have "broken" linkage should let them pass brew linkage --test test. I looked at the output of missing libs for root package and thought that listing specific library names would be an ovekill. In the current implementation, formulae in both homebrew-core and linuxbrew-core can do that. I can move things around to make it Linux-only -- I just need to know if this is what is wanted/needed.

MikeMcQuaid · 2020-06-29T07:55:08Z

Can you provide an example usage of what the root formula would look like with this?

Shall this be a bit more precise and affect broken_dylibs only?

Yes, I think so. I think this should specify a list of libraries (or even a glob) and there needs to be a match otherwise it's an error (so we are forced to remove these when they no longer reply).

Shall this be always false on a Mac and define allow_missing_libs on Linux only (under extend/os/linux/linkage_checker)?

If we have no usage on macOS so far: yes, I think so. In that case the DSL should be designed in such a way that we could add it on macOS later if need be and be clear that it only applies to Linux now.

Nice work @maxim-belkin!

maxim-belkin · 2020-06-29T10:34:21Z

Can you provide an example usage of what the root formula would look like with this?

In the current implementation, it'll look like this:

https://github.com/maxim-belkin/linuxbrew-core/blob/823efb5eb087a64c108aa5e5087cf3dcbe3e288c/Formula/root.rb#L57

(not sure why the above link doesn't render into a code snippet 😕)

I think this should specify a list of libraries (or even a glob) and there needs to be a match otherwise it's an error (so we are forced to remove these when they no longer reply).

Makes sense. Shall it be implemented as a block or similarly to skip_clean?

If we have no usage on macOS so far: yes, I think so. In that case the DSL should be designed in such a way that we could add it on macOS later if need be and be clear that it only applies to Linux now.

OK, I'll update the PR accordingly. But just to make sure we're on the same page: do you expect formulae on macOS to respond false to .allow_missing_libs? (or whatever other name you may suggest -- I spent 1.3 seconds thinking about it) or fail on that call?

MikeMcQuaid · 2020-06-29T11:26:22Z

(not sure why the above link doesn't render into a code snippet 😕)

Not the same repo.

In the current implementation, it'll look like this:

https://github.com/maxim-belkin/linuxbrew-core/blob/823efb5eb087a64c108aa5e5087cf3dcbe3e288c/Formula/root.rb#L57

Gotcha. Yeh, I think this should specify the libraries and have something in the name or a parameter that mentions linux.

Shall it be implemented as a block or similarly to skip_clean?

Similar to skip_clean works for me. Could even allow providing a regex (which errors if there's 0️⃣ matches).

OK, I'll update the PR accordingly. But just to make sure we're on the same page: do you expect formulae on macOS to respond false to .allow_missing_libs? (or whatever other name you may suggest -- I spent 1.3 seconds thinking about it) or fail on that call?

Thanks! I think it depends on whether the name is Linux-y or whether it takes a linux parameter. @Homebrew/linux thoughts?

iMichka · 2020-06-29T11:48:31Z

I think we should go with linux in the method's name. We have done the same for on_macos/on_linux, where we have specific methods by OS, and not one generic method with a parameter.

By the way, I think the root libraries are files provided by root. Maybe there we would not need to specificy the list of files?
For openjdk though, there are also external shared libraries, so there a list might be useful.

Or we can just go for a list of files everywhere to be consistent. There won't be that many formuale needing this anyway (maybe 3-4).

MikeMcQuaid · 2020-06-29T12:19:36Z

I think we should go with linux in the method's name. We have done the same for on_macos/on_linux, where we have specific methods by OS, and not one generic method with a parameter.

Or: it always has to be inside a on_linux block? My only concern is if we do at some point decide we need this on macOS that renaming this method is going to be a pain (and also we have no methods except on_linux that have linux in their name and only uses_from_macos for macOS which wouldn't ever be used on Linux anyway).

Or we can just go for a list of files everywhere to be consistent. There won't be that many formuale needing this anyway (maybe 3-4).

A middle-ground would be a glob. My only concern is this list is pretty long.

By the way, I think the root libraries are files provided by root. Maybe there we would not need to specificy the list of files?

If these are all files that are in lib: maybe just setting it without a list is enough?

sjackman · 2020-06-30T18:37:26Z

Or: it always has to be inside a on_linux block?

👍

Bike shedding:

on_linux do
  ignore_missing_libraries %w[libASImage.so libCling.so libCore.so]
end

or perhaps skip_missing_libraries.

MikeMcQuaid · 2020-07-01T13:58:20Z

on_linux do
  ignore_missing_libraries %w[libASImage.so libCling.so libCore.so]
end

Works for me 👍.

maxim-belkin · 2020-07-10T21:45:15Z

Updated. Formulae can now specify ignore_missing_libraries as an array of strings and/or regex-es:

ignore_missing_libraries ["libASImage.so", "libCling.so", "libCore.so", %r{libxcb\.so\.\d}]

brew style is now unhappy with the number of lines in formula.rb:

$ brew style formula.rb
== formula.rb ==
C: 52:  1: Class has too many lines. [1403/1400]

1 file inspected, 1 offense detected

Library/Homebrew/linkage_checker.rb

maxim-belkin · 2020-07-10T23:57:00Z

Shall we add a special case :self to allow missing linkages to libraries provided by the formula itself?

sjackman · 2020-07-14T00:32:54Z

Shall we add a special case :self to allow missing linkages to libraries provided by the formula itself?

I'm inclined not to add the additional complexity. ignore_missing_libraries should be used relatively rarely.

jonchang · 2020-07-16T13:49:21Z

I think this needs a rebase and a quick brew style --fix.

jonchang · 2020-07-17T13:14:30Z

jonchang · 2020-07-17T13:15:09Z

Thanks @maxim-belkin! Note that this shouldn't be used in formulae until we tag a release that contains this commit, but it can be tested on e.g. CI builds.

MikeMcQuaid

Nice work! Some comments that'd be nice to address.

MikeMcQuaid · 2020-07-20T07:58:04Z

Library/Homebrew/extend/os/linux/formula.rb

+      case x
+      when Regexp
+        x.match? lib
+      when String
+        lib.include? x
+      end


What about when it's neither? Would be nice to error in that case.

We can add some logic here but... shall we wait and see if there is a need for that? Right now I suspect there will be 0 use cases for that error-catching "else".

This will fail non-gracefully if there's a need for that. An else with a raise is minimal code to be maintained but will produce a nicer error message

This will fail non-gracefully if there's a need for that.

How would this fail? I thought it'll just quietly ignore it. I'm probably missing something... Anyways, I agree with adding an else but shall it odie or owarn?

Well, I guess "fail" is the wrong word here but "silently ignore the invalid non-String/non-Regex input" is probably a better one.

MikeMcQuaid · 2020-07-20T07:58:36Z

Library/Homebrew/extend/os/linux/formula.rb

  class << self
    undef on_linux

    def on_linux(&_block)
      yield
    end
+
+    def ignore_missing_libraries(*libs)
+      libs.flatten!


This will modify the caller which doesn't seem like what we'd want. Merging with libs.flatten seems better.

What will happen if you use this on macOS? I'd say it should have a custom error rather than "method not defined"

MikeMcQuaid · 2020-07-20T08:00:18Z

Library/Homebrew/linkage_checker.rb

+  def broken_dylibs_with_expectations
+    output = {}
+    @broken_dylibs.each do |lib|
+      output[lib] = (unexpected_broken_libs.include? lib) ? ["unexpected"] : ["expected"]


Could this be an if? Bit hard to follow as-is.

maxim-belkin force-pushed the allow_missing_libs branch 2 times, most recently from c6e78a8 to e165f45 Compare July 10, 2020 21:40

sjackman reviewed Jul 10, 2020

View reviewed changes

Library/Homebrew/linkage_checker.rb Outdated Show resolved Hide resolved

maxim-belkin force-pushed the allow_missing_libs branch from e165f45 to f8ece3d Compare July 10, 2020 23:41

maxim-belkin mentioned this pull request Jul 12, 2020

Extensible Homebrew DSLs #7984

Closed

maxim-belkin added 3 commits July 16, 2020 13:51

Allow missing libraries

e7b3b8e

Formula: ignore_missing_libraries DSL

c61aba4

Fix style (Convert if-elsif to case-when)

f2ddcda

maxim-belkin force-pushed the allow_missing_libs branch from f8ece3d to f2ddcda Compare July 16, 2020 13:53

jonchang merged commit 13f0d4a into Homebrew:master Jul 17, 2020

MikeMcQuaid reviewed Jul 20, 2020

View reviewed changes

maxim-belkin deleted the allow_missing_libs branch August 17, 2020 18:47

BrewTestBot added the outdated PR was locked due to age label Dec 17, 2020

Homebrew locked as resolved and limited conversation to collaborators Dec 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow missing libraries #7847

Allow missing libraries #7847

maxim-belkin commented Jun 28, 2020 •

edited

maxim-belkin commented Jun 28, 2020

iMichka commented Jun 28, 2020

maxim-belkin commented Jun 28, 2020 •

edited

MikeMcQuaid commented Jun 29, 2020

maxim-belkin commented Jun 29, 2020

MikeMcQuaid commented Jun 29, 2020

iMichka commented Jun 29, 2020

MikeMcQuaid commented Jun 29, 2020

sjackman commented Jun 30, 2020

MikeMcQuaid commented Jul 1, 2020

maxim-belkin commented Jul 10, 2020 •

edited

maxim-belkin commented Jul 10, 2020

sjackman commented Jul 14, 2020

jonchang commented Jul 16, 2020

jonchang commented Jul 17, 2020

jonchang commented Jul 17, 2020

MikeMcQuaid left a comment

MikeMcQuaid Jul 20, 2020

maxim-belkin Jul 20, 2020

MikeMcQuaid Jul 20, 2020

maxim-belkin Jul 21, 2020

MikeMcQuaid Jul 21, 2020

MikeMcQuaid Jul 20, 2020

MikeMcQuaid Jul 20, 2020

MikeMcQuaid Jul 20, 2020

Allow missing libraries #7847

Allow missing libraries #7847

Conversation

maxim-belkin commented Jun 28, 2020 • edited

maxim-belkin commented Jun 28, 2020

iMichka commented Jun 28, 2020

maxim-belkin commented Jun 28, 2020 • edited

MikeMcQuaid commented Jun 29, 2020

maxim-belkin commented Jun 29, 2020

MikeMcQuaid commented Jun 29, 2020

iMichka commented Jun 29, 2020

MikeMcQuaid commented Jun 29, 2020

sjackman commented Jun 30, 2020

MikeMcQuaid commented Jul 1, 2020

maxim-belkin commented Jul 10, 2020 • edited

maxim-belkin commented Jul 10, 2020

sjackman commented Jul 14, 2020

jonchang commented Jul 16, 2020

jonchang commented Jul 17, 2020

jonchang commented Jul 17, 2020

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxim-belkin commented Jun 28, 2020 •

edited

maxim-belkin commented Jun 28, 2020 •

edited

maxim-belkin commented Jul 10, 2020 •

edited