search

Macchina-CLI / libmacchina

A library providing access to all sorts of system information.
https://crates.io/crates/libmacchina
MIT License
68 stars 20 forks source link

feat: add support for RPM `ndb` databases #159

Closed RubixDev closed 1 year ago

RubixDev commented 1 year ago

This closes #154.

As mentioned by @CarterLi in https://github.com/Macchina-CLI/libmacchina/issues/154#issuecomment-1509428314, fastfetch uses librpm to count RPM packages. There are Rust bindings to librpm (librpm-sys), however depending on that crate proved to introduce many build complications, and, more importantly, would require librpm to be installed on every system that macchina, or other binaries depending on libmacchina, are run on.

So I instead created a new small crate called rpm-pkg-count which directly links to librpm and uses fastfetch's method of retrieving the package count. To mitigate the second issue I make use of the libloading crate, which allows me to try and load the librpm library at runtime and simply return None if it is not found.

The way I currently added rpm-pkg-count to libmacchina is behind a new feature called rpm. With the feature disabled (as it is by default), libmacchina continues to use the sqlite solution. If you wish, I could also replace the current sqlite implementation with the new one. Note, however, that the new method requires librpm to be installed on systems for all users that want rpm package count support, even on distros that use the sqlite backend, unlike the current manual sqlite solution (this may need to be documented somewhere).

Edit: I removed the rpm feature again and now libmacchina will always try the existing sqlite method first, as that seems to be faster than using librpm (see https://github.com/Macchina-CLI/libmacchina/pull/159#issuecomment-1546939452). Only if that fails, librpm will be used.

Gobidev commented 1 year ago

It looks like your current implementation only runs the detection using your crate when the rpm feature is enabled. It would be better if count_rpm() also tries the previous detection method using sqlite when this detection fails to also provide support for systems that work with sqlite and do not have librpm installed.

Edit: I did some testing and found that if both methods work for a system, the sqlite implementation is noticeably faster than using librpm, so we should probably always try that first and if it fails also try the librpm method.

RubixDev commented 1 year ago

The sqlite implementation is now always run first and only if it fails librpm is used. imo the extra feature isn't really required at this point.

Gobidev commented 1 year ago

imo the extra feature isn't really required at this point.

I agree, the only reason to disable it now would be to reduce the amount of dependencies, which was also not done for sqlite.

grtcdr commented 1 year ago

Looks good to me as well.

Thanks to everyone for taking their time to review this PR and for leaving the helpful comments and remarks in order to improve it -- and thank you @RubixDev for all your effort!

RubixDev commented 1 year ago

and thank you @RubixDev for all your effort!

You're welcome!

Just one extra note. I noticed the CI build failing on two targets so I pushed an update to rpm-pkg-count (now v0.2.1) that should fix this by relying on type inference.

Gobidev commented 1 year ago

The builds pass with v0.2.1!

@grtcdr can you make a new release with this feature? That would help out a lot^^

We should probably also add a note somewhere that makes clear that librpm needs to be installed for RPM package count to work on certain distributions.

grtcdr commented 1 year ago

@grtcdr can you make a new release with this feature? That would help out a lot^^

Sure thing!

We should probably also add a note somewhere that makes clear that librpm needs to be installed for RPM package count to work on certain distributions.

Would you like to add that to the README under a section titled "Notes"?