helm-system-packages-eshell bugs

[cloudchin]

Hi, thanks for the package. Using helm for this type of querying is a magical experience. Installation, however, is pretty buggy at least on my setup. Using it twice within the same emacs session, I experienced bugs on both tries.

Setup:

• Fedora 30 (dnf)
• System distributed Emacs 26
• Melpa & package.el based helm setup, also w/ xterm-color.el.

Repro

M-x helm-system-packages RET <<a_package_to_install>>... select/navigate to it... f2 <<your_sudo_password>> RET

At this point, it shows the typical dnf pre-install summary and prompts for confirmation but misplaces the cursor by a single Tab character (see attached output). Continue anyways, pressing y RET like you normally would. When it fails (presumably due dnf's sanity check vs extra TAB) and the prompt appears again, press y RET to successfully install.

Now keep the *helm-system-packages-eshell* buffer, C-x b switch away and do some work. Later repeat above workflow to install another package. This time the install action fails completely returning run-hooks: Symbol’s function definition is void: helm-system-packages-refresh.

Inspecting helm-system-packages-refresh inside the *helm-system-packages-eshell* buffer confirm it is nil, and seems helm-system-packages--make-init has some missing related TODOs. xref reviewing calls to helm-system-packages-refresh did not make it clear how your package maybe effecting the associated eshell session at run-time though besides sending the initial constructed command.

Help? See also eshell session output: helm-system-packages-eshell_20190830.txt

[cloudchin]

Also, stepping back a bit, it seems crazy dicey to use eshell for system package management. Are there potential less-complex and more stable alternatives that could be used?

[Ambrevar]

Glad you find it magical, I tend to agree! :)

Sorry about the bad experience with dnf. I haven't worked on the dnf package myself, @DamienCassou contributed it, so he might know better what's going on. It's always a bit hard to test this Emacs package as a whole because of the many different systems that are need + because of architectural issues (see issue #25).

• For the tabbing issue, what happens if you issue the same command manually in an Eshell buffer? Do you get the same issue? Then it's probably an issue with dnf. We could always remove the confirmation or add our own confirmation as a workaround.

• Regarding the refresh function: I believe this happened because the previous refresh command failed. If you look at helm-system-packages-call-as-root you'll see these lines:

        (add-hook 'eshell-post-command-hook 'helm-system-packages-refresh nil t)
        (add-hook 'eshell-post-command-hook
                  (lambda () (remove-hook 'eshell-post-command-hook 'helm-system-packages-refresh t))
                  t t)

So it seems that somehow your local eshell-post-command-hook didn't get cleaned up. Maybe that happened because helm-system-packages-dnf-refresh raised an error. What happens if you enable toggle-debug-on-error before running this recipe for the first time?

• Choice of Eshell: The issues you've mentioned might not be related to Eshell. So is there something that bothers you about Eshell in particular? I'm not sure if Eshell is more or less complex than, say, Bash or Zsh, and it seems good enough for running simple commands like we do here. That said, no problem with using an alternative, we can always make it an option: it's quite easy, all we need to do is to abstract away the call site for Eshell and let the user configure what to call instead. As for the alternatives, I can see only one: Emacs' shell. Patches are welcome! :)

[cloudchin]

Maybe what's throwing me off is that executing actions on helm candidates does not usually entail post execution interaction to "complete" the executed action. eshell, as one of the newest terminal packages, is a 15 on a 1-10 scale for handling a confirmation prompt lol. These are dangerous operations the user needs to be protected from, but still... I don't usually see comint-based process calling. I love the idea of unified confirmation, too, which allows using --yes or equivalent.

helm-grep-ag-init from helm-grep looks close. There's a similar and simpler setup in deadgrep I liked too but maybe not as applicable due to helm. Thoughts?

For the tabbing issue, what happens if you issue the same command manually in an Eshell buffer? Do you get the same issue? Then it's probably an issue with dnf. We could always remove the confirmation or add our own confirmation as a workaround.

In a manually called eshell, I was prompted a second time again (which again, worked). I stayed in this eshell session sudo dnf install <...> sudo dnf remove <...>'ing a bunch of times and wasn't able to get the bug to come back. Worked on first call. Also--I also tested the install action in shell-mode and the prompt works just fine, surprisingly.

I am not very good at stack tracing/debugging yet, but will try and come back @ your second bullet. Thank you.

[Ambrevar]

I'm very confused with what you are trying to say :p

Why is Eshell interaction throwing you off? What does it have to do with Helm? What's dangerous? What's the link with comint' (Eshell does not usecomint')?

What's the link with helm-grep-ag-init?

My hunch is that you are saying you don't want to run interactive commands in Eshell. Then we can very certainly pass some --no-confirm command line option to dnf. That would fix it.

[cloudchin]

I don’t want to use eshell when a safer and lighter alternative calling method exists — I believe one exists.

Above are refs to functions which don’t use eshell; in fact, I grepped my elpa directory and can’t find a package using eshell to call external processes.

Apt —no-confirm is —yes in dnf, so I think we’re getting at the same thing.

[cloudchin]

I like your idea to build in the confirmation prompt—you could make asynchronous external calls then.

Going along, if we’re afraid that we’ll take away ability for users to see the installed dependencies, then we should support that—they shouldn’t be using the install command to begin with for dependency discovery. Apt/dpkg and dnf support looking up package dependencies.

[Ambrevar]

Emacs can call external process with call-process. That's also what Eshell does under the hood.

The reason Eshell is used by Helm System Packages is for a specific purpose: to allow "shell interactivity" during the execution of the installation. Among others, this allows for cancelling the operation, looking at the log, stacking logs of multiple commands. In other words, do what a shell does. So it makes a lot of sense to use Eshell (or Emacs shell) here. Switching to call-process would be a significant loss in features.

if we’re afraid that we’ll take away ability for users to see the installed dependencies

This is done by querying the package manager, and it's already supported by Helm System Packages. Look at Helm's action menu on a package, you can list the dependencies from there.

[DamienCassou]

I always have an error message when installing a package:

Debugger entered--Lisp error: (void-function helm-system-packages-refresh)

I never cared enough to have a look at it. I always kill the `helm-system-packages-eshell buffer after having used it so I can use it again later.

I never had the tab problem you mention. I just press M-> to go to the end of the buffer and press y RET to validate.

[cloudchin]

Letting the issue settle for the last ~two weeks I want to step back.

I don't think making a sweeping change like throwing out eshell makes sense since it mostly works, and there are a lot of library dependencies that can collectively have a much bigger impact on UX from a maintenance point. dnf is not mature yet (for example) and isn't supported by a stable library or documentation from what I can tell.

Props @ on bringing all this together--helmized package-querying is very powerful. Closing for now until I can contribute when I have more time. It's on my list.

[Ambrevar]

Thanks for the heads up!