search

rpm-software-management / librpm.rs

Rust bindings to the RPM Package Manager libraries (librpm, librpmbuild, librpmsign)
https://rustrpm.org/librpm/
Mozilla Public License 2.0
32 stars 14 forks source link

[WIP] Change the API to be more robust and idiomatic #16

Closed mkpankov closed 4 years ago

mkpankov commented 4 years ago

Supersedes #7 and #14.

Fixes #12, #13, #15.

Documentation is not updated yet, want to run CI first.

This warrants a minor version bump since we're below 1.0.

This change is Reviewable

mkpankov commented 4 years ago

I see it fails for 2 reasons:

  1. On old Rust 1.31, there's an unstable feature used by newer bindgen. Is Rust 1.31 important for you for some reason?
  2. On new bindgen, there are warnings about u128 not being FFI-safe. This I'm looking into.
mkpankov commented 4 years ago

The reason 2. appeared is this issue, apparently.

I don't think we can fix it. I can only disable warnings for now.

tarcieri commented 4 years ago

Re:

Is Rust 1.31 important for you for some reason?

No, you can bump the MSRV

tarcieri commented 4 years ago

@mkpankov so this seems like the sort of stuff the RPM developers warned me about... I think until RPM5 it's just going to be very, very brittle for any sort of multithreaded access, and I was told the best way to use TransactionSets was one initialized as a sort of global context which lives for the lifetime of the program, and in a multithreaded environment, guard it with a mutex.

Any interest in trying the ctor approach to tearing down the global transaction set in a dtor on exit?

mkpankov commented 4 years ago

@tarcieri I don't need this functionality for my project for now, so I think I'm going to postpone it until that need arises again.

That's really a pity and I find it unclear why it's an issue to librpm at all, if two threads never access the DB simultaneously.

We could prohibit Send too - it's not hard at all, and it would prevent any sort of multithreaded access w/o global TransactionSet shenanigans. I'm just frustrated because I don't understand why we should do that.

Do you still have contacts of RPM developers? Do you think I could clear it up with them?

tarcieri commented 4 years ago

Do you still have contacts of RPM developers? Do you think I could clear it up with them?

@n3npq was the main one I talked to before, but he seems inactive now and deleted all of his previous comments.

@ignatenkobrain might be a good person to talk to. Otherwise I'd suggest making an issue on https://github.com/rpm-software-management/rpm/

ignatenkobrain commented 4 years ago

Sure, I'm ready to help. Feel free to join #rpm.org on freenode as well. Just tell me what you need help with :)

mkpankov commented 4 years ago

@ignatenkobrain Hi! I'd like your comments on two points:

  1. https://github.com/RustRPM/librpm.rs/pull/16#discussion_r353007877
  2. https://github.com/RustRPM/librpm.rs/pull/16#discussion_r353579109
Conan-Kudo commented 4 years ago

@mkpankov You'll want to rebase this pull request on current master. Also, could you squash your commits into more logically reasonable chunks? There's a lot of stuff happening here...

mkpankov commented 4 years ago

@Conan-Kudo what's the point if it doesn't work?

dralley commented 4 years ago

I'm not sure if anyone is aware of https://github.com/Richterrettich/rpm-rs but it might be a nice source of inspiration for APIs.

tarcieri commented 4 years ago

@dralley we're aware of it, and there's even an open issue suggesting they could use the librpmbuild-sys crate as an alternative backend, however note that rpm-rs is the equivalent of librpmbuild, whereas this project wraps librpm itself which provides access to the local RPM database

dralley commented 4 years ago

Thanks for the response and clarification, I bring it up only because of the bullet point in the readme for adding RPM builder API (i.e. librpmbuild), although I probably should have been more clear about that myself given the current more limited scope of the library