archlinux / alpm.rs

Rust bindings for libalpm
GNU General Public License v3.0
112 stars 21 forks source link

API changes for v3 #21

Open Morganamilo opened 3 years ago

Morganamilo commented 3 years ago
fosskers commented 3 years ago

Why shouldn't it be Send? Do we want to guarantee only a single binding to the library at a time? Hm although stuff it in an Arc might do it, anyway.

Morganamilo commented 3 years ago

The reason was because the callbacks it now stores. But thinking about it they are all boxed so moving alpm into a new thread doesn't move the contents. Something I still need to reason over.

Morganamilo commented 3 years ago

Thinking about it more. Could could store something !Sync + !Send in the context and access it in another thread via the callbacks So it should not be Send.

I know you originally asked to make it send but I don't really see the need of it. You could always just pass the root and dbpath and init alpm in the other thread.

fosskers commented 3 years ago

And I think I was asking for that in my initial experimentations - I haven't needed it since.

Morganamilo commented 2 years ago

trans prepare (and commit) currently look like this:

pub fn trans_prepare(&mut self) -> Result<(), (PrepareResult<'_>, Error)>

With PrepareResult being:

pub enum PrepareResult<'a> {
    PkgInvalidArch(AlpmListMut<'a, Package<'a>>),
    UnsatisfiedDeps(AlpmListMut<'a, DependMissing>),
    ConflictingDeps(AlpmListMut<'a, OwnedConflict>),
    Ok,
}

This can be a bit awkward as you can't just handle.trans_prepare()? as PrepareResult doesn't implement Error or Display. It's also hard to pass with anyhow as due to storing the alpm lists the value is !Send and !Sync. It makes it a little awkward as you have to .map_err(|e| e.1) and you also lose some of the information.

Also PrepareResult should be named PrepareError.

It should probably be more like trans_add_pkg() which returns Result<(), AddError> . Store the error and the value and implement std Error and into alpm::Error. The function should also just hold a raw alpmlist and have a getter to turn it the error value.

Morganamilo commented 2 years ago

I'm considering inverting how the structs work for v3.

instead of a Pkg struct that contains a pointer it will be &Pkg that contains an alpm_pkg_t. Then like &str, &Pkg will only be holdable by reference. I think that may work better for some ergonomics.

Morganamilo commented 2 years ago

Unions can be done in a way that doesn't require us to match on them: https://github.com/rust-lang/rfcs/blob/master/text/2195-really-tagged-unions.md

Morganamilo commented 2 years ago

I got around to implementing all of this in https://github.com/archlinux/alpm.rs/tree/v3.

Only think not done is transmuting the unions to enums directly. As due to padding that would require putting all the fields in the enum variant directly and not a struct. Which then causes issues as I wish to keep them private.

Don't expect this any time soon as I don't see a new pacman any time soon and it also depend on an un merged patch.