-
-
Notifications
You must be signed in to change notification settings - Fork 12.1k
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 15.0.0 #106925
llvm 15.0.0 #106925
Conversation
Formula/llvm.rb
Outdated
# Linking with these causes you subtle problems on macOS, | ||
# since everything loads `/usr/lib/libc++.1.dylib`. | ||
# https://github.com/Homebrew/homebrew-core/issues/96915 | ||
runtimes += %w[libcxx libcxxabi] if OS.linux? |
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.
Thoughts on this, @Bo98?
On the one hand, I feel like it might be a bit of an overreaction. On the other, we already prefer linking with /usr/lib/libc++.1.dylib
anyway, so it's not like we have much use for these...
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.
Also, I can move this up into the else
block above. I'll do so on the next run.
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 agree that these need to go. Or if for some reason they need to ship, then they cannot live in lib
anymore.
Ideally the C++ headers would stay however, since I think those are necessary for clang++ usage on < 10.15.3 SDK and shouldn't affect anything on newer SDKs.
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.
Alternative is to symlink the C++ header directory to the CLT header directory. Not entirely sure what would work better.
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'm still a little sceptical that this is the right solution, though, since there's been lots of use of the bundled libc++
, even in other formulae (despite our efforts to avoid it). So I don't think the issue is that you're linking with two versions of libc++
-- that's been happening for a long time.
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.
Trying this.
Though, the scan-build
test is failing, even after trying to point it to bin/"clang"
. Seems to be libc++
-related, since it didn't fail when I removed libc++
.
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.
Moving libc++ might end up fixing that, we'll see.
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.
Oh wait, didn't notice it's already failing with the change. Does LLVM 14 fail with a similar absolute clang reference?
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.
Does LLVM 14 fail with a similar absolute clang reference?
It does not. It works with an unqualified clang++
or a bin/"clang++"
.
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.
Error looks related to libc++
headers.
macOS build fails with
|
Linux fails at building OpenMP with:
|
80a4c56
to
08f39a8
Compare
Formula/llvm.rb
Outdated
|
||
if OS.mac? | ||
args << "-DLLVM_BUILD_LLVM_C_DYLIB=ON" | ||
args << "-DLLVM_ENABLE_LIBCXX=ON" | ||
args << "-DDEFAULT_SYSROOT=#{macos_sdk}" if macos_sdk | ||
runtimes_cmake_args << "-DCMAKE_INSTALL_RPATH=#{rpath}" | ||
# Disable builds for OSes not supported by the CLT SDK. | ||
runtimes_cmake_args += %w[I WATCH TV].map { |os| "-DCOMPILER_RT_ENABLE_#{os}OS=OFF" } |
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 WATCH TV
😄
What's changed here though to need this?
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.
This llvm/llvm-project@a680c21 seems to lead to #106925 (comment).
(Yes, I chuckled at I WATCH TV
too. Though really only the I
is potentially needed right now.)
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.
Actually, I'm not sure why this is failing. find_darwin_sdk_version
runs xcrun --sdk iphonesimulator --show-sdk-version
and checks that it returns 0
. I tried this on an ARM runner and it seems to work.
Is it because we set DEVELOPER_DIR
or something?
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 suppose we could just require Xcode to build this again...
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 think you're correct. It'll probably be an issue for CLT-only systems too.
I suppose we could just require Xcode to build this again...
I'd rather not.
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.
You probably won't be able to submit apps for those platforms when built with Homebrew LLVM so I don't think this is an issue. Was just mostly interested why it broke.
Homebrew is also a macOS package manager and offer no iOS support in anything else we ship.
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.
You probably won't be able to submit apps for those platforms when built with Homebrew LLVM so I don't think this is an issue. Was just mostly interested why it broke.
Maybe not, but it might be nice to have access to sanitisers, etc, for debugging.
Homebrew is also a macOS package manager and offer no iOS support in anything else we ship.
Can we force this to use the Xcode SDK if it's available but fall back to the CLT and disable iOS builds if not?
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.
We could but it's tricky to selectively apply it to just iOS etc SDKs.
Setting DARWIN_iphonesimulator_OVERRIDE_SDK_VERSION
(etc) is possible however. It seems like before it might have just auto-disabled if the SDK is not available (maybe? - need to check) so this feels like a bit of a regression anyway and I don't think it should be a fatal error. Maybe worth comparing the bottle with a CLT-only build at some point.
ARM Monterey failures:
ARM Big Sur failures are similar. Every linkage failure I looked at is for |
macOS failures all look pretty similar, except Catalina, which failed to build. |
This is no longer needed after Homebrew#106925.
This is no longer needed after Homebrew#106925.
This is no longer needed after Homebrew#106925.
This is no longer needed after Homebrew#106925.
This is no longer needed after Homebrew#106925.
This is no longer needed after Homebrew#106925.
This is no longer needed after Homebrew#106925.
This is no longer needed after Homebrew#106925.
We don't need to carry around this build-time resource anymore. The issue that required this has been fixed in Homebrew#106925.
This is no longer needed after #106925.
This is no longer needed after #106925.
This is no longer needed after #106925.
This is no longer needed after #106925.
This is no longer needed after #106925.
This is no longer needed after #106925.
This is no longer needed after #106925.
This is no longer needed after #106925.
This is no longer needed after #106925.
This is no longer needed after #106925.
This is no longer needed after #106925.
This is no longer needed after #106925.
This is no longer needed after #106925.
We don't need to carry around this build-time resource anymore. The issue that required this has been fixed in #106925.
In Homebrew#106925, I enabled LTO for our LLVM build. This creates static archives that contain LLVM bitcode instead of object code. This makes the static archives more difficult to use, requiring workarounds such as https://github.com/Homebrew/homebrew-core/blob/c01f1794fc3decce04b71cae03966213fc7af34d/Formula/enzyme.rb#L30 and has caused problems for multiple downstream projects. We can fix this by converting the bitcode into object code, which is what Fedora does with their LLVM build. They also build their toolchain with LTO. Alternatively, we can disable LTO, but that foregoes significant speedups we get from enabling it. While we're here, let's add some test coverage for features that were recently enabled that we don't test. Fixes: ziglang/zig#12923 halide/Halide#7055 mesonbuild/meson#10879 Homebrew/discussions#3666
In #106925, I enabled LTO for our LLVM build. This creates static archives that contain LLVM bitcode instead of object code. This makes the static archives more difficult to use, requiring workarounds such as https://github.com/Homebrew/homebrew-core/blob/c01f1794fc3decce04b71cae03966213fc7af34d/Formula/enzyme.rb#L30 and has caused problems for multiple downstream projects. We can fix this by converting the bitcode into object code, which is what Fedora does with their LLVM build. They also build their toolchain with LTO. Alternatively, we can disable LTO, but that foregoes significant speedups we get from enabling it. While we're here, let's add some test coverage for features that were recently enabled that we don't test. Fixes: ziglang/zig#12923 halide/Halide#7055 mesonbuild/meson#10879 Homebrew/discussions#3666 Closes #112154. Signed-off-by: Sean Molenaar <1484494+SMillerDev@users.noreply.github.com> Signed-off-by: BrewTestBot <1589480+BrewTestBot@users.noreply.github.com>
brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew test <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingbrew install --build-from-source <formula>
)? If this is a new formula, does it passbrew audit --new <formula>
?The last version bump took ages, so I'd like to get started early to try to get it done faster this time.
Things I'd like to fix along the way:
libc++
and the bundledlibc++
libLLVM
andlibclang
I have ideas for how to fix the above two, but I've written these issues down here so I don't forget. Nevertheless, suggestions on how to fix them welcome.