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: make bottle compatible with Xcode-only installs #79666

Closed
wants to merge 1 commit into from

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 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 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

This patch implements various improvements I found while working on #77975.

A few are cosmetic, but the primary substantive change is the use of the
C_INCLUDE_DIRS CMake variable. [1]

This adds Homebrew's include directory along with Xcode's system
header path to Clang's default include search path. The former change
aligns our Clang with Apple's, which searches /usr/local/include by
default. [2] The latter change allows the bottle to be poured in
Xcode-only installs so that we no longer need the pour_bottle? check.

We also add /Library/Developer/CommandLineTools/usr/include to the
default header search path to align our build with Apple's.

[1] https://reviews.llvm.org/D69221
[2] From /usr/bin/clang -E -xc -v /dev/null:

Apple clang version 12.0.5 (clang-1205.0.22.9)
[snip]
#include "..." search starts here:
#include <...> search starts here:
 /usr/local/include
 /Library/Developer/CommandLineTools/usr/lib/clang/12.0.5/include
 /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include
 /Library/Developer/CommandLineTools/usr/include
 /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks (framework directory)
End of search list.
[snip]

This patch implements various improvements I found while working on Homebrew#77975.

A few are cosmetic, but the primary substantive change is the use of the
`C_INCLUDE_DIRS` CMake variable. [1]

This adds Homebrew's `include` directory along with Xcode's system
header path to Clang's default include search path. The former change
aligns our Clang with Apple's, which searches `/usr/local/include` by
default. [2] The latter change allows the bottle to be poured in
Xcode-only installs so that we no longer need the `pour_bottle?` check.

We also add `/Library/Developer/CommandLineTools/usr/include` to the
default header search path to align our build with Apple's.

[1] https://reviews.llvm.org/D69221
[2] From `/usr/bin/clang -E -xc -v /dev/null`:

    Apple clang version 12.0.5 (clang-1205.0.22.9)
    [snip]
    #include "..." search starts here:
    #include <...> search starts here:
     /usr/local/include
     /Library/Developer/CommandLineTools/usr/lib/clang/12.0.5/include
     /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include
     /Library/Developer/CommandLineTools/usr/include
     /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks (framework directory)
    End of search list.
    [snip]
@carlocab carlocab requested a review from Bo98 June 20, 2021 15:02
@BrewTestBot BrewTestBot added the python Python use is a significant feature of the PR or issue label Jun 20, 2021
@carlocab
Copy link
Member Author

For reference, with our bottled LLVM:

❯ ~/homebrew/opt/llvm/bin/clang -E -xc -v /dev/null
clang version 12.0.0
[snip]
#include "..." search starts here:
#include <...> search starts here:
 /Users/carlocab/homebrew/Cellar/llvm/12.0.0_1/lib/clang/12.0.0/include
 /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include
 /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks (framework directory)
End of search list.


sdk = MacOS.sdk_path_if_needed
args << "-DDEFAULT_SYSROOT=#{sdk}" if sdk

extra_includes = [HOMEBREW_PREFIX/"include"]
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm hoping this won't affect the bottle being cellar: :any, since this is stored in a text file. If it makes the bottle non-relocatable we can drop it.

Copy link
Member

Choose a reason for hiding this comment

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

C_INCLUDE_DIRS should be baked in to clang binaries so it will affect relocatability.

Copy link
Member Author

Choose a reason for hiding this comment

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

DEFAULT_SYSROOT isn't (at least, not in a way you can find with strings), so I was maybe a bit hopeful here.

@carlocab
Copy link
Member Author

Killing the tests while #79465 is running.

@danielnachun
Copy link
Member

Sorry to make you wait to test this! I'll wait until you've had a chance to run this before I push any more changes to the LLVM PRs I have open.

@carlocab
Copy link
Member Author

Oh, wait, this might not be a good idea. https://github.com/Homebrew/linuxbrew-core/pull/22742#issuecomment-810641119

Will need to look into this some more.

@carlocab carlocab marked this pull request as draft June 20, 2021 19:15
@Bo98
Copy link
Member

Bo98 commented Jun 21, 2021

The latter change allows the bottle to be poured in
Xcode-only installs so that we no longer need the pour_bottle? check.

C_INCLUDE_DIRS behaves differently to DEFAULT_SYSROOT. The latter can be overridden with -isysroot. The former only can with the combination of -nostdlibinc & -isystem. This is a significant change of behaviour for those who want to use a specified SDK.

I also think C++ stdlib header handling won't read C_INCLUDE_DIRS but will read DEFAULT_SYSROOT.

@Bo98
Copy link
Member

Bo98 commented Jun 21, 2021

Apple's way of handling CLT & Xcode does not lie within LLVM itself - it's with how /usr/bin/clang goes through xcrun, which sets the SDKROOT env.

@carlocab
Copy link
Member Author

Yup, realising that now. Though, xcrun seems to be doing more than just setting SDKROOT, since Apple Clang is also including /Library/Developer/CommandLineTools/usr/include.

@carlocab carlocab closed this Jul 6, 2021
@carlocab carlocab deleted the llvm-improvement branch July 6, 2021 12:13
@github-actions github-actions bot added the outdated PR was locked due to age label Aug 6, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age python Python use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants