-
-
Notifications
You must be signed in to change notification settings - Fork 12.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
llvm lld flang wasi-runtimes 19.1.3 #196094
Conversation
4e9b983
to
dd13a6f
Compare
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.
bdf82d9
to
865984d
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the answers! I'll open a separate issue about -syslibroot
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.
Would also be great to also hear more on what people on Xcode-only installs think given the CLT soft requirement has been there for a while. Has brew install --build-from-source llvm
been a good experience? How often do you upgrade? Is there a reason why building LLVM from source is seen as a better option to installing the CLT?
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.
Sorry, cannot answer those questions, I'm acting here because of rust-lang/cc-rs#1278 and rust-lang/rust#131477, and hence it is somewhat important to me that Homebrew gets the details here right, but I might have been completely mistaken in certain regards (again, sorry!) because I haven't used Homebrew myself for a while.
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.
Ah good point about missing *-apple-macosx
- that's definitely worth fixing. I remembered about them initially but completely forgot when this PR was opened.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
brew
and point the config directory toLibrary/Homebrew/os/mac/clang-config
or something like that so thatbrew update
will be all you need. - Ship this in a special
clang-config
formula or something that every LLVM formula uses.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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.
Will fix this before LLVM 20, but would love more thoughts on the other thread about Xcode-only installs.
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
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 |
Punting on Xcode discussions (#196094 (comment)) and non |
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>
?