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 lld flang wasi-runtimes 19.1.3 #196094

Merged
merged 8 commits into from
Nov 1, 2024
Merged

Conversation

carlocab
Copy link
Member

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

  • llvm 19.1.3
  • lld 19.1.3
  • flang 19.1.3
  • wasi-runtimes 19.1.3

@carlocab carlocab added CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. long build Set a long timeout for formula testing CI-no-fail-fast-deps Continue dependent tests despite failing GitHub Actions matrix tests. and removed CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Oct 30, 2024
Formula/l/llvm.rb Outdated Show resolved Hide resolved
@carlocab carlocab force-pushed the llvm-lld-flang-wasi-runtimes branch 2 times, most recently from 77b8335 to 1e6a047 Compare October 30, 2024 15:35
carlocab added a commit to Homebrew/brew that referenced this pull request Oct 30, 2024
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 carlocab force-pushed the llvm-lld-flang-wasi-runtimes branch 2 times, most recently from bdf82d9 to 865984d Compare October 31, 2024 07:54
Formula/l/llvm.rb Outdated Show resolved Hide resolved
@carlocab carlocab force-pushed the llvm-lld-flang-wasi-runtimes branch 2 times, most recently from ef892c7 to c1f007e Compare October 31, 2024 08:21
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
Copy link
Member

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:

Suggested change
(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.

Copy link
Member

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.

Copy link
Member Author

@carlocab carlocab Oct 31, 2024

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.

Copy link
Member

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.

Copy link
Member Author

@carlocab carlocab Oct 31, 2024

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?

Copy link

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

Copy link
Member

@Bo98 Bo98 Nov 10, 2024

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?

Copy link

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.

Copy link
Member

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #197277 and #197278

Formula/l/llvm.rb Show resolved Hide resolved
Comment on lines 515 to 520
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
Copy link
Member

@Bo98 Bo98 Oct 31, 2024

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 to Library/Homebrew/os/mac/clang-config or something like that so that brew update will be all you need.
  • Ship this in a special clang-config formula or something that every LLVM formula uses.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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)

Copy link
Member Author

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
@saschasc
Copy link
Contributor

@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.

@carlocab
Copy link
Member Author

We don't have the CI for it I'm afraid, but the changes here are designed so that the :sonoma bottle works out of the box for :sequoia.

@carlocab carlocab added long dependent tests Set a long timeout for dependent testing CI-skip-recursive-dependents Pass --skip-recursive-dependents to brew test-bot. and removed long dependent tests Set a long timeout for dependent testing labels Nov 1, 2024
@carlocab
Copy link
Member Author

carlocab commented Nov 1, 2024

Punting on Xcode discussions (#196094 (comment)) and non postinstall handling (#196094 (comment)).

Copy link
Contributor

github-actions bot commented Nov 1, 2024

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-no-fail-fast-deps Continue dependent tests despite failing GitHub Actions matrix tests. CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. CI-skip-recursive-dependents Pass --skip-recursive-dependents to brew test-bot. long build Set a long timeout for formula testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants