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

llvm (all versions): add missing versioned libLLVM symlink; fix Mojave sysroot #47146

Closed
wants to merge 4 commits into from

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Nov 25, 2019

llvm-config expects libLLVM-9.dylib (or the equivalent for LLVM 6-8) in order to properly function. It is however missing and I believe this to be the cause of #47142.

Since I was here revision bumping to include this fix, I've also taken the opportunity to fix the default sysroot for Mojave. We didn't get any reported issues about it AFAIK (only for GCC) - but now the behavior matches that of Xcode's LLVM build.

@fxcoudert
Copy link
Member

If llvm-config expects this, why isn't llvm installing the symlink itself? Something seems not right here… we shouldn't have to symlink manually, the build process should install all necessary files.

@Bo98
Copy link
Member Author

Bo98 commented Nov 25, 2019

That question has been asked before but unanswered. It apparently does work on Linux.

The issue was first picked up in a comment of this code review however it was only noticed a few months later. A bug was eventually reported but however did not get a response.

It is documented as supposed to still work - though those docs haven't been updated in years and still mention LLVM 2.7.

@fxcoudert
Copy link
Member

The relevant cmake file is there: https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-shlib/CMakeLists.txt
I'm not sure what's making it work on Linux but not on Darwin. But I don't like to hardcode symlinks that upstream doesn't provide.

@Bo98
Copy link
Member Author

Bo98 commented Nov 25, 2019

There's an unexplained NOT APPLE in the symlink part of their CMake script (introduced in the above code review - but I think the prior code didn't do it either for CMake) but llvm-config explicitly declares in code for Darwin that it should be there. One or the other is wrong.

I believe this did not affect their old, and long gone, autoconf system.

@Bo98
Copy link
Member Author

Bo98 commented Nov 25, 2019

I can see if it's third time lucky and reraise the question upstream if you want.

@fxcoudert
Copy link
Member

Please do. If we add hack after hack in our formulas, they will only get unmaintainable. And the root issue is never fixed.

To some extent, if LLVM ships a compiler with breakage in some corner cases, it's their problem and we don't want to patch every little issue.

@fxcoudert fxcoudert added the upstream issue An upstream issue report is needed label Dec 27, 2019
@fxcoudert
Copy link
Member

I'm gonna close this: it is for LLVM to fix, it's clearly a bug in their build system.

@fxcoudert fxcoudert closed this Dec 27, 2019
@Bo98
Copy link
Member Author

Bo98 commented Dec 28, 2019 via email

@lock lock bot added the outdated PR was locked due to age label Jan 27, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age upstream issue An upstream issue report is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants