stanislav-tkach / os_info

Rust library to detect the operating system type
MIT License
175 stars 52 forks source link

Semantic/Custom version confusion #318

Open tedinski opened 2 years ago

tedinski commented 2 years ago

I haven't fully verified this is reproducible yet.

I think what I'm seeing is that:

Could this be the presence/absence of the lsb_release mechanism working? Looks like that might reliably return Custom: https://github.com/stanislav-tkach/os_info/blob/master/os_info/src/linux/lsb_release.rs#L14 but then the file_release mechanism will try to parse: https://github.com/stanislav-tkach/os_info/blob/master/os_info/src/version.rs#L40

Should os_info include a "version comparison" function with appropriate hacks to make this more reliable? Or at least... have I missed documentation about how to reliably detect a version? Or is this just a bug?

stanislav-tkach commented 2 years ago

Thank you for the report! I suppose this is a bug. Unfortunately, I won't be able to dig into this in the nearest future - I have recently changed a job, so now it takes most of my time. Though I will still be able to review and merge a fix if you would like to try to deal with this issue.

By the way, now I think that the whole semantic/custom approach is unnecessary and only adds complications. I feel like storing a version string as it is would be enough, but changing it is a breaking change.

tedinski commented 2 years ago

By the way, now I think that the whole semantic/custom approach is unnecessary and only adds complications

I possibly agree, but there might still be value in providing some functions that help inspect OS versions, where just reverting to Option<String> or the like kinda punts that task onto users. (OTOH, maybe that version comparison stuff should be from some other existing crate? IDK)

As a bonus, if we add some of these now, people can switch over to them and then be insulated from a future breaking change to Version.

For my own purposes, I mostly just need to test for specific versions because our goal is "On this old LTS? Apply this workaround" so I was thinking of adding something like:

impl Version {
  fn looks_like(&self, version_str: &str) -> bool { ... }
}

The goal here would be that I could write os.version().looks_like("18.04") and I get what I want without worry about the future breaking change, since (with the breaking change) it'd just becomes a simple string comparison. (Whereas for now, we'd have to try applying the version parsing function on version_str if self were Semantic.)

The drawback of this particular API though is that if people give it "18.4" they'd get the same flaky behavior I'm seeing. :/ I suppose the below would be more difficult to mis-use, but then you'd definitely be committed to doing some kind of parsing, even if just internally:

impl Version {
  fn looks_like(&self, major: u64, minor: u64) { ... }
}

Would you be interested in one of these as a PR?

stanislav-tkach commented 2 years ago

Sorry for the late reply and thanks for such a detailed explanation of your use case!

I don't see any harm in extending the api in such way. I only have one consideration about additional dependencies. It makes sense to hide the new api under a feature if some "heavy" crates are needed for the implementation.

ydirson commented 1 year ago

There is similar issue with Debian versions as VMs under XCP-ng / Xen. I will need to dig this one.