llvm lld flang wasi-runtimes 19.1.3 #196094
Conversation
carlocab
force-pushed
the
llvm-lld-flang-wasi-runtimes
branch
from
4e9b983
to
dd13a6f
Compare
carlocab
added
CI-no-fail-fast
carlocab
force-pushed
the
llvm-lld-flang-wasi-runtimes
branch
2 times, most recently
from
77b8335
to
1e6a047
Compare
This will be used by `llvm` (and, presumably, in the future, versioned LLVM formulae). The idea is that we will write a config file for each OS version pointing to the correct SDKROOT so that `llvm` does not require rebuilding/reinstalling when a user upgrades to a new major version of macOS. See Homebrew/homebrew-core#196094.
carlocab
force-pushed
the
llvm-lld-flang-wasi-runtimes
branch
2 times, most recently
from
bdf82d9
to
865984d
Compare
Formula/l/llvm.rb
Outdated
| (clang_config_file_dir/"#{arch}-apple-darwin#{kernel_version}-#{driver}.cfg").atomic_write <<~CONFIG | ||
| --sysroot=#{MacOS::CLT::PKG_PATH}/SDKs/MacOSX#{macos_version}.sdk | ||
| CONFIG |
There was a problem hiding this comment.
Given that we hardcode the CLT here, this won't work at all for Xcode-only installs. Wonder if it's just worth dropping the pour_bottle? check and then let users install the CLT if they need to use clang.
Most users seem interested in the libraries anyway.
carlocab
force-pushed
the
llvm-lld-flang-wasi-runtimes
branch
2 times, most recently
from
ef892c7
to
c1f007e
Compare
Formula/l/llvm.rb
Outdated
| arches.each do |target_arch| | ||
| target_triple = "#{target_arch}-apple-darwin#{kernel_version}" | ||
| drivers.each do |driver| | ||
| (clang_config_file_dir/"#{target_triple}-#{driver}.cfg").atomic_write <<~CONFIG |
There was a problem hiding this comment.
If you're using the clang-cl patch then you should be able to just do:
| (clang_config_file_dir/"#{target_triple}-#{driver}.cfg").atomic_write <<~CONFIG | |
| (clang_config_file_dir/"#{target_triple}.cfg").atomic_write <<~CONFIG |
or at least that's kinda why I did the clang-cl patch.
There was a problem hiding this comment.
Consider also deploying a #{arch}-apple-darwin.cfg, which will be be a fallback for unsupported versions, which uses the unversioned --sysroot=#{MacOS::CLT::PKG_PATH}/SDKs/MacOSX.sdk. Hopefully nobody needs it but I think it makes sense to do.
There was a problem hiding this comment.
Right, I forgot about why we had the clang-cl patch.
Consider also deploying a
#{arch}-apple-darwin.cfg, which will be be a fallback for unsupported versions, which uses the unversioned--sysroot=#{MacOS::CLT::PKG_PATH}/SDKs/MacOSX.sdk. Hopefully nobody needs it but I think it makes sense to do.
What about Xcode-only installs? We also don't handle it currently, and this change breaks them, even when building from source, but not really sure what to do about it.
There was a problem hiding this comment.
It's realistically difficult/impossible to support Xcode without having a libxcselect wrapper. Apple don't even manage it without that.
$ echo | /Library/Developer/CommandLineTools/usr/bin/clang -E -Wp,-v -
clang -cc1 version 16.0.0 (clang-1600.0.26.3) default target arm64-apple-darwin23.6.0
ignoring nonexistent directory "/usr/local/include"
ignoring nonexistent directory "/usr/include"
#include "..." search starts here:
#include <...> search starts here:
/Library/Developer/CommandLineTools/usr/lib/clang/16/include
/Library/Developer/CommandLineTools/usr/include
/System/Library/Frameworks (framework directory)
/Library/Frameworks (framework directory)
End of search list.
$ echo | xcrun /Library/Developer/CommandLineTools/usr/bin/clang -E -Wp,-v -
clang -cc1 version 16.0.0 (clang-1600.0.26.3) default target arm64-apple-darwin23.6.0
ignoring nonexistent directory "/usr/local/include"
ignoring nonexistent directory "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/local/include"
ignoring nonexistent directory "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/Library/Frameworks"
#include "..." search starts here:
#include <...> search starts here:
/Library/Developer/CommandLineTools/usr/lib/clang/16/include
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include
/Library/Developer/CommandLineTools/usr/include
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks (framework directory)
End of search list.Some people to install the latest Xcode as /Applications/Xcode-beta.app (particularly during beta cycles), some install it in ~/Applications instead, etc.
Even the existing solution here doesn't support that anyway so not sure what you mean by "breaking" it.
There was a problem hiding this comment.
Well by "breaking" it, I mean that a user with an Xcode-only install at least has a chance of getting a somewhat-sensible DEFAULT_SYSROOT which comes from MacOS.sdk_path_if_needed if they build from source:
❯ sudo mv /Library/Developer/CommandLineTools .
❯ brew ruby -e 'puts MacOS.sdk_path_if_needed'
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.sdk
Now everyone for whom this worked for when they built llvm from source will still get a toolchain that refers to a non-existent path for the sysroot.
Should we just odie inside install if the CLT is not installed?
There was a problem hiding this comment.
Seems to work even if Xcode.app is moved:
❯ sudo mv /Applications/Xcode.app /Applications/Xcode_15.app
❯ brew ruby -e 'puts MacOS.sdk_path_if_needed'
/Applications/Xcode_15.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.sdk
But maybe calling it Xcode_15.app helps.
| { | ||
| 11 => 20, | ||
| 12 => 21, | ||
| 13 => 22, | ||
| 14 => 23, | ||
| 15 => 24, | ||
| }.each do |macos_version, kernel_version| | ||
| write_config_files(macos_version, kernel_version, Hardware::CPU.arch) | ||
| end |
There was a problem hiding this comment.
Do we already know this formula doesn't work on 10.14 and 10.15? Also would be good to iterate over https://github.com/Homebrew/brew/blob/master/Library/Homebrew/macos_version.rb#L21-L32 automatically.
There was a problem hiding this comment.
Do we already know this formula doesn't work on 10.14 and 10.15?
No, but 10.14 doesn't need a config file IIRC.
Also would be good to iterate over Homebrew/brew@
master/Library/Homebrew/macos_version.rb#L21-L32 automatically.
Yes, but that needs Homebrew/brew@42dd0ac which isn't in a release tag yet.
There was a problem hiding this comment.
No, but 10.14 doesn't need a config file IIRC.
10.14 is the first version without /usr/include (unless you install the header pkg but we do not support/require that). https://github.com/Homebrew/brew/blob/f18e1ea86ab32a8d6119f2d1ae0c6dfcdc77395c/Library/Homebrew/os/mac/xcode.rb#L284
Formula/l/llvm.rb
Outdated
| def post_install | ||
| return unless OS.mac? | ||
| # TODO: should we check for all config files here? | ||
| return if (clang_config_file_dir/"#{Hardware::CPU.arch}-apple-darwin#{OS.kernel_version.major}-clang.cfg").exist? | ||
|
|
||
| write_config_files(MacOS.version.major, OS.kernel_version.major, Hardware::CPU.arch) | ||
| end |
There was a problem hiding this comment.
Was initially wondering if this was really needed given we will pick up new macOS versions 3+ months prior to release but I guess makes sense for the versioned formulae that won't be version bumped.
Feels hacky to make people do brew postinstall though - brew upgrade is much more intuitive. I wonder if it makes sense to either:
- Ship these files statically in
brewand point the config directory toLibrary/Homebrew/os/mac/clang-configor something like that so thatbrew updatewill be all you need. - Ship this in a special
clang-configformula or something that every LLVM formula uses.
There was a problem hiding this comment.
Well the idea of postinstall was for users who didn't get a default config file for whatever reason. The idea is that it's meant to be a rare occurrence, so doing a lot more about it (e.g. files in brew, a separate formula) seems overkill.
There was a problem hiding this comment.
Isn't the point to avoid people needing to avoid brew reinstall? It would indeed probably be a rare occurrence for main formula (though rare enough to probably be covered by the unversioned case), but not for versioned formulae which are rarely revision bumped.
There was a problem hiding this comment.
A versioned formula carrying these changes is still several months away -- I don't think that's worth considering right now. We can handle those properly when it comes to it.
The bigger issue is Xcode-only installs, which will get broken as soon as this is merged: #196094 (comment)
Also, backport some changes that will allow us to use config files.
Also: - avoid referencing LLVM cellar paths in symlinks - fix some build flags - add config files to simplify usage with our clang
carlocab
force-pushed
the
llvm-lld-flang-wasi-runtimes
branch
from
c1f007e
to
f347d81
Compare
|
@carlocab I've seen that there are no bottles for Sequoia Intel (only arm64_sequoia). Would it be possible to include them as well from 19.1.3 and upcoming? This would be awesome. |
|
We don't have the CI for it I'm afraid, but the changes here are designed so that the |
carlocab
added
long dependent tests
HOMEBREW_NO_INSTALL_FROM_API=1 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 doingHOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it passbrew audit --new <formula>?