Lacking abstraction

[DamienCassou]

I've just found your blog and this package. I was excited. When I saw there was no support for dnf I started writing one starting from the support for dpkg. Then I realized that it was a lot of code to duplicate from dpkg even though I was expecting to mostly only implement a list of strings of command line arguments (in the same way as https://gitlab.com/jabranham/system-packages/).

Why is there so much stuff to implement? Is there any abstraction missing? Why isn't helm-system-packages depending on system-packages?

[Ambrevar]

Hi Damien! Thanks for the feedback :D

In many ways you are right, Helm System Packages lacks a good abstraction. Reasons are mostly historical: when I started this package, I took over @thierryvolpiatto's two initial implementations for dpkg and portage. I wanted it to re-use the existing code base and to make sure everything was as uniform as possible between pacman, guix, xbps and other package managers to come.

Ensued the current code base: it was the easiest and most efficient to Get Things Done™. Now would be a good time for a massive refactoring, be it's a bit harder than it seems at first glance: if you give it a deeper look you'll soon notice that much of the code is very similar up to some tiny annoying details that make everything quite hard to factor.

The core issue here is that we need to parse the command outputs and they are all massively different. Some actions don't even map exactly across the various package managers (e.g. some actions don't exist in brew).

Regarding https://gitlab.com/jabranham/system-packages/: it only deals with commands, not with output parsing, so it's much easier to make a uniform abstraction. We could re-use system-packages.el in this package, but it would add a depedency for not much benefit: about 3-5 commands are needed per package and that's it. The rest of the job must be done by us.

If you've got some free time, it would be fantastic if you could contribute a dnf interface. It's pretty easy once you got the dnf output parsed. I suggest you start from pacman though, it's much more finished (I used to be a Arch Linux user but never really into Debian).

Let me know if you are down for it! Cheers!

[DamienCassou]

Please see #26 for a prototype. Initial thoughts on what is lacking:

• documentation on helm-system-packages--cache-set. Apparently, names and descriptions should be 2 strings, not "string buffers" as the current documentation suggests. Please also document what is the display-list parameter.
• I think each module (e.g., helm-system-packages-pacman) should not have any dependency on helm itself. Please let helm-system-packages.el provide some abstraction so modules don't have to call helm or helm-build-in-buffer-source.

[Ambrevar]

• --cache-set and display-list: right, I'll work it out.

• (require 'helm): I can recall there was a good reason to defer the require to the modules, but I can't remember why... :p I'll think about it. Maybe it's just wrong. As for the abstraction, I think you are right, I had been willing to factor all this but I've been waiting until the various modules were stable enough. I guess it's safe to proceed now.

[thierryvolpiatto]

Pierre Neidhardt [email protected] writes:

Some actions don't even map exactly across the various package managers (e.g. some actions don't exist in brew).

Instead of using :action slot use :action-transformer, like this you can use only one set of actions for all.

-- Thierry

[Ambrevar]

Didn't know about :action-transformer, thanks for the hint!

[Ambrevar]

I just remembered one strong argument why modules are not more factorized: simply because it's very hard to test and maintain by a single person. Testing a foreign package manager usually requires virtual machines, which can be costly and cumbersome to setup.

This is why we must be conservative and make sure every module works independently from the rest.

That said, we can work on factorization and move the modules to the new abstraction one by one.

[Ambrevar]

I've committed a draft of a package manager abstraction. See #27.

[Ambrevar]

#27 has been merged with some modifications. Let me know if there is anything else you'd like to see factored.

[DamienCassou]

Great job, thank you. Some more abstractions:

• I still see a lot of helm-related code that in my opinion should go away in modules. For example, helm-system-packages-guix-info is checking helm-in-persisten-action and calling helm-marked-candidate. Would it be possible that modules just define a function which takes a list of packages as arguments?
• For the info action, I suggest that such a function returns a list of package descriptions that helm-system-packages would insert into the right buffer.
• The helm actions could be predefined and modules would only specify associated functions if any:

(defvar helm-system-packages-guix
  (helm-system-packages-manager-create
   :name "guix"
   :actions '(
      :info #'helm-system-packages-guix-info)))

[Ambrevar]

• helm-marked-candidate: the problem is that Helm actions only expect one such candidate. The

                                        (if helm-in-persistent-action
                                            (list candidate)
                                          (helm-marked-candidates))
  

  block is rather idiomatic in Helm. I agree it would be nice if the interface accepted a list of candidates instead, but I'm not sure Helm System Packages should abstract that. @thierryvolpiatto What do you think?

• info actrion: Isn't it what helm-system-packages-show-information is already doing? Or do you mean something else?

• Helm actions: your solution sounds less flexible. What if a module wants to define a new type of action? Using a separate variable also lets the user define their own actions without redefining the whole manager interface.

[DamienCassou]

The [...] block is rather idiomatic in Helm

I agree but my whole point is about abstracting away from helm for 2 reasons:

• maybe tomorrow I would like to use ivy instead;
• implementing a module is already complicated enough without the need to understand helm's API

info actrion : Isn't it what helm-system-packages-show-information is already doing?

Not quite. Look at my PR #26. You will see a dnf-info function that is doing the dirty work I would like an abstraction to do instead. Now, look at dnf--info which is the only thing I would like to provide.

Helm actions: your solution sounds less flexible

you are right. This is always the case when you abstract things away. Moreover, this is probably the only way to implement the above (abstracting away from helm to get the selected packages as parameter).

What if a module wants to define a new type of action?

you could provide a :extra-actions that would be the equivalent of :actions right now. That way, you have both flexibility for those who need it and abstraction/simplification for most cases.

Using a separate variable also lets the user define their own actions without redefining the whole manager interface

this could be done at the level of helm-system-packages instead of per-module. This doesn't prevent the user from adding an action to a specific manager:

(helm-system-packages-add-manager-action "dnf" '("Cleanup dnf database" . ...))

[thierryvolpiatto]

Damien Cassou [email protected] writes:

The [...] block is rather idiomatic in Helm

I agree but my whole point is about abstracting away from helm for 2 reasons:

• maybe tomorrow I would like to use ivy instead;

So why developing a helm package? Once you have removed all the helm related code you endup with simple completing-read's without the benefit of all the helm features not present in other packages, also the "helm-" prefix of your package name have non sense once you have removed all helm related code.

-- Thierry

[Ambrevar]

@DamienCassou: I had initially misunderstood what you wanted to abstract. But you are right: the output processing of the various package managers should be abstracted away from the Helm interface. Much of the current implementation is due to laziness because no one expressed interest in an Ivy interface before.

That said:

• I'm not sure Ivy would be as suited as a package manager because as far as I can remember, Ivy does not handle multiple selections (correct me if I'm wrong) and the minibuffer's vertical limit makes it a bit less convenient to display long package lists.

• @thierryvolpiatto has a point: A big part of every manager's implementation is the Helm interface which is not really factorizable (e.g. keymaps, display, etc.). If we do factor Helm out of each modules, then the sum of the Helm part and the output filtering part will be significantly bigger. The interface between the two parts might be tricky to get right, considering how widely different the managers are (consider portage, pacman and guix).

Conclusion: It will take a significant effort to factor Helm aside and I think it makes little sense to go down that road unless someone is really interested in making an Ivy interface.

dnf-info

OK, it's not much but yes, we can abstract that away.

:extra-actions

Why separating actions at all? Isn't it simpler to provide a list of functions instead? This is what the current implementation does. Note that "actions" do not have a Helm-specific type, it's just an alist of (DESCRIPTION . FUNCTION). Why not sticking to that? Maybe I'm misunderstanding what you are trying to achieve here.

this could be done at the level of helm-system-packages instead of per-module.

OK.

[DamienCassou]

I'm not sure Ivy would be as suited as a package manager

that was not my point, sorry. My point was that if modules depend less on helm and stay focused on the package manager they support, the developers life will be simpler when developing a new module or when the completion interface changes.

Why separating actions at all? Isn't it simpler to provide a list of functions instead?

you may, but this approach has some drawbacks:

• each module has to specify a description and keybindings separately instead of having both shared across all modules
• each module has to take care of the post-processing (e.g., calling helm-system-packages-show-information)

This is more code to copy-paste which means it will be harder to maintain every module.

[Ambrevar]

Some of the Helm specific code found in modules would not be shorter with an abstraction. A good example is actions: with an abstraction, you'd need to provide a list of functions, which is what Helm does anyways. Adding an abstraction layer here would be duplicating code.

I thought about using shared keybindings but in practice they are too different I'm afraid.

I'm not sure what you have in mind when it comes to abstracting the actions. Could you provide an example?

[DamienCassou]

I'm not sure what you have in mind when it comes to abstracting the actions. Could you provide an example?

I provided one above. Here is it again:

(defvar helm-system-packages-guix
  (helm-system-packages-manager-create
   :name "guix"
   :actions '(
      :info #'helm-system-packages-guix-info)
  :extra-actions: helm-system-packages-guix-extra-actions)) ; <- if needed

[Ambrevar]

Yes, but I don't understand what

(defvar helm-system-packages-guix
  (helm-system-packages-manager-create
   :name "guix"
   :actions '(
      :info #'helm-system-packages-guix-info)
  :extra-actions: helm-system-packages-guix-extra-actions)) ; <- if needed

improves over

(defvar helm-system-packages-guix
  (helm-system-packages-manager-create
   :name "guix"
   :actions (list helm-system-package-guix-actions helm-system-packages-default-actions))

with helm-system-packages-default-actions being anything that can be factored.

[DamienCassou]

This looks good as well. Can you please tell me how helm-system-packages.el would define helm-system-packages-default-actions? Using well known names such as `(intern (concat "helm-system-packages-" manager "-info")) would work quite fine and would simplify even more what I suggested above. The only downside is things becomes a big magic. More documentation is needed so module developers don't have to guess what to implement.

[Ambrevar]

Hmm, looks like I wrote too fast, I don't see anything that can be factored :(

Magic names with intern are one way to go, but as you said it's a bit magic and I think it's better to avoid it. The last factorization was one step in that direction: it avoided one intern.

So if we don't go by magic names, then, regardless of the interface, the action-specific functions will have to be glued to the interface at some point. So I'm not sure what your suggestion simplifies.

Currently:

(defcustom helm-system-packages-guix-actions
  '(("Show package(s)" . helm-system-packages-guix-info)
    ("Install" . helm-system-packages-guix-install)
    ("Uninstall" . helm-system-packages-guix-uninstall)
    ("Browse homepage URL" . helm-system-packages-guix-browse-url)
    ("Find files" . helm-system-packages-guix-find-files)
    ("Show dependencies" . helm-system-packages-guix-show-dependencies)
    ("Show reverse dependencies" . helm-system-packages-guix-show-reverse-dependencies))
  "Actions for Helm guix."
  :group 'helm-system-packages
  :type '(alist :key-type string :value-type function))

(defvar helm-system-packages-guix
  (helm-system-packages-manager-create
   :name "guix"
   :refresh-function #'helm-system-packages-guix-refresh
   :dependencies '("guix" "recsel")
   :help-message 'helm-system-packages-guix-help-message
   :keymap helm-system-packages-guix-map
   :transformer #'helm-system-packages-guix-transformer
   :actions helm-system-packages-guix-actions))

Your suggestion:

    (defvar helm-system-packages-guix
      (helm-system-packages-manager-create
       :name "guix"
       :refresh-function #'helm-system-packages-guix-refresh
       :dependencies '("guix" "recsel")
       :help-message 'helm-system-packages-guix-help-message
       :keymap helm-system-packages-guix-map
       :transformer #'helm-system-packages-guix-transformer
       :actions '(
          :info #'helm-system-packages-guix-info
          :install #'helm-system-packages-guix-install
          :uninstall #'helm-system-packages-guix-uninstall
          :browse-url #'helm-system-packages-guix-browse-url
          :find-files #'helm-system-packages-guix-find-files
          :show-dependencies #'helm-system-packages-guix-show-dependencies
          :show-reverse-dependencies #'helm-system-packages-guix-show-reverse-dependencies)
      :extra-actions: helm-system-packages-guix-extra-actions)) ; <- if needed

[DamienCassou]

I'm not sure what your suggestion simplifies

I think it simplifies 2 things:

1. helm action functions are now defined by helm-system-package directly. What each module defines is a function whose interface is imposed by helm-system-package to do only what is specific to the package manager itself. For :info, this means

• helm-system-package is responsible for (1) converting helm candidates into a list of packages, (2) pass this list to the module-specific function (e.g., helm-system-packages-guix-info), (3) write the result to a dedicated Org buffer through helm-system-packages-show-information.
• each module is now responsible for transforming a list of packages passed as argument into a list of Org-formatted strings

2. modules are not responsible for defining the helm actions themselves which means:

• abstracting away from the helm API: modules won't have to change if helm's API changes, module authors don't have to learn helm's API, ...
• define one set of descriptions and keybindings instead of asking modules to copy/paste

I feel that I'm repeating myself a lot. If you are not convinced by above arguments, I suggest we agree to disagree and move on :-).

[Ambrevar]

OK, I understand what you mean. Sorry for the previous confusion. I've had a similar architecture in mind at the beginning, but that was without counting on the existing code and the friction that arises when maintaining the various modules. Since I've been using only pacman and now guix, I did not feel the effort was worth the gain considering I could update only one package manager to a new interface (while dpkg and portage were stuck to the old one).

The road we are going down now is to move dnf and guix to a new architecture while we must keep the old. It's not pretty, and the changes you are suggesting are going to double the code base. We would also have to warn newcomers to only refer to dnf and guix: if they don't, they will be heavily confused.

What we would ideally need is a test suite embedding various pre-recorded outputs (info, search, etc.) so that the modules can be safely worked on without having the actual package manager at hand. From there, we could safely change the interface to something more data-centric as you suggest.

Any idea on how to produce those output recordings without access to the package manager?

[DamienCassou]

Sorry for the previous confusion

no need to apologize. It's at least half my fault :-).

What we would ideally need is a test suite embedding various pre-recorded outputs

that would be a really good start. If you want to go the integration test route, you could also test your package with several docker images, each built with a different OS. Gitlab has a CI supporting that by default.

Any idea on how to produce those output recordings without access to the package manager?

you can easily get access to any package manager you want through virtualization. If you give me a list of package-manager commands, I will execute them for one or two package managers.

[Ambrevar]

Absolutely, but I don't have the resources to virtualize anything at the moment. You can find the list of commands by looking up the "process-file" and "call-process" functions.

Continuous integration would also be welcome of course.

[DamienCassou]

I found another reason for abstraction: if modules are kept independent from helm, helm-system-packages could use multiple modules at the same time: e.g., if the user has both dnf and guix, M-x helm-system-packages would show a list of packages coming from both package managers.

[Ambrevar]

At the moment it should already be possible to run helm-system-packages over dnf and guix separately.

As to displaying both of them at the same time... I guess this would work in different sources. Is that what you were thinking? If so, I think it's a good idea.

[DamienCassou]

Pierre Neidhardt [email protected] writes:

As to displaying both of them at the same time... I guess this would work in different sources. Is that what you were thinking? If so, I think it's a good idea.

you could get a list of all packages from both managers, each one in a separate helm section (I don't know Helm's vocabulary, sorry). The package is not there yet, but it could become reasonable with the refactorings I suggest.

-- Damien Cassou http://damiencassou.seasidehosting.st

"Success is the ability to go from one failure to another without losing enthusiasm." --Winston Churchill

[Ambrevar]

Yes, we talk about the same thing: "source" is a section in Helm's parlance.

[Ambrevar]

@DamienCassou: It's been a while! In the meantime I've worked on a similar universal package manager abstraction for Nyxt, this time starting from scratch. The abstraction should be much better, plus it has support for functional package managers like Guix and Nix.

https://nyxt.atlas.engineer/article/release-2-pre-release-4.org

[DamienCassou]

Good job!