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 SystemExt::distribution_id() #844

Merged
merged 1 commit into from Sep 26, 2022

Conversation

legeana
Copy link
Contributor

@legeana legeana commented Sep 25, 2022

SystemExt::name() returns human-readable distribution name, but there is no machine-readable alternative.

SystemExt::distribution_id() returns os-release.ID, or std::env::consts::OS if os-release.ID is unavailable. Fallback behaviour is consistent with the os-release.ID specification on Linux.

See also

Closes #843.

@legeana legeana changed the title Add SystemExt::distribution_id() [WIP] Add SystemExt::distribution_id() Sep 25, 2022
@legeana
Copy link
Contributor Author

legeana commented Sep 25, 2022

I am working on test failures.

src/linux/system.rs Outdated Show resolved Hide resolved
src/traits.rs Outdated Show resolved Hide resolved
src/traits.rs Outdated
Comment on lines 1238 to 1239
/// Returns the distribution id as defined by os-release,
/// or OS-name if unavailable.
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about:

Suggested change
/// Returns the distribution id as defined by os-release,
/// or OS-name if unavailable.
/// Returns the distribution id as defined by os-release,
/// or [`std::env::consts::OS`].

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@GuillaumeGomez
Copy link
Owner

A few small nits but otherwise looks great!

@legeana legeana force-pushed the distribution_id branch 3 times, most recently from 41739a4 to ec1d180 Compare September 25, 2022 10:42
@legeana legeana marked this pull request as draft September 25, 2022 11:08
@legeana legeana changed the title [WIP] Add SystemExt::distribution_id() Add SystemExt::distribution_id() Sep 25, 2022
src/lib.rs Outdated
@@ -344,6 +344,8 @@ mod test {
.long_os_version()
.expect("Failed to get long OS version")
.is_empty());

assert!(!s.distribution_id().is_empty());
Copy link
Owner

Choose a reason for hiding this comment

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

Please move this test outside of the if System::IS_SUPPORTED since it can definitely work even on unknown targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Please take a look if this is what you meant.

@GuillaumeGomez
Copy link
Owner

For the once_cell failures, don't worry. I think I'll send a PR to update the minimum supported rust version or enforce the usage of a previous one to prevent breaking the CI. Not sure which way is better...

@legeana legeana force-pushed the distribution_id branch 2 times, most recently from 20d32d6 to 3fe5715 Compare September 25, 2022 22:36
@legeana
Copy link
Contributor Author

legeana commented Sep 25, 2022

For the once_cell failures, don't worry. I think I'll send a PR to update the minimum supported rust version or enforce the usage of a previous one to prevent breaking the CI. Not sure which way is better...

Thank you for clarification!

SystemExt::name() returns human-readable distribution name,
but there is no machine-readable alternative.

SystemExt::distribution_id() returns os-release.ID, or
std::env::consts::OS if os-release.ID is unavailable.
Fallback behaviour is consistent with the os-release.ID
specification on Linux.

See also
- https://www.freedesktop.org/software/systemd/man/os-release.html#ID=
- https://doc.rust-lang.org/std/env/consts/constant.OS.html

Closes GuillaumeGomez#843.
@legeana legeana marked this pull request as ready for review September 26, 2022 01:27
@GuillaumeGomez
Copy link
Owner

Thanks for adding this feature! I'll make a release not too far in the future.

@GuillaumeGomez GuillaumeGomez merged commit fbd44d4 into GuillaumeGomez:master Sep 26, 2022
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.

Machine-parseable equivalent to System::name()
2 participants