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

Replace sys-info with sysinfo crate #17

Merged
merged 2 commits into from
Jan 4, 2025

Conversation

unlimitedsola
Copy link
Contributor

@unlimitedsola unlimitedsola commented Dec 30, 2024

Closes #16 :

Rumors say cargo install bat is failing for some people due to a compilation failure from the sys-info crate. However the fix has not been merged for over half a year and the crate itself hasn't been active for over 2 years now.

There is an alternative crate sysinfo that's more regularly maintained and I believe it should provide similar functionality needed by this project.


Examples copied from sysinfo::System::long_os_version's documentation:

example platform value of System::long_os_version()
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"

I can add the kernel version information too if someone can give suggestions on how to format them together.


EDIT:

This PR depends on the MSRV bump to 1.74 proposed in #18. merged.

A more comprehensive list of outputs is also available in a successful action run based on #18:
https://github.com/sola-contrib/bugreport/actions/runs/12552288110

@unlimitedsola
Copy link
Contributor Author

The current master branch that this is based on doesn't seem to be able to pass the CI configured: https://github.com/sola-contrib/bugreport/actions/runs/12551564715/job/34996187540

@unlimitedsola unlimitedsola mentioned this pull request Dec 30, 2024
@unlimitedsola
Copy link
Contributor Author

I made another PR #18 to fix the CI issues, which also bumps the MSRV for the sysinfo crate for this PR.

@sharkdp
Copy link
Owner

sharkdp commented Dec 31, 2024

Thank you!

Would you mind rebasing this, so we can compare the outputs in CI with master?

@unlimitedsola
Copy link
Contributor Author

Thank you!

Would you mind rebasing this, so we can compare the outputs in CI with master?

Done!

@unlimitedsola
Copy link
Contributor Author

unlimitedsola commented Dec 31, 2024

so we can compare the outputs in CI with master?

The main difference should be that the kernel version is replaced with the OS/distribution version.

I can add the kernel version information too if someone can give suggestions on how to format them together.

I could change this to include the kernel version as well like below:

Ok(ReportEntry::List(vec![
    ReportEntry::Text(format!(
        "OS: {}",
        sysinfo::System::long_os_version().unwrap_or_else(|| "Unknown".to_owned()),
    )),
    ReportEntry::Text(format!(
        "Kernel: {}",
        sysinfo::System::kernel_version().unwrap_or_else(|| "Unknown".to_owned()),
    )),
]))

@sharkdp Please let me know your thoughts on the formatting, or any additional information that's available in the sysinfo crate that you would like to include. Thanks!

@unlimitedsola
Copy link
Contributor Author

I could change this to include the kernel version as well like below:

To save us time from back-and-forth, I pushed an additional commit to add the kernel version information as an option. You can choose whether to include it when applying the patches from this PR.

@sharkdp sharkdp merged commit 73e84cd into sharkdp:master Jan 4, 2025
28 checks passed
@sharkdp
Copy link
Owner

sharkdp commented Jan 4, 2025

Thank you very much!

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.

Consider replacing sys-info with sysinfo crate?
2 participants