alpm.rs icon indicating copy to clipboard operation
alpm.rs copied to clipboard

API changes for v3

Open Morganamilo opened this issue 4 years ago • 8 comments

  • Some getters should return Option #19
  • FileList::contains has unneeded Result<>
  • Alpm should not be Send
  • Rename CommitResult::FileConflict -> CommitResult::Fileconflicts
  • Rework Commit/Prepare Result to be lazy?
  • Add error and Commit/Prepare Result should be more consistent with eachother
  • Package::files() should be an option. ~~No it's fine as is.~~ Actually no
  • Remove handle needed to make an alpmlistmut and remove lifetimes.
  • &mut self for string setters

Morganamilo avatar Jun 05 '21 06:06 Morganamilo

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.

fosskers avatar Jun 05 '21 14:06 fosskers

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 avatar Jun 05 '21 16:06 Morganamilo

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.

Morganamilo avatar Jun 05 '21 16:06 Morganamilo

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

fosskers avatar Jun 05 '21 16:06 fosskers

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 avatar Oct 14 '21 15:10 Morganamilo

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 avatar Dec 04 '21 19:12 Morganamilo

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 avatar Dec 07 '21 11:12 Morganamilo

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.

Morganamilo avatar Dec 23 '21 15:12 Morganamilo