-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add patch for LLVM-3.9+Clang+macOS Sierra #18920
Conversation
Was this submitted / already fixed upstream? |
No. It's still there on master https://github.com/llvm-mirror/compiler-rt/blob/master/cmake/builtin-config-ix.cmake#L57. They might have different backward compatibility requirements so maybe they want a different patch. |
@@ -0,0 +1,33 @@ | |||
--- a/projects/compiler-rt/cmake/builtin-config-ix.cmake | |||
+++ b/projects/compiler-rt/cmake/builtin-config-ix.cmake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this path always present in the llvm tarball, or can this patch only be applied if clang/compiler-rt is being built?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know. Will check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made the patch conditional on building Clang.
I can confirm that this patch allowed LLVM to build for me (fixing an "invalid deployment target" error) on 10.12.2. |
just needs to be opened as an issue or sent as a patch upstream. what are we doing that makes this a problem that every clang developer at Apple hasn't hit? |
somehow this happens to us with everything we use – especially LLVM. |
Adding
|
Bump? I just had
again on a fresh install. |
Wonder whether the buildbots are new enough to get away with leaving that off. Aren't we setting our deployment target to something 10.7 or later, does it not overrule the flags clang is setting that this patch gets rid of? These look like they're still getting set upstream on trunk https://github.com/llvm-mirror/compiler-rt/blob/master/cmake/builtin-config-ix.cmake so we should raise the issue or patch to figure out if these still need to be set so old upstream. |
This patch still fixes the compilation problem for me. |
And I'm still fine with merging this, as soon as it gets opened as an issue or patch upstream |
Ping. |
We should merge this if it fixes things, and undo it when upstream fixes it. |
It's unlikely to ever get fixed upstream if it never gets reported to them. There might be a better solution in terms of build flags that wouldn't require indefinitely carrying a patch to LLVM's build system (which would repeatedly need to be rebased). |
Removes hardcoded lower version bounds incompatible with Sierra in CMake file for compiler-rt
I just got |
Oh, I see, we bumped to LLVM 6, and this patch was only for LLVM 3.9... Looks like the same patch should still work. |
This is why I repeatedly said
and
Please report things upstream instead of indefinitely carrying patches that only exist here. It's not difficult. |
Someone jog my memory here, is the issue that the Sierra toolchain doesn't support a minimum version as low as |
Removes hardcoded lower version bounds incompatible with Sierra
in CMake file for compiler-rt
This seems to necessary to build Clang on Sierra with LLVM 3.9.
cc: @vchuravy who diagnosed the issue.