sdk: use unversioned SDK path on Big Sur #10072

carlocab · 2020-12-20T14:23:19Z

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?
- This returned an error regarding Language::Java::java_home
Have you successfully run brew man locally and committed any changes?

Partial fix to Homebrew/homebrew-core#67075.

This implements @fxcoudert's suggestion in the issue linked above:

If Apple is going to make SDKs depend on minor macOS version, then it could make sense for us to use MacOSX.sdk as MacOS.sdk_path_if_needed.

Before:

❯ brew ruby -e 'puts MacOS.sdk_path_if_needed'
/Library/Developer/CommandLineTools/SDKs/MacOSX11.1.sdk

After:

❯ brew ruby -e 'puts MacOS.sdk_path_if_needed'
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk

BrewTestBot · 2020-12-20T14:23:33Z

Review period will end on 2020-12-22 at 00:00:00 UTC.

fxcoudert · 2020-12-20T17:25:01Z

Many thanks @carlocab for all your recent contributions in homebrew-core, and for getting the ball rolling on the one. I don't know that part of the code well-enough to understand if this is the right approach or if it could break something, but I think it's definitely the right result.

MikeMcQuaid · 2020-12-21T08:08:06Z

Library/Homebrew/os/mac/sdk.rb

@@ -91,6 +91,12 @@ def sdk_paths
              paths[OS::Mac::Version.new(version)] = sdk_path if version.present?
            end

+            if OS::Mac.version >= :big_sur


Should/could there be any logic put or changed in sdk_if_applicable too? I just note that that already references :big_sur. Additionally, a comment would be good here to note why this is being done.

Should/could there be any logic put or changed in sdk_if_applicable too

I'll have a think about it. Did you have anything in particular in mind?

a comment would be good here to note why this is being done

I've been thinking exactly that too. I'll put one in.

I'll have a think about it. Did you have anything in particular in mind?

Nothing specifically! Just wondering if the logic should/could be combined. If you or others think "nope!": fine with me.

Hmmm, no, I think it's fine to keep it. If I were to make a change to it, I would actually just remove the line involving the :big_sur check

brew/Library/Homebrew/os/mac/sdk.rb

Lines 57 to 64 in 993bf95

# Accept an SDK for another OS version if it shares a major version

# with the current OS - for example, the 11.0 SDK on 11.1,

# or vice versa.

# Note that this only applies on macOS 11

# or greater, given the way the versioning has changed.

# This shortcuts the below check, since we *do* accept an older version

# on macOS 11 or greater if the major version matches.

return sdk if OS::Mac.version >= :big_sur && sdk.version.major == OS::Mac.version.major

as I think it's essentially a no-op after this PR.

However, this is under the assumption that sdk_prefix always contains a MacOSX.sdk that points to the right version. We'd need to put it back if this stops being true, or remove/modify the change in this PR (and possibly elsewhere) if MacOSX.sdk suddenly starts pointing to an SDK that we don't want.

There are probably other ways to combine the logic, but I don't see a clean way of doing it right now. I'd need to invest more time in understanding the code to figure it out.

Given the uncertainty regarding the contents of sdk_prefix, I think it's best to keep the rest of the code as-is for now. We can revisit it when we're a few minor upgrades into Big Sur and have a better understanding of how Apple wants to do things.

sdk_prefix always contains a MacOSX.sdk that points to the right version

I was wondering about that. I don't know if there are versions of Xcode/CLT for which it could be untrue.

However, this is under the assumption that sdk_prefix always contains a MacOSX.sdk that points to the right version. We'd need to put it back if this stops being true, or remove/modify the change in this PR (and possibly elsewhere) if MacOSX.sdk suddenly starts pointing to an SDK that we don't want.

Could this be a fallback so if it isn't set at all we set it to this logic here?

I'd be tempted to remove this logic if it becomes a no-op after this but hopefully @mistydemeo will be able to chime in with thoughts.

I'll merge this as-is or in some form tomorrow when other maintainers have (hopefully) had the chance to take a look.

Thanks @carlocab!

it becomes a no-op

To be clear, I think it (the pre-existing :big_sur check) will do two things when this PR is merged:

skips the version check in the succeeding line; and,

handles the case where MacOSX.sdk disappears for some reason and there is a minor-version mismatch in Big Sur's SDK.

But, yes, I would also be interested in other opinions on this to be sure.

2. handles the case where MacOSX.sdk disappears for some reason

Unless we've seen cases where it isn't present: I think it's worth the assumption that this won't happen on Big Sur and above. Thoughts?

I am inclined to agree, but I'll admit I haven't exactly been paying attention to the contents of sdk_prefix. I also avoid installing Xcode like the plague, so I know even less about sdk_prefix for Xcode.

@Bo98 any thoughts on any of this?

fxcoudert

I am 👍 on the idea, cannot specifically okay the details because I don't know that part of brew well enough

@fxcoudert

Fixes Homebrew/homebrew-core#67075. This implements @fxcoudert's suggestion in the issue linked above.

BrewTestBot · 2020-12-22T00:20:11Z

Review period ended.

MikeMcQuaid · 2020-12-22T08:10:53Z

Merging as-is but would love help and input from @mistydemeo and @Bo98 on how best to craft a follow-up PR to improve this (and any other missing edge-cases here). Thanks all!

carlocab · 2020-12-22T08:14:05Z

Thanks @MikeMcQuaid. I'll be watching homebrew-core and the discussions to see if any problems show up, but feel free to tag me if I miss any.

Bo98 · 2020-12-23T03:34:55Z

Library/Homebrew/os/mac/sdk.rb

+              sdk_path = File.join(sdk_prefix, "MacOSX.sdk")
+              version = OS::Mac.full_version
+              paths[version] = sdk_path if File.directory?(sdk_path)


I think this should be OS::Mac.sdk_version rather than full_version or there will be problems should there be a 11.1.1.

But I actually think the best solution here is to not use the runtime MacOS version at all. That's open to edge cases, particularly with outdated CLT installs. This strongly assumes the user is running a CLT/Xcode version released around the same time as their macOS version. This can very easily not be the case. In the case of major macOS versions, we absolutely do not want old SDKs being seen as compatible with the latest OS - we even have brew doctor checks that go through the SDK versions detected here.

I strongly suggest either:

Reading the symlink and detecting the SDK version from the real path, or

Reading the plist in the SDK and extracting the version from that

This should also alleviate some of the concerns in the earlier review.

I think this should be OS::Mac.sdk_version rather than full_version or there will be problems should there be a 11.1.1.

Yes, I agree.

But I actually think the best solution here is to not use the runtime MacOS version at all. That's open to edge cases, particularly with outdated CLT installs.

Aren't outdated CLT installs already picked up by brew doctor? I should also mention that the Big Sur 11.1 update actually wiped out my CLT installation. If Apple keeps doing that, then this may not be too significant a problem. (Big "if", I know.)

we absolutely do not want old SDKs being seen as compatible with the latest OS

It seems we already started being OK with that in the case of Big Sur though. We also have other code that allows for minor-version mismatches in the SDK. I agree that we do not want, say, MacOSX10.14.sdk being picked up on Big Sur, but I'd need to think more carefully about circumstances when this might happen but isn't caught otherwise.

we even have brew doctor checks that go through the SDK versions detected here.

I was concerned about this too, which is why I had a look before this PR was merged at brew doctor to see if this change would affect its CLT version detection information. I don't believe it does, but I'd love a second opinion on this.

Reading the symlink and detecting the SDK version from the real path

This only works for CLT. For Xcode, it is MacOSX11.1.sdk that is the symlink to MacOSX.sdk, and not vice-versa. I guess we could make it work by having two different implementations of sdk_paths in XcodeSDKLocator and CLTSDKLocator, but I suspect that is not a good solution.

Reading the plist in the SDK and extracting the version from that

I'll have a look at this one.

It seems /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk contains (among others) SDKSettings.plist and SDKSettings.json. It appears that the contents of the two are identical. In particular, both have an entry whose key is "Version" that we can use.

@fxcoudert, can you confirm the same for Xcode?

@carlocab Confirmed:

$ cat /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/SDKSettings.json | jq .Version "11.0"

@carlocab Would you be able to create a follow-up PR given the feedback above? Thanks!

Thanks, @MikeMcQuaid. On it.

Only extra thing to remember is that the 10.15 SDK might(?) have 3 components in the version so might need to strip that off to be just 2. I know a 10.15.4 SDK was released at some point.

Aren't outdated CLT installs already picked up by brew doctor?

Only after changes are made in brew. For example, when the Big Sur betas came out, people were already trying it before brew got Big Sur support and reported cryptic errors caused by outdated CLTs. So we added a general check for SDK versions to prevent that happening in the future.

Also helps catch out installs where people have tampered with the SDK - which has happened before.

In the early Big Sur days it also helped with the 10.16 vs 11.0 scenario (which we could not catch with a minimum CLT check) but I don't suspect that being an issue again.

It seems we already started being OK with that in the case of Big Sur though.

Sure, but it won't be okay once macOS 12 SDK lands.

Bo98 · 2020-12-23T03:52:13Z

There is still also the question of what happens to macOS 11 when macOS 12 arrives. Usually, CLT/Xcode remains compatible with the previous macOS for some time, but MacOSX.sdk will still be updated to point to the latest macOS's SDK. While proper SDK version detection will help avoid that using that SDK in this code - I'm somewhat concerned if things will break with any existing bottles as they will start using headers from this newer OS version after an Xcode/CLT update (in particular I'm talking about formulae like llvm/gcc which are designed to have hard baked references to the SDK path). Similar scenarios have had its issues in the past (e.g. git crashing on 10.14 when compiled with the 10.15 SDK).

Though that's something that I can't think of much solutions for. The lack of a MacOSX11.sdk (or similar) is extremely unfortunate.

carlocab · 2020-12-23T05:10:19Z

Thanks for your comments, @Bo98. I'd actually been digging up old discussions on issues related to this (e.g. #5068) to try to establish some context. I found your comments comprehensive and insightful there, and I'm pleased to see that continue here.

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 20, 2020

carlocab mentioned this pull request Dec 20, 2020

Big Sur 11.1 update moved system headers? (Failed compilation using gcc and llvm) Homebrew/homebrew-core#67075

Closed

4 tasks

fxcoudert requested review from MikeMcQuaid and mistydemeo December 20, 2020 17:25

SeekingMeaning mentioned this pull request Dec 21, 2020

f3d 1.1.0 Homebrew/homebrew-core#67306

Closed

MikeMcQuaid reviewed Dec 21, 2020

View reviewed changes

carlocab mentioned this pull request Dec 21, 2020

Bugs after upgrade to MacOSX11.1 warning: no such sysroot directory: '/Library/Developer/CommandLineTools/SDKs/MacOSX11.0.sdk' Homebrew/homebrew-core#67358

Closed

3 tasks

MikeMcQuaid requested a review from a team December 21, 2020 09:28

fxcoudert reviewed Dec 21, 2020

View reviewed changes

sdk: use unversioned SDK path on Big Sur

3f0ed01

Fixes Homebrew/homebrew-core#67075. This implements @fxcoudert's suggestion in the issue linked above.

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 22, 2020

BrewTestBot approved these changes Dec 22, 2020

View reviewed changes

MikeMcQuaid merged commit b4303c2 into Homebrew:master Dec 22, 2020

carlocab deleted the big-sur-sdk-path branch December 22, 2020 08:14

carlocab mentioned this pull request Dec 22, 2020

root: revision bump Homebrew/homebrew-core#67411

Closed

5 tasks

Bo98 reviewed Dec 23, 2020

View reviewed changes

This was referenced Dec 23, 2020

os/mac/sdk: parse version from SDKSettings.json #10112

Merged

os/mac/pkgconfig: merge pc files for 11.1 and 11.0 #10129

Merged

This was referenced Jan 6, 2021

gdal: disable OpenEXR support Homebrew/homebrew-core#65832

Closed

dartsim: revision bump for bullet Homebrew/homebrew-core#68877

Closed

BrewTestBot added the outdated PR was locked due to age label Jan 23, 2021

Homebrew locked as resolved and limited conversation to collaborators Jan 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk: use unversioned SDK path on Big Sur #10072

sdk: use unversioned SDK path on Big Sur #10072

carlocab commented Dec 20, 2020 •

edited

Loading

BrewTestBot commented Dec 20, 2020

fxcoudert commented Dec 20, 2020

MikeMcQuaid Dec 21, 2020

carlocab Dec 21, 2020 •

edited

Loading

MikeMcQuaid Dec 21, 2020

carlocab Dec 21, 2020

fxcoudert Dec 21, 2020

MikeMcQuaid Dec 21, 2020

carlocab Dec 21, 2020 •

edited

Loading

MikeMcQuaid Dec 21, 2020

carlocab Dec 21, 2020

MikeMcQuaid Dec 22, 2020

fxcoudert left a comment

BrewTestBot commented Dec 22, 2020

MikeMcQuaid commented Dec 22, 2020

carlocab commented Dec 22, 2020 •

edited

Loading

Bo98 Dec 23, 2020 •

edited

Loading

carlocab Dec 23, 2020

carlocab Dec 23, 2020

MikeMcQuaid Dec 23, 2020

carlocab Dec 23, 2020

Bo98 Dec 23, 2020

Bo98 Dec 23, 2020 •

edited

Loading

Bo98 commented Dec 23, 2020 •

edited

Loading

carlocab commented Dec 23, 2020

	# Accept an SDK for another OS version if it shares a major version
	# with the current OS - for example, the 11.0 SDK on 11.1,
	# or vice versa.
	# Note that this only applies on macOS 11
	# or greater, given the way the versioning has changed.
	# This shortcuts the below check, since we do accept an older version
	# on macOS 11 or greater if the major version matches.
	return sdk if OS::Mac.version >= :big_sur && sdk.version.major == OS::Mac.version.major

sdk: use unversioned SDK path on Big Sur #10072

sdk: use unversioned SDK path on Big Sur #10072

Conversation

carlocab commented Dec 20, 2020 • edited Loading

BrewTestBot commented Dec 20, 2020

fxcoudert commented Dec 20, 2020

Choose a reason for hiding this comment

carlocab Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlocab Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxcoudert left a comment

Choose a reason for hiding this comment

BrewTestBot commented Dec 22, 2020

MikeMcQuaid commented Dec 22, 2020

carlocab commented Dec 22, 2020 • edited Loading

Bo98 Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bo98 Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

Bo98 commented Dec 23, 2020 • edited Loading

carlocab commented Dec 23, 2020

carlocab commented Dec 20, 2020 •

edited

Loading

carlocab Dec 21, 2020 •

edited

Loading

carlocab Dec 21, 2020 •

edited

Loading

carlocab commented Dec 22, 2020 •

edited

Loading

Bo98 Dec 23, 2020 •

edited

Loading

Bo98 Dec 23, 2020 •

edited

Loading

Bo98 commented Dec 23, 2020 •

edited

Loading