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.

Sign up for GitHub

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

Improve System::long_os_version() on Linux and Android #1427

Merged
merged 4 commits into from
Dec 14, 2024
Merged

Improve System::long_os_version() on Linux and Android #1427

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ddd4dc0
Split Linux's and Android's long_os_version into separate functions
dtolnay Dec 13, 2024
8c8874b
Improve System::long_os_version() on Android
dtolnay Dec 13, 2024
d1e0c22
Improve System::long_os_version() on Linux
dtolnay Dec 13, 2024
d375fc6
Prefix distro version with "unknown" if distro name is not known
dtolnay Dec 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/common/system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,8 +735,8 @@ impl System {
///
/// | example platform | value of `System::long_os_version()` |
/// |---|---|
/// | linux laptop | "Linux 24.04 Ubuntu" |
/// | android phone | "Android 15 Pixel 9 Pro" |
/// | linux laptop | "Linux (Ubuntu 24.04)" |
/// | android phone | "Android 15 on Pixel 9 Pro" |
/// | apple laptop | "macOS 15.1.1 Sequoia" |
/// | windows server | "Windows Server 2022 Datacenter" |
///
Expand Down
36 changes: 28 additions & 8 deletions src/unix/linux/system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,23 +385,43 @@ impl SystemInner {
get_system_info_android(InfoType::Name)
}

#[cfg(not(target_os = "android"))]
pub(crate) fn long_os_version() -> Option<String> {
#[cfg(target_os = "android")]
let system_name = "Android";
let mut long_name = "Linux".to_owned();

let distro_name = Self::name();
let distro_version = Self::os_version();
if let (Some(distro_name), Some(distro_version)) = (&distro_name, &distro_version) {
// "Linux (Ubuntu 24.04)"
long_name.push_str(" (");
long_name.push_str(distro_name);
long_name.push(' ');
long_name.push_str(distro_version);
long_name.push(')');
} else if let Some(distro_or_version) = distro_name.or(distro_version) {
long_name.push_str(" (");
long_name.push_str(&distro_or_version);
long_name.push(')');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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().

Copy link
Owner

@GuillaumeGomez GuillaumeGomez Dec 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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":

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.

}

#[cfg(not(target_os = "android"))]
let system_name = "Linux";
Some(long_name)
}

let mut long_name = system_name.to_owned();
#[cfg(target_os = "android")]
pub(crate) fn long_os_version() -> Option<String> {
let mut long_name = "Android".to_owned();

if let Some(os_version) = Self::os_version() {
long_name.push(' ');
long_name.push_str(&os_version);
}

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"
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the extra comment, always very appreciated! :)

// which is documented as "The end-user-visible name for the end product."
// So this produces a long_os_version like "Android 15 on Pixel 9 Pro".
if let Some(product_name) = Self::name() {
long_name.push_str(" on ");
long_name.push_str(&product_name);
}

Some(long_name)
Expand Down
Loading