-
Notifications
You must be signed in to change notification settings - Fork 52
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
Implement OS Family feature #313
base: master
Are you sure you want to change the base?
Conversation
Something as food for thought. With Families, should |
Thanks! As for Android, I am not sure either and that is why I think the idea of families can be a little bit confusing. Though I agree that it spares the users of the library from writing some boilerplate. |
pub fn family(&self) -> Family { | ||
self.family | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it makes sense to use match here instead of storing family as a separate field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the recommendation here is something like, in a short hand version
match Type {
Type::Debian => Family::Linux,
Type::Gentoo => Family::Linux,
Type::FreeBSD => Family::BSD,
Type::Windows => Family::DOS,
// and so forth
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had exactly this in mind. Simply put this match into the family
method instead of expanding the Info
struct with an additional field. Do you see some disadvantages in this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For BSDs, Windows, and MacOS, that is fine since those categories do not expand often. But for the Linux, this would require everytime a new distro is added as a Type, then this match would have to expand and add another arm. The current method should scale with any number of distros without having to update this function or add a new arm to a match.
That is at least my current thought process. This would be the low maintenance way to approach this.
info.os_type(), | ||
info.version(), | ||
info.bitness() | ||
info.bitness(), | ||
info.family(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it is useful to show the family here? I'm a little afraid for the output to become too verbose. Additionally it can be a little weird in some cases. For example, currently it will be "OS Windows, Family: Unknown".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove that out. Probably not so useful on output, but I also assumed the output here was probably mostly for eyes on validation since os_info's biggest use case is probably as a library to gather the data needed and the caller would just grab the info and the fields it needs.
For Windows, the family should appear as DOS.
OS information:
Type: Windows
Version: 10.0.22000
Bitness: 64-bit
Family: DOS
Mhmm, that is odd. I got that output from my work machine which is Windows 11, but shows the version as Windows 10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhmm, that is odd. I got that output from my work machine which is Windows 11, but shows the version as Windows 10.
According to this information it is correct, but I agree that it can be confusing. Ideally, it should be something like "Windows 11 (10.0.22000)".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, yea that is confusing. You would think you see "10" as a major number for versioning, it specifically would reference Windows 10 since they changed from their random naming scheme of XP, Vista, Millennium to 7, 8, 10, 11.
os_info/src/redox/mod.rs
Outdated
@@ -18,7 +18,7 @@ pub fn current_platform() -> Info { | |||
os_type: Type::Redox, | |||
version, | |||
bitness: Bitness::Unknown, | |||
family: Family::DOS, | |||
family: Family::WindowsNT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also seems weird to me. 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this one, @kraileth suggested changing since Windows is no longer truly "DOS," and I guess that has been true since the 90s. So calling it in the "DOS" family is misleading. He recommended changing it to either something NT since NT is the kernel architecture or just having the family also be Windows. I don't really know enough about Windows and its architecture to really know either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought here: the OS
enviroment variable expands to Windows_NT
on windows systems
I just have one more idea. Perhaps instead of introducing families several functions can be added, such as |
That could work also. How would you imagine that looking implementation wise? |
Hi, idk if this suggestion is sound, but would it be a bad idea to add distro families as well? Like Arch (Arch, Manjaro, Endevour, Garuda, SteamOS, Artix), Debian (Debian, Ubuntu, PopOS, Mint), Gentoo (Gentoo, Sabayon) etc.? Idk how much it is useful for the average user, but knowing that every distro family has its own quirks (package managers, some data storage paths and all that), would be useful to have that kind of distinction between those instead of writing match os_info::get().os_type() {
Type::Arch | Type::EndevourOS | Type::Manjaro | Type::Garuda => {
// Arch specific code...
}
Type::Debian | Type::Mint | Type::Pop | Type::Raspbian | Type::Ubuntu => {
// Debian specific code...
}
Type::Fedora | Type::CentOS | Type::OracleLinux | Type::Redhat | Type::RedHatEnterprise => {
// RedHat specific code...
}
_ => {
// etc...
}
} Every time my project needs to get this distinction, especially when I can't remember some of them from the top of my head and i need to check online which distro family OS belongs to. |
You as users of the library probably have a better vision of how API should like. Originally I need a very simple information in form of a string (kind of "user agent"). So I'm open to your suggestions. 🙃 Still now I think that having some additional function can be better than introducing several kinds of families in form of enums. I suppose it can look something like that: fn is_arch_based(&self) -> bool {
matches!(self.os_type, Type::Arch | Type::EndevourOS | Type::Manjaro | Type::Garuda)
} But again I would like to hear if it will be OK for your use cases. |
@stanislav-tkach ye, function makes sense, enum was just a thing that first popped into my head, and I think functions would be easier to maintain as well so I'm more than ok with this solution, although having matches instead of ifs would have been nice, but I can live with that :) |
The biggest issues I see with these approaches is when a new type is added, these functions will also need to be updated. Adding in an enum for families should make this so all linux distros will show up as generic Linux without having to make sure we add the type to a function with an ever growing list type comparisons. At least for Comtrya's use case, this is all that is needed since every distro uses almost the identical command line tools for doing things like user management. |
@martintc you are right about that, but what I talked about was yet another suggestion that is more or less an extension of your proposal, to have subfamilies for distros (arch/debian/Gentoo/etc. based) so it's easier to deal with instead of writing big match statements every time if anyone needs this kind of distinction, apart from me😂 I still dk which way would be better tbh, Enum or functions, cuz with Enum u need to create and maintain a new type but u also need to create and maintain functions, giving more or less same amount of work for maintenance, so it's rly about preference here |
For implementing specific details on which distro, a function may make more sense. But it begs a larger question as to what is useful or not or as to how far should it be taken. Is it always enough to see if a distro is descended from Debian? In some cases yes, but in some cases no, like Devuan. Where the init system is different. Actually, with Devuan striving to support SysV, Runnit, OpenRC, etc. Having functions such a It leads in a whole world of complexity. |
@martintc ye, you are right, didn't even think of that. Guess manual check (for now at least) it is, cuz i honestly am not sure how deal with this kind of issue |
I do think it is valid and interesting. I am wondering though, if Linux specific details would fit better in its own crate that depends on OS_info. If your interested in doing that, I would definitely be down with working on a library crate with you on it. A library crate that takes the data from os_info and expands on the information. |
@martintc sounds like fun, definitely wouldn't mind to :) I'm not in front of my computer tho, so u can create a repo and i can think of smth when i get home |
Sounds good. I will get one generated but won't be able to generate much until later today after work. But for sure I see the information as valuable. |
Nice |
Hey @martintc, i just realized this is the only way we can communicate at the moment, do u have discord or smth where we could talk about the project? U can email it to me [email protected] if u don't want this to be here |
Sounds good. Here is my discord handle. Here is the repo |
@stanislav-tkach any more opinion on this topic? |
I think that the issue that you have described is more or less the same regardless of which approach (enum or functions) is chosen:
|
Generally I think that expanding the library api is not a big deal, but I agree with you that in this case it can be both burdensome to maintain and confusing to use. |
I see this as part of a larger question: depending on what users want to do, they can be interested in information on the OS kernel (ie. "Linux" for Android too), or userpace family (ie. "Unix" for Linux and *BSD, but likely "Android" (or "Unknown"?) for Android). |
As per #312, this is adding a feature to display the family that an OS belongs to. Purpose is that in the case of Linux, if a caller of the library needs to perform program logic and it doesn't care what distro it is, it can use this to match against rather than match against every distribution listed in
Type.rs
.The addition should not break any existing use of the API, only adds to the API.