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

Migrating to the MacOSX11.sdk symlink #10769

Merged
merged 3 commits into from Aug 11, 2021
Merged

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Mar 3, 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?

Not to be merged until CLT 12.5 stable has sufficiently rolled out - see below.

With thanks to Apple, CLT 12.5 beta 3 released a few hours ago now includes a MacOSX11.sdk symlink.

Big Sur SDKs have been a moving target, with SDKs renaming from MacOSX11.0.sdk to MacOSX11.1.sdk etc. This is a problem because some SDK paths are baked into some formulae. To combat issues like Homebrew/homebrew-core#67075, we moved to the unversioned MacOSX.sdk as it was the only option for path stability.

However MacOSX.sdk itself will cause problems come macOS 12/CLT 13. Based on how all major CLT releases have gone in the past few years, MacOSX.sdk in CLT 13 will contain the macOS 12 SDK, even when installed on Big Sur. Using the macOS 12 SDK on Big Sur could cause problems when building against system libraries. The exact nature of the problems depends highly on what library changes are made in macOS 12, but you can read #7134 for an overview of how using the Catalina SDK caused significant problems on Mojave, such as brewed git crashing.

The MacOSX11.sdk symlink will solve all the above problems. It has path stability and is guaranteed to contain the Big Sur SDK (we have brew doctor checks for SDK tampering).

This change however will require users to have updated to CLT 12.5, when it is released. Once merged, our bottles will start to contain paths pointing to MacOSX11.sdk and this will not exist on earlier CLT versions. gcc, for example, will not function properly with an outdated CLT once we build new bottles with this SDK change. As such, this should not be merged until we reach a point where we are happy that most users, and CIs in particular, have updated to CLT 12.5. It should ideally however be merged no later than WWDC (likely CLT 13 beta release date), should we decide to be waiting that long. This will be an easier question to answer once we know exactly when CLT 12.5 stable is getting released.

Expected effects to the return value of the MacOS.sdk method (assuming latest versions of CLT & Xcode are installed):

  • Mojave:
    • CLT: no change (returns MacOSX10.14.sdk)
    • Xcode: no change (returns MacOSX10.15.sdk)
  • Catalina:
    • CLT: no change (returns MacOSX10.15.sdk)
    • Xcode: no change (returns MacOSX.sdk)
  • Big Sur:
    • CLT: returns MacOSX11.sdk (previously MacOSX.sdk)
    • Xcode: no change (returns MacOSX.sdk)

(Xcode only ever ships with one SDK, with a versioned symlink to that)

I've not fully verified the above results yet (I've not downloaded Xcode beta) but I will do before this is merged.

@BrewTestBot
Copy link
Member

Review period will end on 2021-03-04 at 04:37:51 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Mar 3, 2021
Comment on lines 355 to 368
def detect_version_from_clang_version
detect_clang_version&.sub(/^(\d+)00\./, "\\1.")
detect_clang_version&.sub(/^(\d+)0(\d)\./, "\\1.\\2.")
Copy link
Member Author

@Bo98 Bo98 Mar 3, 2021

Choose a reason for hiding this comment

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

I will likely split this into a separate PR as this affects CLT 12.5 support in general: the Clang version in the beta is currently 1205.0.22.6 which does not match the original regex.

Thankfully the Clang version sort of matches the CLT version here (this often isn't the case with many CLT updates), so we still only need one minimum_version value after this substitution tweak.

@MikeMcQuaid
Copy link
Member

Nice work @Bo98!

As such, this should not be merged until we reach a point where we are happy that most users, and CIs in particular, have updated to CLT 12.5. It should ideally however be merged no later than WWDC (likely CLT 13 beta release date), should we decide to be waiting that long. This will be an easier question to answer once we know exactly when CLT 12.5 stable is getting released.

We can enforce a minimum version of the CLT. We should do this when we merge, IMO.

Big Sur:

  • CLT: returns MacOSX11.sdk

Just to clarify: what's the before/after here?

@Bo98
Copy link
Member Author

Bo98 commented Mar 3, 2021

We can enforce a minimum version of the CLT. We should do this when we merge, IMO.

Yes, that's a part of this PR. It's just a question of when exactly to merge.

Just to clarify: what's the before/after here?

Edited the above to clarify this.

@Bo98
Copy link
Member Author

Bo98 commented Mar 3, 2021

(For example, it would be ideal to let the GitHub Actions macos-latest image update to CLT 12.5 before we enforce the new minimum.)

@MikeMcQuaid
Copy link
Member

  • CLT: returns MacOSX11.sdk (previously MacOSX.sdk)

👍🏻

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

Review period ended.

@BrewTestBot
Copy link
Member

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@BrewTestBot BrewTestBot added the stale No recent activity label Mar 26, 2021
@Bo98 Bo98 added in progress Maintainers are working on this and removed stale No recent activity labels Mar 26, 2021
@Bo98
Copy link
Member Author

Bo98 commented May 5, 2021

Update here: the GitHub Actions macos-11.0 image now has CLT 12.5.

I'll update our self-hosted runners by the end of the week, if I can find a quiet time where there's not a million massive PRs running at the same time.

@Bo98
Copy link
Member Author

Bo98 commented May 10, 2021

CI is now updated.

The brew doctor soft recommendation to update to 12.5 will be included in the 3.1.6 tag today (with the Perl stuff).

I suggest waiting a little time for that to settle and then merge this. I'm looking at brew config in bug reports and a lot are still on 12.4.

@carlocab
Copy link
Member

Do we have analytics data on which CLT/Xcode version users are using? If we don't, would it be useful to have it?

@MikeMcQuaid
Copy link
Member

Do we have analytics data on which CLT/Xcode version users are using?

No, don't think so.

If we don't, would it be useful to have it?

Maybe? I think we can be more aggressive about forcing upgrades when it suits us, though.

@MikeMcQuaid MikeMcQuaid removed the in progress Maintainers are working on this label Aug 11, 2021
@Bo98
Copy link
Member Author

Bo98 commented Aug 11, 2021

We're probably ok to merge this now (after I rebase)? There's surely enough people on CLT 12.5 now and we definitely need to get this merged before Monterey.

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Aug 11, 2021
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense to me, nice work @Bo98!

@MikeMcQuaid MikeMcQuaid merged commit bee3e67 into Homebrew:master Aug 11, 2021
@MikeMcQuaid
Copy link
Member

Thanks @Bo98!

@Bo98 Bo98 deleted the big-sur-sdk branch August 11, 2021 14:21
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 11, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants