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

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

Merged
merged 1 commit into from
Dec 24, 2020
Merged

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

merged 1 commit into from
Dec 24, 2020

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Dec 23, 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 typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?
    • This returned an error about Language::Java::java_home
  • Have you successfully run brew man locally and committed any changes?

This implements feedback from #10072.

We try to use a more sensible version check for the SDK, by querying, in order of priority:

  1. sdk_prefix/MacOSX.sdk/SDKSettings.json
  2. sdk_prefix/MacOSX.sdk/SDKSettings.plist
  3. OS::Mac.sdk_version

@BrewTestBot
Copy link
Member

Review period will end on 2020-12-24 at 13:35:46 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 23, 2020
@carlocab carlocab requested a review from Bo98 December 23, 2020 13:36
@carlocab
Copy link
Member Author

@Bo98 Not sure if the last fallback to OS::Mac.sdk_version is necessary or even desired. Let me know your thoughts.

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.

Great work so far! Any thoughts on removing the existing :big_sur logic you mentioned given this too? Interested in @Bo98's thoughts on that too if they have a chance.

Library/Homebrew/os/mac/sdk.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/mac/sdk.rb Outdated Show resolved Hide resolved
sdk_settings_plist = File.join(sdk_path, "SDKSettings.plist")

if File.exist?(sdk_settings_json)
version = JSON.parse(File.read(sdk_settings_json))["Version"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
version = JSON.parse(File.read(sdk_settings_json))["Version"]
version = JSON.parse(File.read(sdk_settings_json))["Version"].presence

and then I'd suggest using version ||= on the other cases below so it gets set to something even if this version is missing in the JSON.

Copy link
Member

Choose a reason for hiding this comment

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

Can skip this too once it's only the JSON stuff.

Library/Homebrew/os/mac/sdk.rb Outdated Show resolved Hide resolved
@Bo98
Copy link
Member

Bo98 commented Dec 23, 2020

Not sure if the last fallback to OS::Mac.sdk_version is necessary or even desired.

Probably not. I'd much rather reject a SDK if SDKSettings is missing (which should never really happen).

@carlocab
Copy link
Member Author

Thanks, @Bo98. Did you have thoughts regarding this

# 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

in light of the last PR?

@Bo98
Copy link
Member

Bo98 commented Dec 23, 2020

I believe that will still be necessary if the user has, for example, macOS 11.1 installed but hasn't updated Xcode and so still has the 11.0 SDK.

@MikeMcQuaid
Copy link
Member

I believe that will still be necessary if the user has, for example, macOS 11.1 installed but hasn't updated Xcode and so still has the 11.0 SDK.

Thanks @Bo98 👍🏻

@carlocab carlocab requested a review from MikeMcQuaid December 23, 2020 15:19
@Bo98
Copy link
Member

Bo98 commented Dec 24, 2020

Just to copy over the point from the other PR:

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.

Is this something we want to handle or just assume it won't happen again with the Big Sur versioning system? I reckon it won't happen again so I'm fine with this pull request as-is.

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Dec 24, 2020
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 24, 2020
@BrewTestBot
Copy link
Member

BrewTestBot commented Dec 24, 2020

Review period ended.

@MikeMcQuaid
Copy link
Member

Is this something we want to handle or just assume it won't happen again with the Big Sur versioning system? I reckon it won't happen again so I'm fine with this pull request as-is.

Me too. Thanks @carlocab and thanks @Bo98 for all the review.

@MikeMcQuaid MikeMcQuaid merged commit a2ae6f8 into Homebrew:master Dec 24, 2020
@carlocab carlocab deleted the big-sur-sdk-path-v2 branch December 24, 2020 13:32
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 24, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 24, 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.

5 participants