linkage_checker: skip broken linkage in Julia #18235

cho-m · 2024-09-03T02:53:06Z

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

Looking into a way to avoid Julia linkage errors since it uses its own dylib lookup logic which brew considers broken linkage.

Seemed to be easier approach rather than trying to inject unneeded RPATHs into the binary that Julia doesn't want.

Example for Julia:

{
  "julia": "share/julia/compiled/**/*"
}

Need to try out a few more things and check on Linux code path.

MikeMcQuaid

Good idea!

Bo98 · 2024-09-03T13:49:26Z

Can you talk about what's broken about Julia currently? We already have it shipped in homebrew-core with passing linkage.

I feel quite strongly that brew linkage should never return false positives, so if it now suddenly is that is very concerning. I know the RPATH part of it is debated (which is why it has never graduated from a warning - maybe it should move to brew audit which does have false positives and allowlists) but the broken linkage one is another story.

We did have a DSL allowlist for skipping missing library checks in the past but we removed it because we ended up fixing the underlying issues.

Is there anything except Julia that would use this? If it's only Julia we might as well hardcode in share/julia/compiled as that's an unusual path to be installing binaries to anyway.

cho-m · 2024-09-03T14:52:51Z

Can you talk about what's broken about Julia currently? We already have it shipped in homebrew-core with passing linkage.

We shipped it with broken linkage when check was just a warning - https://github.com/Homebrew/homebrew-core/pull/173754/commits/cb36837eca49200b31ff45e96d3ea01788a0b15d

The Linux broken linkage covers the same failure as macOS missing RPATHs. It's just that the missing RPATHs are detected by finding @rpath. We could classify Linux shared libraries with non-resolvable NEEDED libraries and without RPATH/RUNPATH as missing, but it could also just be broken.

Is there anything except Julia that would use this? If it's only Julia we might as well hardcode in share/julia/compiled as that's an unusual path to be installing binaries to anyway.

Not sure about others. We could hardcode in Julia. Right now, the dylibs in that directory are not intended to be used with native dynamic loader which is why they trigger linkage error. I think they are run through Julia's loader instead.

Bo98 · 2024-09-03T15:04:48Z

Can you talk about what's broken about Julia currently? We already have it shipped in homebrew-core with passing linkage.

We shipped it with broken linkage when check was just a warning - https://github.com/Homebrew/homebrew-core/pull/173754/commits/cb36837eca49200b31ff45e96d3ea01788a0b15d

The Linux broken linkage covers the same failure as macOS missing RPATHs. It's just that the missing RPATHs are detected by finding @rpath. We could classify Linux shared libraries with non-resolvable NEEDED libraries and without RPATH/RUNPATH as missing, but it could also just be broken.

RPATH check is still a warning and isn't going to graduate soon (see e.g. GCC) which is why there was the suggestion of moving it to brew audit where the audit_exceptions system would make more sense.

Not sure about Linux - I would have expected that to fail. It would however make sense to adjust the Linux checking to align better with macOS. I don't think we should ever allow ignoring broken linkage on macOS. Maybe that just means making a missing_rpath_allowlist apply more broadly on Linux.

Bo98 · 2024-09-03T15:16:35Z

Perhaps to illustrate the point better: brew linkage --test is important to keep as close to zero false positives as possible without exceptions (in non-strict mode, so excluding warnings) as Homebrew uses it internally to detect if a formula is functional and some third-party CIs do the same.

This linkage check runs even when you are running brew install. audit_exceptions is not available under the API so should not be used to make hard-failing linkage checks pass. Tests that cannot achieve 100% accuracy must be placed under strict mode or relocated to FormulaCellarChecks used by brew audit. So we should not add an allowlist to broken_dylibs as that is checked under non-strict mode.

Strict mode can contain false positives, though is probably overdue reevaluating what's there and why.

cho-m · 2024-09-03T16:12:48Z

Not sure about Linux - I would have expected that to fail.

Currently it does fail so probably fixed (or at least elevated from warning), e.g.

This linkage check runs even when you are running brew install. audit_exceptions is not available under the API so should not be used to make hard-failing linkage checks pass. Tests that cannot achieve 100% accuracy must be placed under strict mode or relocated to FormulaCellarChecks used by brew audit. So we should not add an allowlist to broken_dylibs as that is checked under non-strict mode.

I guess easiest path for now is the hardcoding of Julia. Moving it to brew audit may be better in long run.

carlocab

May as well add && !broken_dylibs_allowed?(file) here too:

brew/Library/Homebrew/linkage_checker.rb

Line 131 in f0be97f

@files_missing_rpaths << file if pathname.rpaths.empty?

cho-m · 2024-09-10T17:04:22Z

On topic of Julia but unrelated to this PR, it may be nice to detect symlinks to other formulae and use that to avoid some "Dependencies with no linkage". Julia does this for most dependencies thus we get:

Dependencies with no linkage:
  ca-certificates
  gmp
  libnghttp2
  libssh2
  mpfr
  openblas
  openlibm

❯ eza -1 $(brew --prefix julia)/lib/julia/ | rg ' -> \.\.'
libatomic.1.dylib -> ../../../../../opt/gcc/lib/gcc/current/libatomic.1.dylib
libblastrampoline.dylib -> ../../../../../opt/libblastrampoline/lib/libblastrampoline.5.dylib
libcurl.dylib -> ../../../../../opt/curl/lib/libcurl.4.dylib
libgcc_s.1.1.dylib -> ../../../../../opt/gcc/lib/gcc/current/libgcc_s.1.1.dylib
libgfortran.5.dylib -> ../../../../../opt/gcc/lib/gcc/current/libgfortran.5.dylib
libgit2.dylib -> ../../../../../opt/libgit2/lib/libgit2.1.8.dylib
libgmp.dylib -> ../../../../../opt/gmp/lib/libgmp.10.dylib
libgomp.1.dylib -> ../../../../../opt/gcc/lib/gcc/current/libgomp.1.dylib
libmbedcrypto.dylib -> ../../../../../opt/mbedtls@2/lib/libmbedcrypto.7.dylib
libmbedtls.dylib -> ../../../../../opt/mbedtls@2/lib/libmbedtls.14.dylib
libmbedx509.dylib -> ../../../../../opt/mbedtls@2/lib/libmbedx509.1.dylib
libmpfr.dylib -> ../../../../../opt/mpfr/lib/libmpfr.6.dylib
libnghttp2.dylib -> ../../../../../opt/libnghttp2/lib/libnghttp2.14.dylib
libopenblas.dylib -> ../../../../../opt/openblas/lib/libopenblas.0.dylib
libopenlibm.dylib -> ../../../../../opt/openlibm/lib/libopenlibm.4.dylib
libpcre2-8.dylib -> ../../../../../opt/pcre2/lib/libpcre2-8.0.dylib
libquadmath.0.dylib -> ../../../../../opt/gcc/lib/gcc/current/libquadmath.0.dylib
libssh2.dylib -> ../../../../../opt/libssh2/lib/libssh2.1.dylib
libssp.0.dylib -> ../../../../../opt/gcc/lib/gcc/current/libssp.0.dylib
libstdc++.6.dylib -> ../../../../../opt/gcc/lib/gcc/current/libstdc++.6.dylib

carlocab · 2024-09-10T17:06:03Z

On topic of Julia but unrelated to this PR, it may be nice to detect symlinks to other formulae and use that to avoid some "Dependencies with no linkage".

It's nice, but doesn't seem worth the extra code since no other formula does this.

cho-m · 2024-09-10T17:20:57Z

It's nice, but doesn't seem worth the extra code since no other formula does this.

It would work for things like ca-certificates dependents which we always symlink pem file from.

Maybe also ppsspp though we skip linkage check as molten-vk has an executable.

Also related would be header usage (e.g. boost, eigen, etc) and having a section that details that these deps are in use even though they are not linked libs.

carlocab · 2024-09-10T17:29:54Z

It would work for things like ca-certificates dependents which we always symlink pem file from.

That's fair.

Also related would be header usage (e.g. boost, eigen, etc) and having a section that details that these deps are in use even though they are not linked libs.

Header-only usage should probably just be :build and/or :test only TBH.

cho-m · 2024-09-10T17:51:09Z

Library/Homebrew/linkage_checker.rb

@@ -92,6 +92,13 @@ def dylib_to_dep(dylib)
 Regexp.last_match(2)
 end

+ sig { params(file: String).returns(T::Boolean) }
+ def broken_dylibs_allowed?(file)
+ return false if formula.name != "julia"


I noticed there is some logic afterward that checks if formula. I guess there is a chance formula will not be resolved?

brew/Library/Homebrew/linkage_checker.rb

Lines 317 to 320 in 5b0a84b

def resolve_formula(keg)

Formulary.from_keg(keg)

rescue FormulaUnavailableError

opoo "Formula unavailable: #{keg.name}"

If so, can modify to check if formula.blank? && ...

EDIT: Most likely typechecking will fail once we add strict typing. Can look into adding types along with handling this.

Could just do

if formula&.name != "julia"

MikeMcQuaid · 2024-09-11T07:58:11Z

Thanks @cho-m!

MikeMcQuaid approved these changes Sep 3, 2024

View reviewed changes

cho-m force-pushed the broken_linkage_allowlist branch from cb96fbc to 33914a1 Compare September 3, 2024 17:35

cho-m changed the title ~~linkage_checker: add broken_linkage_allowlist~~ linkage_checker: skip broken linkage in Julia Sep 3, 2024

cho-m force-pushed the broken_linkage_allowlist branch from 33914a1 to 052de4e Compare September 3, 2024 18:14

cho-m marked this pull request as ready for review September 10, 2024 05:08

cho-m force-pushed the broken_linkage_allowlist branch from 052de4e to ae075e3 Compare September 10, 2024 05:17

carlocab approved these changes Sep 10, 2024

View reviewed changes

linkage_checker: skip broken linkage in Julia

dd25679

cho-m force-pushed the broken_linkage_allowlist branch from ae075e3 to dd25679 Compare September 10, 2024 16:59

carlocab approved these changes Sep 10, 2024

View reviewed changes

carlocab enabled auto-merge September 10, 2024 17:06

carlocab merged commit c0f08f8 into Homebrew:master Sep 10, 2024
27 checks passed

cho-m deleted the broken_linkage_allowlist branch September 10, 2024 17:20

cho-m commented Sep 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

linkage_checker: skip broken linkage in Julia #18235

linkage_checker: skip broken linkage in Julia #18235

cho-m commented Sep 3, 2024 •

edited

Loading

MikeMcQuaid left a comment

Bo98 commented Sep 3, 2024 •

edited

Loading

cho-m commented Sep 3, 2024 •

edited

Loading

Bo98 commented Sep 3, 2024 •

edited

Loading

Bo98 commented Sep 3, 2024 •

edited

Loading

cho-m commented Sep 3, 2024

carlocab left a comment

cho-m commented Sep 10, 2024

carlocab commented Sep 10, 2024

cho-m commented Sep 10, 2024 •

edited

Loading

carlocab commented Sep 10, 2024

cho-m Sep 10, 2024 •

edited

Loading

carlocab Sep 10, 2024

MikeMcQuaid commented Sep 11, 2024

	def resolve_formula(keg)
	Formulary.from_keg(keg)
	rescue FormulaUnavailableError
	opoo "Formula unavailable: #{keg.name}"

linkage_checker: skip broken linkage in Julia #18235

linkage_checker: skip broken linkage in Julia #18235

Conversation

cho-m commented Sep 3, 2024 • edited Loading

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Bo98 commented Sep 3, 2024 • edited Loading

cho-m commented Sep 3, 2024 • edited Loading

Bo98 commented Sep 3, 2024 • edited Loading

Bo98 commented Sep 3, 2024 • edited Loading

cho-m commented Sep 3, 2024

carlocab left a comment

Choose a reason for hiding this comment

cho-m commented Sep 10, 2024

carlocab commented Sep 10, 2024

cho-m commented Sep 10, 2024 • edited Loading

carlocab commented Sep 10, 2024

cho-m Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

carlocab Sep 10, 2024

Choose a reason for hiding this comment

MikeMcQuaid commented Sep 11, 2024

cho-m commented Sep 3, 2024 •

edited

Loading

Bo98 commented Sep 3, 2024 •

edited

Loading

cho-m commented Sep 3, 2024 •

edited

Loading

Bo98 commented Sep 3, 2024 •

edited

Loading

Bo98 commented Sep 3, 2024 •

edited

Loading

cho-m commented Sep 10, 2024 •

edited

Loading

cho-m Sep 10, 2024 •

edited

Loading