Fix #165 by reusing existing pacman logic

raphCode commented 3 years ago

I already explained the problem in the issue #165, so I tried to fix it, too:

Basically, I removed everything in action_builddir() after the tarcheck and replaced it with a call to pacman::ensure_aur_packages_installed() with the full path to the built archive file. This way existing code paths gets reused and the logic in action_builddir() becomes easier.

If I screwed up and overlooked why a custom terminal loop in action_builddir() was necessary, please tell me. No need to spend time to find gentle words in that case, I can take it :)

This is my first Rust contribution, and I want to learn Rust, so if anything can be even slightly improved, I am interested to hear.

raphCode commented 2 years ago

@vn971 did you find time to review this yet? :)

vn971 commented 2 years ago

@raphCode hey! Fair, fair. I need to return to this; been a bit stressful lately. I'll try to take another look this weekend

vn971 commented 2 years ago

Thanks for the PR and the issue's research contribution!

From one perspective, I'm super happy that a potential duplication was noticed, and a solution was proposed.

On the other hand, I see some shortcomings to the current approach:

In the proposed PR, data from srcinfo is zip-ed with a collection of built archive files. I'm afraid that's likely not correct for pkgbase-es with multiple packages in them. We need to do something different?
The old code and its replacement function slightly differently. The new code with ensure_packages_installed just wants to ensure that packages with certain "names" are installed. The old code wants to have this particular build to be installed, even if e.g. the same package is already installed, maybe even with the very same version.

To solve the above two, I'm wondering if we can use some illegal package name such as _ as for ensure_packages_installed to make sure that the package file is definitely installed

I don't like that it's a hack
but then on the other hand, we re-use some code, which is good (less code = good as far as I'm concerned:)

This is my current thinking, didn't decide what to do yet

vn971 commented 2 years ago

UPDATE: oh, the first remark about zip is incorrect/irrelevant, as the archive_names variable itself is derived from the very same srcinfo.pkgs. Maybe the code can be transformed to not use this "hidden knowledge" from above and be constructed directly though

vn971 commented 2 years ago

@raphCode I've created a commit that tries to address this, thoughts? https://github.com/vn971/rua/pull/186

vn971 commented 2 years ago

Thanks for the pull request! I've effectively merged it

The only thing I've changed a little bit before merging is using a more explicit map operation on a collection, instead of mapping the same collection twice independently and zip-ing it back [implicitly relying on the same length of it].

raphCode commented 2 years ago

Since you already merged, I only glanced through your mentioned shortcoming, but I think I understand it. Maybe I thought about that as well back then, but I don't really remember. It seems we came to the same conclusion at least!

I am glad that you were able to improve and merge it, good catch that the whole map and zip can be simplified!

vn971 / rua

Fix #165 by reusing existing pacman logic #173