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

Fix SDK issues on Mojave and High Sierra #7134

Merged
merged 3 commits into from
Mar 9, 2020
Merged

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Mar 8, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

The amount of workarounds is getting ridiculous. Time to tackle the core of the problem.

Mojave (and likely Catalina come Xcode 12)

After the removal of /usr/include, Apple directed people to use the SDK. However by default, Homebrew uses the latest SDK. This is wrong when dealing with system headers.

For example, Git compiled against the system curl headers. On Mojave under Xcode 11, Homebrew would pick the 10.15 SDK. These curl headers are newer so the LIBCURL_VERSION_NUM was higher. This triggered Git to enable some features only available on newer curl. This compiled fine, but would fail at runtime because Mojave's curl library doesn't support the features Git was trying to use.

Instead I changed it so to always prioritise matching the SDK with the OS version. In the event this is not found (which is the case for 10.10-10.13), then the rescue BaseSDKLocator::NoSDKError covers that already and returns the latest SDK.

However, this doesn't quite solve the problem under all setups.

Xcode 11 on Mojave does NOT ship with the 10.14 SDK - only the CLT does. Because we prioritise Xcode SDK searching over the CLT, it will never find the 10.14 SDK. I've changed it now to search the CLT first. This is a notable change, but I can't see any reason why it would be problematic on older systems.

These changes also fixes significant GCC issues. GCC is compiled with various patches to the SDK. This does not work if the SDK used does not match what it was compiled with. We currently compile with the SDK matching the OS as it causes the least issues for those using GCC outside of the Homebrew environment, and actually matches the behaviour of Clang.

There is one outstanding issue: Ruby. This is arguably an Apple bug. gem does not work unless SDKROOT is set to the correct SDK matching the OS. We no longer set SDKROT in the superenv as of mid-2018. That was an area that I wanted to avoid touching as it certainly seems like something that could potentially trigger issues with a few formulae. I'm not sure of a nice way to solve this unless we made a gem shim or something.

High Sierra

Additionally, there were a few issues on High Sierra, albeit less frequent. This was caused by the logic of MacOS::CLT.separate_header_package?. The naming is somewhat legacy as the "separate header package" was just some compatibilty crossover period and is no longer supplied. What this really was being used for was to see whether /usr/include existed - and thus affect whether the -isysroot would be set to the SDK or not. If it was false and the CLT was installed, no SDK would be inserted.

This check was erroneous on High Sierra with CLT 10 installed. The check compared whether the CLT was version >= 10, but did not consider the fact that the system header situation only applies to Mojave and not High Sierra. This meant that both /usr/include and the equivalent in the 10.14 SDK was in the search path. Usually, /usr/include takes priority but there was some cases where the SDK was used.

The fix was to correct MacOS::CLT.separate_header_package? to also consider the OS version.

@MikeMcQuaid
Copy link
Member

There is one outstanding issue: Ruby. This is arguably an Apple bug. gem does not work unless SDKROOT is set to the correct SDK matching the OS. We no longer set SDKROT in the superenv as of mid-2018. That was an area that I wanted to avoid touching as it certainly seems like something that could potentially trigger issues with a few formulae. I'm not sure of a nice way to solve this unless we made a gem shim or something.

Does this mean the ruby formula, all ruby and ruby@ formulae, the system gem, rbenv stuff or more or less or what? 😂


The PR changes here make sense to me. What do they mean for current formulae workarounds? Are they no longer necessary and can be removed? Will they break stuff until they are changed?

Basically: agree with the code as-is here but just have questions about rollout and formula changes.

Thanks again @Bo98!

@Bo98
Copy link
Member Author

Bo98 commented Mar 9, 2020

Does this mean the ruby formula, all ruby and ruby@ formulae, the system gem, rbenv stuff or more or less or what? 😂

Sorry, I mean invoking system gem. For example Cocoapods: https://github.com/Homebrew/homebrew-core/blob/3aa801262681ded2cb587cbda56a4dfd57b47a19/Formula/cocoapods.rb#L21. It's really Apple's bug, but one that's probably not going to be fixed, on Mojave anyway. The exact same happens outside the Homebrew environment with gem install cocoapods, but SDKROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk gem install cocoapods works.

Building native extensions.  This could take a while...
ERROR:  Error installing cocoapods-1.9.0.gem:
	ERROR: Failed to build gem native extension.

    current directory: /usr/local/Cellar/cocoapods/1.9.0/libexec/gems/ffi-1.12.2/ext/ffi_c
/System/Library/Frameworks/Ruby.framework/Versions/2.3/usr/bin/ruby -r ./siteconf20200229-74843-12oz9d8.rb extconf.rb
mkmf.rb can't find header files for ruby at /System/Library/Frameworks/Ruby.framework/Versions/2.3/usr/lib/ruby/include/ruby.h

extconf failed, exit code 1

Are they no longer necessary and can be removed?

Yes.

Will they break stuff until they are changed?

No. The workarounds are overriding what we are changing here. They just will have nil effect after this change as they will be overwriting HOMEBREW_SDKROOT with what it should already be set to.

@MikeMcQuaid
Copy link
Member

The exact same happens outside the Homebrew environment with gem install cocoapods

Ok, cool.

SDKROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk gem install cocoapods works.

How about we just manually set the SDKROOT in the gem-using formulae that need it? Alternatively, the gem shim sounds like a good call if it'd be needed by 5+ formulae.

No. The workarounds are overriding what we are changing here. They just will have nil effect after this change as they will be overwriting HOMEBREW_SDKROOT with what it should already be set to.

🎉

@Bo98
Copy link
Member Author

Bo98 commented Mar 9, 2020

How about we just manually set the SDKROOT in the gem-using formulae that need it? Alternatively, the gem shim sounds like a good call if it'd be needed by 5+ formulae.

Yeah that seems like the best idea. There's currently workarounds in about 2-3, but that just means 2-3 have been updated since October. I'll keep an eye out and see if that number increases.

@MikeMcQuaid MikeMcQuaid merged commit 82f5b51 into Homebrew:master Mar 9, 2020
@MikeMcQuaid
Copy link
Member

Nice work again @Bo98! Reminder to wait for a new tag before merging any PRs removing these workarounds 🎉.

@Bo98 Bo98 deleted the sdk branch March 9, 2020 19:58
@lock lock bot added the outdated PR was locked due to age label Apr 15, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants