llvm: fix llvm-config #76646

carlocab · 2021-05-04T17:37:04Z

Have you followed the guidelines for contributing?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

llvm-config relies on a versioned libLLVM (e.g. libLLVM-12), but
their build system does not install one for some reason.

With the symlink:

❯ llvm-config --libs
-lLLVM-12

Without:

❯ llvm-config --libs
-lLLVMWindowsManifest -lLLVMXRay -lLLVMLibDriver -lLLVMDlltoolDriver ... -lLLVMRemarks -lLLVMBitstreamReader -lLLVMBinaryFormat -lLLVMTableGen -lLLVMSupport -lLLVMDemangle

This deviates from the intended behaviour, as seen on Linux, where
llvm-config --libs returns the output shown above with the symlink.
Attempting to link against the libraries shown by llvm-config without
this fix is also leads to build failures due to missing symbols.

This will also allow Julia to use brewed llvm instead of a vendored
one. (#76527)

carlocab · 2021-05-04T17:38:09Z

@Bo98 reasonable bandaid until your LLVM patch? Or we could just wait for your patch too, I guess.

Bo98 · 2021-05-04T17:46:10Z

This was suggested before and rejected. Though that was a couple years ago.

Or we could just wait for your patch too

I'm still on the fence whether I want to apply it to LLVM 12 or just let it roll into LLVM 13. Any slight touch to LLVM dylibs usually upsets some downstream project, but doing so on major versions is usually seen as acceptable.

carlocab · 2021-05-04T17:58:31Z

This was suggested before and rejected.

Was the reasoning that this is something that should be fixed upstream? I agree. But, as you point out, it's been years.

I'm still on the fence whether I want to apply it to LLVM 12 or just let it roll into LLVM 13.

Have you upstreamed your patch?

Bo98 · 2021-05-04T18:02:01Z

Have you upstreamed your patch?

I will be in a couple days. I've sent an email to first clarify the ABI status of libclang - it's ABI version 1 in Debian/Ubuntu and ABI version 12 in others.

carlocab · 2021-05-06T23:57:00Z

Thoughts on this, @Homebrew/core?

A similar fix was proposed (and rejected) in #47146. However, the current circumstances are slightly different:

this has been an outstanding issue upstream now for quite some time;
this PR addresses a user-submitted issue (LLVM #76798); and,
this PR enables use of a brewed dependency rather than a vendored one in a separate (proposed) formula (julia 1.6.1 (new formula) #76527)

I'm inclined to say these outweigh the objection at #47146, but I'd like some extra opinions on this.

fxcoudert · 2021-05-07T09:21:48Z

What is the status of the LLVM patch? I see a 2019 bug report with no action (https://bugs.llvm.org/show_bug.cgi?id=40252) but is there an actual patch that has been accepted? Rejected? Ignored?

MikeMcQuaid · 2021-05-07T10:24:36Z

Formula/llvm.rb

@@ -141,6 +142,13 @@ def install
      system "cmake", "--build", ".", "--target", "install-xcode-toolchain" if MacOS::Xcode.installed?
    end

+    on_macos do
+      if build.stable? && !(lib/"libLLVM-#{version.major}.dylib").exist?


When does/doesn't this exist?

The last time I built from HEAD it installed a libLLVM-13git library, so I assume it exists in HEAD builds. Verifying this now is difficult because building from HEAD seems to fail at the moment, plus it takes ages.

I'm not aware of any circumstances for it to exist in a stable build. I just put it there because it was going to be wrapped in a conditional anyway (build.stable?), so may as well add an extra guard to avoid overwriting this library if/when this bug does get fixed.

We can drop the existence check if you prefer.

Yeh, dropping existence check makes sense to me 👍🏻. Lets make this error out if it ever starts to exist so we know to remove this.

Done. Also, added a test to help us more quickly detect breakage from this.

MikeMcQuaid · 2021-05-07T10:24:53Z

What is the status of the LLVM patch?

Would also like to know this.

Bo98 · 2021-05-07T11:41:19Z

I never did anything in 2019 because I could never decide whether the problem was in the llvm-config side or the libLLVM side.

After a year and a half experience in maintaining the LLVM formula, including overseeing a couple major updates, I've identified that the lack of proper ABI versioning in LLVM shared libraries is problematic. Therefore the problem to me is indeed the lack of a libLLVM-12.dylib rather than llvm-config needing patching to not look for it.

Beyond some libclang weirdness, LLVM libraries do not guarantee any sort of ABI compatibility between major releases. The existence of an unversioned libLLVM.dylib violates this assumption. We saw a few missing symbol errors in the LLVM 12 update as a result, and we've likely missed some if any were lazily loaded.

I created this provisional patch which I'm going to propose upstream this weekend to see what their thoughts are. We'll see if there is disagreements but to me this seems like the logical approach.

One thing I've been trying to clarify first is the unique nature of libclang. Apparently this particular one has bigger ABI guarantees between versions. Upstream's build system however applies the same ABI versioning rules (on Linux) to libclang as it does to other LLVM libraries, while Debian patch this so that the libclang they ship is ABI version 1. This discrepency is something I'm hoping to clarify.

Bo98 · 2021-05-07T11:45:48Z

(This diverges a bit from llvm-config problem but the fix would happen to cover both.)

`llvm-config` relies on a versioned `libLLVM` (e.g. `libLLVM-12`), but their build system does not install one for some reason. With the symlink: ❯ llvm-config --libs -lLLVM-12 Without: ❯ llvm-config --libs -lLLVMWindowsManifest -lLLVMXRay -lLLVMLibDriver -lLLVMDlltoolDriver ... -lLLVMRemarks -lLLVMBitstreamReader -lLLVMBinaryFormat -lLLVMTableGen -lLLVMSupport -lLLVMDemangle This deviates from the intended behaviour, as seen on Linux, where `llvm-config --libs` returns the output shown above with the symlink. Attempting to link against the libraries shown by `llvm-config` without this fix is also leads to build failures due to missing symbols. This will also allow Julia to use brewed `llvm` instead of a vendored one. (Homebrew#76527) Closes Homebrew#76798.

carlocab · 2021-05-14T04:29:06Z

Any outstanding questions/objections here? @MikeMcQuaid @fxcoudert

BrewTestBot · 2021-05-14T11:06:27Z

🤖 A scheduled task has triggered a merge.

BrewTestBot added the python Python use is a significant feature of the PR or issue label May 4, 2021

carlocab mentioned this pull request May 4, 2021

julia 1.6.1 (new formula) #76527

Closed

5 tasks

carlocab mentioned this pull request May 6, 2021

LLVM #76798

Closed

2 tasks

MikeMcQuaid reviewed May 7, 2021

View reviewed changes

carlocab force-pushed the llvm-config branch from 80b0d0b to f075712 Compare May 7, 2021 14:45

carlocab force-pushed the llvm-config branch from f075712 to 292354a Compare May 7, 2021 14:54

carlocab mentioned this pull request May 14, 2021

nvc 1.5.1 #77261

Closed

MikeMcQuaid approved these changes May 14, 2021

View reviewed changes

BrewTestBot closed this in c607d7d May 14, 2021

carlocab deleted the llvm-config branch May 14, 2021 11:34

github-actions bot added the outdated PR was locked due to age label Jun 14, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jun 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llvm: fix llvm-config #76646

llvm: fix llvm-config #76646

carlocab commented May 4, 2021

carlocab commented May 4, 2021

Bo98 commented May 4, 2021 •

edited

Loading

carlocab commented May 4, 2021

Bo98 commented May 4, 2021

carlocab commented May 6, 2021 •

edited

Loading

fxcoudert commented May 7, 2021

MikeMcQuaid May 7, 2021

carlocab May 7, 2021

MikeMcQuaid May 7, 2021

carlocab May 7, 2021

MikeMcQuaid commented May 7, 2021

Bo98 commented May 7, 2021 •

edited

Loading

Bo98 commented May 7, 2021

carlocab commented May 14, 2021

BrewTestBot commented May 14, 2021

llvm: fix llvm-config #76646

llvm: fix llvm-config #76646

Conversation

carlocab commented May 4, 2021

carlocab commented May 4, 2021

Bo98 commented May 4, 2021 • edited Loading

carlocab commented May 4, 2021

Bo98 commented May 4, 2021

carlocab commented May 6, 2021 • edited Loading

fxcoudert commented May 7, 2021

MikeMcQuaid May 7, 2021

Choose a reason for hiding this comment

carlocab May 7, 2021

Choose a reason for hiding this comment

MikeMcQuaid May 7, 2021

Choose a reason for hiding this comment

carlocab May 7, 2021

Choose a reason for hiding this comment

MikeMcQuaid commented May 7, 2021

Bo98 commented May 7, 2021 • edited Loading

Bo98 commented May 7, 2021

carlocab commented May 14, 2021

BrewTestBot commented May 14, 2021

Bo98 commented May 4, 2021 •

edited

Loading

carlocab commented May 6, 2021 •

edited

Loading

Bo98 commented May 7, 2021 •

edited

Loading