Skip to content

Conversation

@JOT85
Copy link

@JOT85 JOT85 commented Jun 29, 2020

The basis of this PR is https://www.freedesktop.org/software/systemd/man/os-release.html

I've added fields specified in that man page which weren't in the OsRelease structure before. These fields appear in other operating systems such as Arch, Fedora and derivatives. These are:

  • ANSI_COLOR (this is the main thing I found missing!)
  • CPE_NAME
  • DOCUMENTATION_URL
  • BUILD_ID
  • VARIANT
  • VARIANT_ID
  • LOGO

Default values for name, pretty_name and id are now also provided as specified by the man page.

OsRelease::new falls back to /usr/lib/os-release.

Also added tests with os-release files from multiple distros with added weird formatting.

Thanks!

JOT85 added 3 commits June 29, 2020 22:17
Also added default values for fields, fall back on `/usr/lib/os-release`
if `/etc/os-release` cannot be opened and added more tests.
@mmstick
Copy link
Member

mmstick commented Jul 8, 2020

Should optional keys be Option<String> instead of String?

@mmstick
Copy link
Member

mmstick commented Jul 8, 2020

I think it'd be better if optional fields are left in the extras hashmap, and accessible through methods.

@JOT85
Copy link
Author

JOT85 commented Jul 8, 2020

I think it'd be better if optional fields are left in the extras hashmap, and accessible through methods.

Almost all of the fields are optional though. In fact, only 3 aren't.

I think all fields in the spec should be present in the struct with additional (unofficial) fields in the hash-map.

I considered making optional fields Option<String> however the API before left optional fields as blank strings if they were not provided so I assumed that was a choice made and stuck to that API.

Doc correction: mention that version is optional.
@JOT85
Copy link
Author

JOT85 commented Jul 9, 2020

Should optional keys be Option instead of String?

I considered making optional fields Option however the API before left optional fields as blank strings if they were not provided so I assumed that was a choice made and stuck to that API.

Just to be clear, I think having fields as Option<String> is a perfectly sensible API and a change to that API is not a bad thing. I also don't mind the fields being blank if no value is specified as that doesn't loose much important information. That was how the original API worked and so I didn't change it. What are your thoughts?

id: "linux".into(),
pretty_name: "Linux".into(),

version: String::new(),

Choose a reason for hiding this comment

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

(I know this is an ancient PR, but we were considering adding a new use of this crate in a project and I went to look at it and noticed the last commit was 6 years ago, which...isn't wrong since obviously things are stable, but on the other hand I'd like to just know things I depend on are maintained)

Anyways: I think this can just be ..Default::default() right?

Copy link
Member

Choose a reason for hiding this comment

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

Haven't seen any use case that required updating it. There is already a hash map storing extra fields.

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.

3 participants