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

Add hsSystemInfo for gathering basic system info. #1014

Merged
merged 5 commits into from
Nov 17, 2021

Conversation

Hoikas
Copy link
Member

@Hoikas Hoikas commented Nov 12, 2021

This is for the upcoming redesigned crash dialog.

This replaces DisplaySystemVersion() with a more featureful hsSystemInfo that will, in addition to displaying the Windows version can display:

  • the wine version and OS kernel version if running under wine
  • the distribution name and version if running on Linux
  • the name of the CPU
  • the amount of system memory

This information will be extremely useful when attempting to diagnose crashes. A small test case tool plSystemInfo has also been added.

Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

Just to check, there's future work needed around macOS info, right?

@Hoikas
Copy link
Member Author

Hoikas commented Nov 12, 2021

Yeah, this should compile and function on macOS, but per @dgelessus, there is a way to get the "friendly" macOS name from plists that we could use. Right now, this should say "Darwin XXX" instead of "macOS YY".

@dpogue
Copy link
Member

dpogue commented Nov 12, 2021

Ahh, okay. I just saw the part where it falls back to uname.

IIRC I looked into pulling the correct macOS info out of plist files at one point and it involved jumping through hoops and private API and I grumbled about it before working on something more fun 😛

@Hoikas
Copy link
Member Author

Hoikas commented Nov 12, 2021

Looks like we'd want some of this: https://developer.apple.com/library/archive/documentation/CoreFoundation/Conceptual/CFPropertyLists/Articles/Saving.html#//apple_ref/doc/uid/20001175-CJBEHAAG Maybe if I ever get around to installing a compiler on the mac mini I acquired. Too bad macOS is a steaming pile.

Sources/Plasma/CoreLib/hsSystemInfo.cpp Outdated Show resolved Hide resolved
Sources/Plasma/CoreLib/hsSystemInfo.cpp Outdated Show resolved Hide resolved
@Hoikas
Copy link
Member Author

Hoikas commented Nov 12, 2021

I've split the OS logic out into their own functions and unified the signatures.

Copy link
Contributor

@dgelessus dgelessus left a comment

Choose a reason for hiding this comment

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

As an alternative to the whole version dict stuff, macOS also has a command-line tool called sw_vers which prints the same information:

$ sw_vers
ProductName:	Mac OS X
ProductVersion:	10.14.6
BuildVersion:	18G9323
$ sw_vers -productName
Mac OS X
$ sw_vers -productVersion
10.14.6
$ sw_vers -buildVersion
18G9323

Comment on lines +141 to +143
CFDictionaryRef dict = _CFCopyServerVersionDictionary();
if (dict == nullptr)
dict = _CFCopySystemVersionDictionary();
Copy link
Contributor

@dgelessus dgelessus Nov 12, 2021

Choose a reason for hiding this comment

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

Any particular reason for using these undocumented functions rather than reading the dictionaries from the plists (ServerVersion.plist and SystemVersion.plist in /System/Library/CoreServices)? The plists are widely known and at least somewhat documented, so they are unlikely to go away, whereas I couldn't find any information about these internal functions outside of the CoreFoundation source code. (The undocumented functions actually just read the plists internally, so it shouldn't make a difference.)

Helpful example code for how to do the plist reading: https://stackoverflow.com/a/56403143

Copy link
Member Author

Choose a reason for hiding this comment

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

Per @dpogue, Apple does shenanigans if you try to open that plist file: https://stackoverflow.com/questions/69097567/macos-version-returned-as-10-16-instead-of-12-0 - we've found that many other projects use these undocumented APIs to do the same job in a reliable, reproducible way. Hence this code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, compatibility hacks... According to this JDK bug, you can read .SystemVersionPlatform.plist to get the real version number on macOS 11 and later, and if that doesn't exist, fall back to the regular SystemVersion.plist.

And if you're calling sw_vers, you apparently have to pass the environment variable SYSTEM_VERSION_COMPAT=0, or it will also return 10.16 instead of 11.0.

Copy link
Member

Choose a reason for hiding this comment

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

What Hoikas is doing here is actually the same as what sw_vers does (at least, according to the open-source sw_vers code that Apple published): https://opensource.apple.com/source/DarwinTools/DarwinTools-1/sw_vers.c.auto.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, Apple likes to use undocumented APIs in their applications without providing a documented equivalent for external developers 🙂

Yeah, these functions are not that likely to go away, especially if other major projects use them as well. I just think that in a crash handler, it's better to be extra safe and avoid undocumented APIs, just in case they do change and end up crashing the crash handler somehow.

@Hoikas
Copy link
Member Author

Hoikas commented Nov 13, 2021

I've rebased this to fix the conflicts with #1018.

@Hoikas Hoikas merged commit 6f6e20d into H-uru:master Nov 17, 2021
@Hoikas Hoikas deleted the system_info branch November 17, 2021 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants