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

shims/mac/super: add ruby shims to set SDKROOT #10595

Merged
merged 1 commit into from
Feb 26, 2021
Merged

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Feb 11, 2021

  • 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 typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

I think we're past the 5+ formulae threshold now.

System gem on Mojave is broken and can't build native extensions without SDKROOT.

As evidenced by #9410, the bug appears to have now spread to Catalina.

@BrewTestBot
Copy link
Member

Review period will end on 2021-02-12 at 16:39:07 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 11, 2021
@MikeMcQuaid
Copy link
Member

Is this Xcode or CLT specific?

@Bo98
Copy link
Member Author

Bo98 commented Feb 11, 2021

Is this Xcode or CLT specific?

We mandate the CLT on older OS versions, and you indeed will need the CLT to get the correct SDK.

In terms of the bug, if xcode-select -p is:

  • Xcode, then it'll point to the 10.15 SDK on Mojave or the 11 SDK on Catalina. Older SDKs are not available under Xcode.
  • CLT, then it'll point to /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk which will symlink to the latest SDK - not the one matching the OS (so 10.15 SDK on Mojave, 11 SDK on Catalina).

Using a newer SDK is wrong as Ruby headers will be for a newer Darwin version (11 SDK has Ruby headers for darwin20 but Catalina is darwin19; and even worse 10.15 SDK has Ruby 2.6 headers while 10.14 uses Ruby 2.3).

The shims will point system Ruby to /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk and /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk for Mojave and Catalina respectively, and an effective no-op on Big Sur (until CLT 13 inevitably breaks things again, in which case this shim will fix it).

@MikeMcQuaid
Copy link
Member

We mandate the CLT on older OS versions, and you indeed will need the CLT to get the correct SDK.

Sorry, I mean: is this still an issue on those older OS versions which don't have Xcode installed at all?

The shims will point system Ruby to /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk and /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk for Mojave and Catalina respectively, and an effective no-op on Big Sur (until CLT 13 inevitably breaks things again, in which case this shim will fix it).

👍🏻

@Bo98
Copy link
Member Author

Bo98 commented Feb 11, 2021

is this still an issue on those older OS versions which don't have Xcode installed at all?

Ah yes, it depends on xcode-select -p. I've updated my previous comment on that. It's a problem either way.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 12, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@Bo98 Bo98 force-pushed the ruby-shims branch 3 times, most recently from 5a26738 to 782ac20 Compare February 13, 2021 03:37
Comment on lines -250 to +279
Library/Homebrew/shims/scm/svn --homebrew=print-path
brew sh -c "svn --homebrew=print-path"
Copy link
Member Author

Choose a reason for hiding this comment

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

The previous behaviour, to my knowledge, is not supported? I hope no one relied on being able to invoke a shim outside of the brew environment.

Copy link
Member

Choose a reason for hiding this comment

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

Given that we were in our CI environment: I don't think we should break this. At least it'd be good to attempt to infer HOMEBREW_LIBRARY if it's not specified based on the shim location?

Copy link
Member Author

@Bo98 Bo98 Feb 15, 2021

Choose a reason for hiding this comment

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

Tricky because of the different directory levels. Probably would need to go through levels and search for a specific file/folder.

Note that any HOMEBREW_MACOS_VERSION_NUMERIC check in this file would have also not worked, and never has when called this way. So I'd need to cover that too if we want to support this.

Copy link
Member

Choose a reason for hiding this comment

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

Fine with me then. Can we try to produce a human-readable error when this isn't defined? Also, I reckon hold off until after 3.0.2 is tagged so we can get a bit of wider testing time with this before a new tag.

@MikeMcQuaid
Copy link
Member

@Bo98 Good to 🚢 now if you are.

@Bo98 Bo98 merged commit 90e32b2 into Homebrew:master Feb 26, 2021
@Bo98 Bo98 deleted the ruby-shims branch February 26, 2021 11:45
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 29, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 29, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants