Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linkage_checker: ignore broken linkage with LLVM libc++. #13833

Merged
merged 1 commit into from Sep 19, 2022

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Sep 9, 2022

  • 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?

When dyld is not able to find a library at its install name, it
attempts to find it inside its default search path.

This means that for libraries we ship that are also provided by the
system (e.g. libc++), brew linkage can fail even if the formula
still works (i.e. brew test succeeds).

Given this, it makes sense to attempt to dlopen the library first
before deciding the linkage is broken. This will be useful when we ship
Homebrew/homebrew-core#106925, which will move the install location of
libc++, causing brew linkage to fail for any formula that links with
that libc++.

Those formulae that link with LLVM's libc++ will fall back to Apple's
libc++, which is why we see multiple brew tests succeed even when
brew linkage failed in the LLVM PR linked above.

This change helps avoid unnecessary source rebuilds for formulae that
link with LLVM's libc++ after LLVM 15 ships.

@carlocab carlocab requested a review from Bo98 September 9, 2022 04:52
@BrewTestBot
Copy link
Member

Review period will end on 2022-09-12 at 04:52:28 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Sep 9, 2022
# this search procedure. If this succeeds, the formula is unlikely
# to be broken.
@system_dylibs << basename
elsif (dep = dylib_to_dep(dylib))
@broken_deps[dep] |= [dylib]
elsif MacOS.version >= :big_sur && dylib_found_via_dlopen(dylib)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is still necessary, since System Frameworks typically cannot be found by their basename.

@@ -196,7 +196,14 @@ def check_dylibs(rebuild_cache:)
rescue Errno::ENOENT
next if harmless_broken_link?(dylib)

if (dep = dylib_to_dep(dylib))
basename = File.basename(dylib)
if dylib_found_via_dlopen(basename)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this first does change the output of brew linkage slightly. System dylibs (e.g. libSystem.B.dylib) will typically be reported with only their basename, but system frameworks (e.g. CoreServices) need to be found via an absolute path so one will be reported.

This is probably more meaningful anyway, since these paths are no longer on-disk.

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Sep 9, 2022
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Sep 9, 2022
@BrewTestBot
Copy link
Member

BrewTestBot commented Sep 9, 2022

Review period ended.

@Bo98
Copy link
Member

Bo98 commented Sep 9, 2022

This makes sense for libc++ (if nothing was compiled with -D_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS) but I'm less convinced it's desirable in the general case.

While yes dyld technically may work this way and the executable may "run", if something is (for example) linking to Homebrew curl but ends up using system curl, there could be problems later on and we will want to know about it, even if it's just for the automatic formula reinstall.

I would be ok special casing this for libc++ at the moment though, as long as we note down which formulae are affected by it.

@carlocab
Copy link
Member Author

Yea, I initially started with a special case for libc++ and then wondered whether it should just be more general.

I'll switch it back.

@carlocab carlocab marked this pull request as draft September 13, 2022 11:26
This linkage will be broken in LLVM 15, but this is typically harmless
since dyld will load `/usr/lib/libc++.1.dylib` instead.
@carlocab carlocab changed the title linkage_checker: attempt dlopen before reporting broken linkage. linkage_checker: ignore broken linkage with LLVM libc++. Sep 19, 2022
@carlocab carlocab marked this pull request as ready for review September 19, 2022 04:39
@carlocab
Copy link
Member Author

Scoped this to only LLVM libc++.

This will report no linkage with libc++ at all if linkage with LLVM's libc++ is broken. Ideally we probably want to report linkage with /usr/lib/libc++.1.dylib instead.

I'm punting on that since I'd like to get this in along with the tag for the Glibc fixes.

@carlocab carlocab merged commit 7603418 into Homebrew:master Sep 19, 2022
@Bo98
Copy link
Member

Bo98 commented Sep 19, 2022

Ideally we probably want to report linkage with /usr/lib/libc++.1.dylib instead.

In the long term, we'll phase out using LLVM's libc++ in our formulae.

@carlocab carlocab deleted the libcxx-linkage branch September 19, 2022 05:11
@carlocab
Copy link
Member Author

In the long term, we'll phase out using LLVM's libc++ in our formulae.

This should already be phased out in Homebrew/homebrew-core#106925. I'm just thinking of formulae in external taps that declare an LLVM dependency and inadvertently link with LLVM's libc++.

@github-actions github-actions bot added the outdated PR was locked due to age label Oct 20, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants