Improve System::long_os_version() on Linux and Android #1427
Conversation
dtolnay
force-pushed
the
longosversion
branch
from
8cb7cf4 to
22c59ea
Compare
src/unix/linux/system.rs
Outdated
| long_name.push_str(" ("); | ||
| long_name.push_str(&distro_or_version); | ||
| long_name.push(')'); |
There was a problem hiding this comment.
This part might be weird — I am not sure what circumstances would lead to System::name() being Some and System::os_version() being None, or vice versa. The former wouldn't be an issue: "Linux (Ubuntu)" is perfectly fine. The latter is worse: "Linux (24.04)".
I considered leaving out os_version() if name() is None, but then we have a situation that the value of long_os_version() specifically does not include the os_version().
There was a problem hiding this comment.
I think adding "unknown" in case the OS name is not known would likely improve the situation? If the version is unknown then just "Linux (Ubuntu)" sounds fine I think.
So with the three cases it gives:
Linux (Ubuntu 24.04)
Linux (unknown 24.04)
Linux (Ubuntu)
What do you think?
There was a problem hiding this comment.
Yes, good call. I like that.
Speaking of "unknown", I forgot about checking for "unknown" in the Android case which is something I mentioned in #1419 (comment). Because getString in Build.java can return "unknown":
- https://android.googlesource.com/platform/frameworks/base/+/bc3378bf08f908335c721e597f51f6b95374f0c8/core/java/android/os/Build.java#137
- https://android.googlesource.com/platform/frameworks/base/+/bc3378bf08f908335c721e597f51f6b95374f0c8/core/java/android/os/Build.java#1564
- https://android.googlesource.com/platform/frameworks/base/+/bc3378bf08f908335c721e597f51f6b95374f0c8/core/java/android/os/Build.java#55
But looking at this more, I think "unknown" is specific to the Java API. They have public static final String MODEL where they have chosen to use the string "unknown" instead of null when a particular property hasn't been set. Whereas in Rust, we deal with libc::__system_property_get returning 0 by turning it into None (System::name() returns Option<String> not String). So I think we do not need to check for "unknown" after all.
dtolnay
force-pushed
the
longosversion
branch
from
22c59ea to
d1e0c22
Compare
| if let Some(short_name) = Self::name() { | ||
| long_name.push(' '); | ||
| long_name.push_str(&short_name); | ||
| // Android's name() is extracted from the system property "ro.product.model" |
There was a problem hiding this comment.
Thanks for the extra comment, always very appreciated! :)
|
Looks all good to me, thanks! Gonna merge it tomorrow once CI is done and brain has successfully rebooted. 😉 |
dtolnay
force-pushed
the
longosversion
branch
from
6ec433c to
d375fc6
Compare
|
Thanks again! |
Closes #1420. Closes #1419.