Skip to content

macos: reimplement OS version detection - #7714

Merged
andrewrk merged 2 commits into
ziglang:masterfrom
mikdusan:target-macos
Jan 13, 2021
Merged

macos: reimplement OS version detection#7714
andrewrk merged 2 commits into
ziglang:masterfrom
mikdusan:target-macos

Conversation

@mikdusan

@mikdusanmikdusan commented Jan 7, 2021•
edited
Loading

Copy link
Copy Markdown
Member

The macOS version is now obtained by parsing SystemVersion.plist.

Test cases added for plist files that date back to '2003 Panther and up
to the recent '2020 Big Sur 11.1 release of macOS.

Thus we are now able to reliably identify 10.3...11.1 and higher.

  • drop use of kern.osproductversion sysctl
  • drop use of kern.osversion sysctl (fallback)
  • drop kern.osversion tests
  • add lib.std.zig.system.detect()
  • add minimalistic parser for SystemVersion.plist
  • add test cases for { 10.3.0, 10.3.9, 10.15.6, 11.0, 11.1 }

closes#7569

@mikdusanmikdusan added the os-macos macOS label Jan 7, 2021
kubkon
kubkon reviewed Jan 7, 2021
View reviewed changes
Comment threadlib/std/zig/system/macos.zig Outdated
@kubkon

kubkon commented Jan 7, 2021

Copy link
Copy Markdown
Member

Quickly skimming over this, looks great! I'll reserve some time tomorrow to go over this properly, and come back with feedback.

Mouvedia
Mouvedia reviewed Jan 7, 2021
View reviewed changes
Comment threadlib/std/target.zig
Mouvedia
Mouvedia suggested changes Jan 8, 2021
View reviewed changes
Comment threadlib/std/target.zig
Comment threadlib/std/zig/system/macos.zig Outdated
daurnimator
daurnimator reviewed Jan 9, 2021
View reviewed changes
Comment threadlib/std/zig/system.zig Outdated
daurnimator
daurnimator reviewed Jan 9, 2021
View reviewed changes
Comment threadlib/std/zig/system/macos.zig Outdated
@mikdusan
mikdusanforce-pushed the target-macos branch 3 times, most recently from be73ae9 to f9a8abeCompareJanuary 9, 2021 22:02
andrewrk
andrewrk requested changes Jan 11, 2021
View reviewed changes

@andrewrkandrewrk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have some requested changes, but the core approach to version detection looks great. Thanks for tackling this problem.

Comment threadlib/std/zig/system/macos.zig Outdated
Comment threadlib/std/zig/system/macos.zig Outdated
if (build[yindex] < '0' or build[yindex] > '9') break;
if (std.fs.cwd().readFile(path, &buf)) |bytes| {
if (parseSystemVersion(bytes)) |ver| {
// never return red herring

@andrewrkandrewrkJan 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

was there a version of macos named "red herring"? I don't understand this comment

@mikdusanmikdusanJan 11, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

10.16 is the backwards compatible version and it's a red herring. Apple is purposely hiding any differentiation between 11.0, 11.1, 11.2, ... . It's low fidelity for our purposes. There is no macOS that returns 10.16 via these mechanisms:

  • sw_vers command line
  • "About This Mac" in desktop Apple menu
  • system_profiler GUI
  • system_profiler command line
  • when running Xcode on Big Sur it defaults to 11.1 because it detects it proper

Comment threadlib/std/zig/system/macos.zig Outdated
Comment threadlib/std/zig/system/macos.zig Outdated
Comment threadlib/std/zig/system/macos.zig Outdated
}
// continue
} else |err| {
std.log.warn(format_failure, .{ "parse", path, err });

@andrewrkandrewrkJan 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why warn rather than debug?

@mikdusanmikdusanJan 11, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

not sure I really understand the semantic differences and just went with it because it was suggested earlier

@mikdusanmikdusanJan 11, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

ah I misinterpreted your question as referring to std.debug.warn ; after scanning std.log.Level I'd recommend log.err be used:

 /// Error: A bug has been detected or something has gone wrong but it is
/// recoverable.

Comment threadlib/std/zig/system.zig Outdated
@mikdusan
macos: reimplement OS version detection
f2be1fb
The macOS version is now obtained by parsing `SystemVersion.plist`.
Test cases added for plist files that date back to '2005 Panther and up
to the recent '2020 Big Sur 11.1 release of macOS.
Thus we are now able to reliably identify 10.3...11.1 and higher.
- drop use of kern.osproductversion sysctl
- drop use of kern.osversion sysctl (fallback)
- drop kern.osversion tests
- add `lib.std.zig.system.detect()`
- add minimalistic parser for `SystemVersion.plist`
- add test cases for { 10.3, 10.3.9, 10.15.6, 11.0, 11.1 }
closesziglang#7569
andrewrk
andrewrk requested changes Jan 12, 2021
View reviewed changes

@andrewrkandrewrk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In general I'm not a fan of using warn. If it's a problem, we should deal with it appropriately, and probably fail. If it's not a problem, why do we need to warn about it?

To be clear what I meant was I'm not a fan of using logging for error reporting in functions such as this. In this code we end up doing catch {} which eats the error, meaning that there was in fact not a problem at all. But just prior to that we logged an error! Something's gone wrong. I don't think this code, which is standard library code, should do any logging at all. It should report the error up to the caller, and let the caller decide what is the situation.

And for this situation:

For macOS, the system is more uniform. I think we should expect version detection to work and report failure if it does not.

I don't think catch {} and sticking with the default version range is the right call. NativeTargetInfo.detect should fail if macOS version detection fails.

@mikdusan
more fixups
4c3de99
- clarify comments
- `NativeTargetInfo.detect()` propagate macOS errors
- `macos.detect()` drop `std.log` usage
@mikdusan

mikdusan commented Jan 12, 2021

Copy link
Copy Markdown
MemberAuthor

@andrewrk changes incorporated.

@andrewrk
andrewrk merged commit e564d2c into ziglang:masterJan 13, 2021
@mikdusan
mikdusan deleted the target-macos branch September 19, 2023 18:11
@cole-hcole-h mentioned this pull request Jul 13, 2024
Merged
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kubkonkubkonkubkon left review comments
@andrewrkandrewrkandrewrk requested changes
+2 more reviewers
@daurnimatordaurnimatordaurnimator left review comments
@MouvediaMouvediaMouvedia requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

os-macosmacOS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

macOS Big Sur real versioning hidden from pre-Big Sur binaries

5 participants

@mikdusan@kubkon@andrewrk@daurnimator@Mouvedia