Refactor all backends - Githubissues

ripytide commented 5 months ago

Work in Progress, I'll summarize the changes after I get it compiling.

InnocentZero commented 5 months ago

Since this is close to completion (I believe), I'll wait for you to finish it off and @steven-omaha to merge it before starting my work on feature flags.

ripytide commented 5 months ago

Summary of Changes:

Simplified the Backend trait to the four fundamental methods and five associated types.
Switched the Packages type to statically typed variations PackagesIds, PackagesInstall, PackagesRemove and PackagesQuery to allow for per-backend types for each fundamental use-case.
Switched Group type to be based on the PackagesInstall type.
Refactored all backends to be based on the new Backend trait and refactored to use the run_args function.
Mass replaced "noconfirm" to "no_confirm" in the codebase since it is two words.
Restructured the workspace into a single crate since we no longer have multiple crates.
Removed non-critical functionality (Group commands, Package Search)
Split pip and pipx into separate backends as they have completely different cli's and as far as I can tell are actually separate package managers for separate package types (libraries vs applications)
Renamed all backends to be be named based on their underlying package manager rather than the name of the distribution that package manager is based on (excluding arch since the aur_helpers like paru and yay are all compatible with pacmand and so can be used synonymously (although maybe we could make them separate backends and not use a aur_helper config option which would be much more flexible to the user?))
Removed the arch and debian features and made the optional dependencies non-optional to allow run-time backend selection

Things temporarily removed:

Temporarily removed the review code and put a todo!() in place since it didn't make sense to fix it to work with the new types when it will soon be re-written using ratatui anyway.
Temporarily removed group file parsing code and put a todo!() in place as I believe this is soon to be changed by @InnocentZero and so it might make sense to refactor and change it all at once rather than first refactoring and then changing it separately. (Although I can refactor it back to the original functionality if so desired by @InnocentZero if that would make your task easier)

InnocentZero commented 5 months ago

* Temporarily removed group file parsing code and put a `todo!()` in place as I believe this is soon to be changed by @InnocentZero and so it might make sense to refactor and change it all at once rather than first refactoring and then changing it separately. (Although I can refactor it back to the original functionality if so desired by @InnocentZero if that would make your task easier)

Let it be todo!() s for now. I'll implement it from scratch as it seems like a good idea since we're mass-refactoring anyways.

InnocentZero commented 4 months ago

@ripytide Should I consider the PR done? Are the failing tests because of todo!() macros or something else?

@steven-omaha if I am to start working on feature flags should I use this or master as the base? I'm hesitant to use this since I'm not entirely sure how much work is left and master is in a usable state as of now.

ripytide commented 4 months ago

@InnocentZero I consider the PR done and ready for review. The tests pass on my machine but fail in CI due to CI currently expecting the arch and debian feature flags which are removed in the PR.

InnocentZero commented 4 months ago

Sounds good. I'll start working on my PR.

InnocentZero commented 4 months ago

@ripytide The build fails on my system. I'm using arch linux. I presume it is because of the absence of libapt. Can you check if that is the case? I presume that while removing feature flags you kind of made it necessary for developers to have the relevant libraries.

ripytide commented 4 months ago

Ah interesting, I didn't realize the two dependencies had non-rust build dependencies. In which case I can see three options:

Make the two dependencies optional again
Keep the dependencies non-optional
Remove the dependencies completely and go back to using the package manager CLIs

Pros of 1.

Allows pacdef to be compiled without the two packages' build dependencies

Cons of 1.

Creates multiple build configurations which makes packaging pacdef on platforms harder (like the AUR)

Pros of 2.

Keeps packaging simpler as only one build configuration

Cons of 2.

The build dependencies must be installed to build pacdef from source. (Though not from binary install, like pacdef-bin from the AUR.

Pros of 3.

Only one build configuration
All backends would be consistent (CLI parsing/interfacing)

Cons of 3.

Code for those backends may become more complicated as we would have to parse CLI output ourselves

ripytide commented 4 months ago

I'd be happy with any of the options, but I think I prefer option 3 the most.

InnocentZero commented 4 months ago

3 sounds good to me as well. pacman, and by extension paru support neat formats that are easy to parse (one package in each line). However, I've never used apt before. So I'm not sure if it supports formatting outputs.

@steven-omaha what is your opinion on all of this?

InnocentZero commented 4 months ago

With the recent thing going on with libgit2 on archlinux I'd now heavily recommend staying away from external libraries as much as possible. Had to manually reinstall all the cargo packages that needed libgit2 because libgit2 updated from 1.7 to 1.8 and all hell broke loose. Maybe it's a me thing, but better to not have any external dependence at all ig.

steven-omaha commented 3 months ago

Hello everyone, sorry for the long absence on my behalf. RL got seriously in the way.

From the mentioned options, my personal preference would be number 1, since it is the most reliable one. If it compiles, it will work. Option 3 can break in an unpredictable manner. If someone changes the output format of paru, pacman, apt oder dnf slightly, things could go wrong without a clear indication why. Yet, seeing that both of you prefer option 3, we can go with it for now.

Will take a look at the PR now.

steven-omaha commented 3 months ago

Okay, so I'm running into some problems here. Currently I cannot test / run this, because of a couple of todo!()s in crucial parts of the program (group loading). Reviewing this is difficult, because it rewrote everything that happens under the hood and it's just a lot of code that is currently in an unusable state.

It also touches a couple of unrelated things, like renaming --noconfirm to --no_confirm (which deviates from the way pacman and paru do this, apt uses --yes here), or not showing errors anymore when a package search yields no results (also pacman and paru behavior, not sure about the others). We could argue (or rather bike shed) about this, but these changes have no place in a refactor of the type system. You can leave this in for now. In the future just ask. Or open a different PR for this.

For the future, we need to find a better workflow for PRs that turn out this huge.

@ripytide Do you intend on continuing with this PR?

ripytide commented 3 months ago

Testing this at the moment will be difficult as the config parsing was removed rather than re-working the existing parsing code since it is soon to be re-done anyway as mentioned above. I don't think that should block the pr though, once the parsing code is written in a next PR we can start testing then.

--noconfirm to --no_confirm is indeed a flyby change that in an ideal world would have been done in a separate PR, I could go back and separate it out if you really want but I don't really see the point. The rationale for the change is that this is a rust code base and so I would expect it to follow rust's naming conventions in which separate words are separated by an underscore for readability, most rust cli's follow this convention I believe.

The error on no package found I have no strong feelings about, I can't quite remember why it doesn't fail on no package found anymore as this was quite a while ago now, we can change the behavior in another PR or I can update this one idm.

In future hopefully such large PR's won't be necessary as a lot of the scale of this PR comes from the amount of duplication of responsibility in the original code-base. I could also split the PR into one per back-end but really most of the changes are the same so it made more sense to me to just do them all together.

I do intend to continue with this PR as I believe it dramatically improves the code-base.

steven-omaha commented 3 months ago

I could go back and separate it out if you really want but I don't really see the point.

I agree with you. As I have written:

You can leave this in for now.

Just be mindful of this in the future. Thanks.

InnocentZero commented 3 months ago

Hi everyone, I apologise on a delay to respond from my side as well. Life has been unpredictably hectic for me, and would probably continue that way for the next couple of weeks. So I won't be able to rework the file parsing for now. I'll hopefully be completely back on track with this by the last week of July.

@ripytide I skimmed over the changes you mentioned earlier today and I think you got the pip/pipx part wrong.

It's true that pip can install libraries. But it's not recommended to do it unless you're in a venv. Apart from that, pip can also download binaries.

Except it's not recommended for that either since the corresponding libs can pollute the global space. Hence pipx using venvs for each install and having symlinks to PATH.

I'm not sure what the new changes exactly are as I don't have the time to go through all the commits to understand stuff going on, but I really believe both of them should be one backend. Their output format is reasonably similar (json with roughly similar output structure) and so is their functionality. No sane user would, in their right mind, use both.

@steven-omaha do you mind if I request you to have a pinned issue as a task tracker with assignees? This would also help in better roadmapping + prevention of such drastic codebase changes. Thanks.

ripytide commented 3 months ago

Thanks for the explanation on pip and pipx, I think I understand them more now.

While I agree with all you've said on the recommended usages of pip and pipx, I think there are still some fairly compelling reasons to keep them as separate backends despite them installing from the same packaging ecosystem.

Firstly, I think it's more discoverable than having a single backend and using a config option to toggle between the installer, as not all users are likely to read through all the config options whereas I'd imagine most users are very likely to list the available backends somehow (especially if we had a pacdef list-backends command or something).

There's also the naming issue if you combine the two clis into one backend, what do we call the backend? If we call it pip then users may get confused that it can actually use pipx, if we call it python then what happens when we want to add an alternative python package manager like uv?

Finally, there's also the code side of things. Although the two backends have mostly similar command line options, there are differences. For example pip uses pip list --format json whereas pipx uses pipx list --json to get a list of packages in json format. We could implement this by using ifs in the code everywhere the two clis differentiate but I find from a code quality standpoint it would be easier to read and maintain two separate backends. This also works in our favor if we ever want to provide custom options on a per-backend basis as there are many options supported by pipx but not pip and the same vice-versa.

ripytide commented 3 months ago

I'd say this argument also applies to pacman, yay and paru so I think I'd also be in favor of splitting those also.

InnocentZero commented 3 months ago

Hi everyone, so I got some free time as of now and would like to work to get this into a working thing, but as of now I cannot even compile it because of the absence of libapt. I can't even figure out a way to install it in arch as paru shows two outdated aur packages for apt, and neither of them is a C library.

As of now I'm commenting everything out that involves apt and then attempting to work on it.

InnocentZero commented 3 months ago

I'd say this argument also applies to pacman, yay and paru so I think I'd also be in favor of splitting those also.

The issue with that is how many such package managers would you support? Despite the fact that all of them have nearly the same syntax and mostly the same output as well. So maybe splitting pip and pipx makes sense due to different output structures, but idt I'll say the same for arch package managers.

ripytide commented 3 months ago

Hi everyone, so I got some free time as of now and would like to work to get this into a working thing, but as of now I cannot even compile it because of the absence of libapt. I can't even figure out a way to install it in arch as paru shows two outdated aur packages for apt, and neither of them is a C library.

If we're all in agreement with removing the package manager dependencies then we might as well do that now which would fix that issue. I can add some commits to do that if we'd like?

The issue with that is how many such package managers would you support? Despite the fact that all of them have nearly the same syntax and mostly the same output as well. So maybe splitting pip and pipx makes sense due to different output structures, but idt I'll say the same for arch package managers.

One potential use-case I can think of for separating pacman/paru/yay is what if someone had a personal preference to use pacman for non-AUR packages and paru for AUR packages. I don't think maintenance costs are too high since if their command structures are the same we can still create some utility functions that all three can use to prevent code duplication while still letting them appear as separate backends to users.

InnocentZero commented 3 months ago

Hi @ripytide , can you hold on to those commits for a moment? I'm adding rudimentary support for parsing group files and as soon as that is done, I'll probably push to this PR itself. I'm also adding some methods to PackageInstall and error types to pacdef::Error to make my life slightly simpler. Is this okay with you?

ripytide commented 3 months ago

@InnocentZero, that's absolutely fine by me. I'll add you to the branch so you can add any commits you'd like.

InnocentZero commented 3 months ago

@ripytide Have a look at the changes. There is still stuff to be done, but at the moment it is able to parse config files properly.

InnocentZero commented 3 months ago

A (nonexhaustive) list of breaking changes and regressions I found:

Backend names like rust now do not work anymore. They have instead been renamed to package manager equivalents like cargo. (A welcome change in my opinion)
review doesn't work atm. (we should have some basic interface present if ratatui is going to take time)
package subcommands have removed the package subcommand from them. So instead of pacdef package sync it's just pacdef sync now.
group subcommands are completely removed (#82 )
Explicitly installed flatpak runtimes are considered unmanaged (#86 )
Despite pipx being the package manager of python, pip also reports unmanaged packages. These were actually installed by pacman instead of pip and should not be picked up by pacdef. I probably need to explicitly disable it, but it was still very surprising for me tbh. This is probably just arch specific.
Rustup now uses full names instead of just the toolchain name and component name. I was probably the only one using it but it's a breaking change regardless.
unmanaged lists backends despite the fact that the backend doesn't exist/has nothing unmanaged. More visual clutter imho.

InnocentZero commented 3 months ago

Looks like 2.0 is gonna have a lot of breaking changes after all...

InnocentZero commented 3 months ago

One potential use-case I can think of for separating pacman/paru/yay is what if someone had a personal preference to use pacman for non-AUR packages and paru for AUR packages. I don't think maintenance costs are too high since if their command structures are the same we can still create some utility functions that all three can use to prevent code duplication while still letting them appear as separate backends to users.

This makes sense.

ripytide commented 3 months ago

@InnocentZero the changes look good to me, good spot on the IntoIterator change. FromString also looks good for rustup package ids.

InnocentZero commented 3 months ago

The latest commit converts the entire packages.rs to a macro based approach instead of manually hardcoding backends left and right. This makes it very easy for new contributors to add new backends that they can test. Simply implement the backend trait for the package manager of your choice and add the names to generate_structs macro in the bottom of packages.rs.

ripytide commented 3 months ago

Wow, that's much better, nice macro-fu.

Magniquick commented 1 month ago

What exactly happened here ? Could you tell me why was this closed ?

InnocentZero commented 1 month ago

What exactly happened here ? Could you tell me why was this closed ?

Ripytide and I are working on a separate fork which we'll hopefully merge upstream once it looks good. It has a lot of new features, including a different file format for group files.

See it here

Magniquick commented 1 month ago

Is it possible/stable enough to build and use right now ? (Finally got enough time to install arch : P)

ripytide commented 1 month ago

Unfortunately not yet, though it should be ready soon.

steven-omaha / pacdef

Refactor all backends #81