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

Replaced sys-info with systemstat. #4504

Closed
wants to merge 2 commits into from
Closed

Replaced sys-info with systemstat. #4504

wants to merge 2 commits into from

Conversation

Nimpruda
Copy link

Replaced sys-info with systemstat. systemstat doesn't implement neither a hostname nor a os_release function tho, so i had to ignore the tests.

@CLAassistant
Copy link

CLAassistant commented Mar 27, 2020

CLA assistant check
All committers have signed the CLA.

@ry ry changed the title First implementation of #4496 Replaced sys-info with systemstat. Mar 30, 2020
checksum = "05aec50c70fd288702bcd93284a8444607f3292dbdf2a30de5ea5dcdbe72287b"
dependencies = [
"memchr 1.0.2",
]
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize that systemstat depends on nom, and a very old version of nom at that.

Looking through their source code they have some very ridiculous macros
https://github.com/myfreeweb/systemstat/blob/a7c54aa0d052989e93f1ef84eb1686fbbfeb92f9/src/platform/linux.rs#L62-L82

This does not give me confidence...

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

@Nimpruda Thank you very much for this contribution - your patch looks fine, but I'm having second thoughts about systemstat... I need to research more if there isn't an easier way to achieve the same functionality.

@bartlomieju
Copy link
Member

Closing as stale; we'll revisit this topic after 1.0 is released. Thanks for your work @Nimpruda

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.

4 participants