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

Remove sys-info dependency #5

Merged
merged 1 commit into from Jan 27, 2023
Merged

Conversation

rgwood
Copy link
Contributor

@rgwood rgwood commented Jan 24, 2023

Hi there,

This PR removes the sys-info dependency, which is relatively large with a few dependencies of its own. Because we only use a tiny part of sys-info (the Linux part of os_release()), it's easy to "inline" what we need and avoid taking a dependency.

I think this makes sense to keep compile times and binary size down, especially on non-Linux platforms where we don't need anything from sys-info. Let me know what you think!

Licensing

sys-info is MIT-licensed, so as I understand it this is OK because I've copied the copyright+permission notices verbatim. Doing so in source code comments seemed easiest.

Copy link
Owner

@TheLarkInn TheLarkInn left a comment

Choose a reason for hiding this comment

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

Looks great and makes sense! 🎉🎉 Thanks for the contribution! 🙇

@TheLarkInn TheLarkInn added the enhancement New feature or request label Jan 27, 2023
@TheLarkInn
Copy link
Owner

Desirability as compile time and downloaded binary size is a new realization for me. However, in regards to binary size, doesn't the compiler on Release remove unused parts of a library?

@TheLarkInn TheLarkInn merged commit 4be196e into TheLarkInn:main Jan 27, 2023
@rgwood
Copy link
Contributor Author

rgwood commented Jan 28, 2023

in regards to binary size, doesn't the compiler on Release remove unused parts of a library?

That’s a good point. I believe you’re right, so this is mostly a compile time improvement (IIRC the entire dependent crate gets compiled even if only part of it is used in the final executable).

From a binary size perspective, one (small, hypothetical) issue with the previous approach would be non-Linux platforms. The compiler didn’t have enough info to know that os_release() would never be called on non-Linux platforms and it looks like the non-Linux path for os_release() has quite a bit going on. But this is admittedly a bit speculative, I shoulda measured binary size :)

@TheLarkInn
Copy link
Owner

I love this idea to be able to measure binary sizes for a compiled library. Perhaps this would be a nice tool to create for PR workflows! I'm going to write this in my notebook to work on for the future!

@rgwood
Copy link
Contributor Author

rgwood commented Jan 29, 2023

You might find cargo bloat useful for inspiration: https://github.com/RazrFalcon/cargo-bloat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants