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@6: Fix C++ compilation for Mojave and Catalina #45693

Closed
wants to merge 1 commit into from
Closed

llvm@6: Fix C++ compilation for Mojave and Catalina #45693

wants to merge 1 commit into from

Conversation

jamiesnape
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • 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 <formula>)?

Revises tests to be closer to real-world usage and add extra tests to verify some of the bundled tools work.

Relates #45061 and #45304. FYI @imbsky.

@zbeekman
Copy link
Contributor

zbeekman commented Oct 28, 2019

@jamiesnape: could you please walk us through the rationale for all of the test changes? That would make reviewing this a bit easier.

EDIT: OK, I may just be a bit tired... I think I mostly follow the test logic. Anyway, explicitly discussing each change to an old test would be helpful if you find the time.

@@ -109,6 +109,11 @@ def install
-DLLVM_CREATE_XCODE_TOOLCHAIN=ON
]

if MacOS.version >= :mojave
sdk_path = MacOS::CLT.installed? ? "/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk" : MacOS.sdk_path
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it should be updated in homebrew/brew itself rather than this formula. What makes llvm@6 need this as a special case vs other formulae?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure many other formulae would need such logic unless they are compilers. We need an explicit SDK path so the compiler that is built by the formula (versus the compiler that is building the formula) knows where to find the SDK in future. But, yes, it would be useful logic (if MacOS::CLT.installed? == False is truly a supported configuration).

@jamiesnape
Copy link
Contributor Author

jamiesnape commented Oct 29, 2019

So as far as the tests go, there are different configurations to be tested, depending on the location of the SDK that is being used by the compiler. It can explicitly use the SDK from the command line tools, the SDK from Xcode, or it can use whatever its default SDK is, which depends on whether it was built on a system that had the command line tools installed.

There are also different libc++ libraries. There is the one built by the formula and the one bundled with Xcode and the command line tools.

LLVM also bundles various tools. I am testing scan-build because it has special logic in the formula, and clang-format as an example of a Clang tool (that would fail if the formula were misconfigured).

(And I remove -nostdinc since removing all built-in headers and trying to reconstruct most of it is not something anyone would ever do, and you can override the necessary includes without it anyone.)

@simmonmt
Copy link

My apologies if I'm saying something that's common knowledge, but there's significant value to alignment between homebrew llvm's default search paths and those used by the Apple compiler beyond just Catalina compatibility.

Apple doesn't ship clang-tidy, for example, but homebrew does. It can be an exercise in frustration trying to use homebrew's clang-tidy with a compilation database created from Apple clang++ invocations because the compilation database (a list of compile commands with flags) won't include the implicit search paths the Apple clang++ uses. I'm seeing this on my Mac Pro still stuck on High Sierra.

Would it be possible to use this opportunity to align the search paths Apple vs homebrew across the board, rather than making a fix specific to Catalina?

@jamiesnape
Copy link
Contributor Author

That would need a patch to the source code.

@smorimoto
Copy link
Contributor

A discussion on them was already in #45304. The purpose of these PR is also there. It's a very long thread, but I recommend read it if you're interested.

@smorimoto
Copy link
Contributor

I think this PR is ready to merge.

@zbeekman
Copy link
Contributor

zbeekman commented Nov 1, 2019

EDIT: That was premature; the bottle was deleted from CI.... 😭

Thanks for your contribution to Homebrew! 🎉 🥇

Without awesome contributors like you, it would be impossible to maintain Homebrew to the high level of quality users have come to expect!!!

@zbeekman
Copy link
Contributor

zbeekman commented Nov 1, 2019

@BrewTestBot test this please!

Copy link
Contributor

@zbeekman zbeekman left a comment

Choose a reason for hiding this comment

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

LGTM

@SMillerDev SMillerDev added the ready to merge PR can be merged once CI is green label Nov 2, 2019
@smorimoto
Copy link
Contributor

👀

Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

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

👍

@chenrui333 chenrui333 closed this in 576c498 Nov 2, 2019
@smorimoto
Copy link
Contributor

👍

@jamiesnape jamiesnape deleted the llvm@6-sysroot branch November 6, 2019 20:41
@lock lock bot added the outdated PR was locked due to age label Jan 2, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 2, 2020
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 ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants