Warn on missing password executable

[aartaka]

Description

This warns the user if Nyxt cannot find the executable for their password manager, instead of erroring down the line.

Discussion

It uses :around method, which might be dangerous. But then, password-mode is all about hijacking into password-interface and making it more considerate, so it's aligned with the general approach.

Warning in any other place is not feasible, because it's either too early (in password-manager, because it mustn't know anything about Nyxt or user config) or too late (in password:execute and uiop:*-program).

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

• Git hygiene:
  • I have pulled from master before submitting this PR
  • There are no merge conflicts.
• I've added the new dependencies as:
  • ASDF dependencies,
  • Git submodules,

    cd /path/to/nyxt/checkout
    git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser

  • and Guix dependencies.
• My code follows the style guidelines for Common Lisp code. See:
  • Norvig & Pitman's Tutorial on Good Lisp Programming Style (PDF)
  • Google Common Lisp Style Guide
• I have performed a self-review of my own code.
• My code has been reviewed by at least one peer. (The peer review to approve a PR counts. The reviewer must download and test the code.)
• Documentation:
  • All my code has docstrings and :documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)
  • I have updated the existing documentation to match my changes.
  • I have commented my code in hard-to-understand areas.
  • I have updated the changelog.lisp with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).
    • Changelog update should be a separate commit.
  • I have added a migration.lisp entry for all compatibility-breaking changes.
  • (If this changes something about the features showcased on Nyxt website) I have these changes described in the new/existing article at Nyxt website or will notify one of maintainters to do so.
• Compilation and tests:
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works. (If possible.)
  • I ran the tests locally ((asdf:test-system :nyxt) and (asdf:test-system :nyxt/gi-gtk)) and they pass.

[aadcg]

This PR is related to #3213 and addresses it partially, but it doesn't close it so I've edited the top post. I'm handling the remaining tasks of that issue.

---

@aartaka I confess that this is not the solution I had in mind. And your description also seems to communicate that a different approach is desirable.

I think we should catch the error at execute. But we need to catch it differently depending on whether we use uiop:run-program or uiop:launch-program. (uiop:run-program "tomato") returns exit code 127 so that's easy. In the other case, we can fetch the exit code via (uiop:wait-process (uiop:launch-program "tomato")). What do you think?

Please ignore my comment above. The issue that we're trying to solve is that sera:resolve-executable may return NIL. But then execute from libraries/password-manager/password.lisp should handle it. If there's no executable, then there's nothing to execute.

While both uiop:run-program and uiop:launch-program handle the case when they're passed a NIL value, an error should be raised when (executable interface) returns NIL.

[jmercouris]

Is there something I am missing, would the following work?

(defgeneric execute (interface arguments &rest run-program-args &key wait-p &allow-other-keys)
  (:method ((interface password-interface) (arguments list) &rest run-program-args &key (wait-p t) &allow-other-keys)
    (when (executable interface)
      (apply (if wait-p #'uiop:run-program #'uiop:launch-program)
             (append (uiop:ensure-list (executable interface)) arguments)
             (alexandria:remove-from-plist run-program-args :wait-p))))
  (:documentation "Execute the command matching the INTERFACE, with ARGS.

`uiop:run-program' is used underneath, with RUN-PROGRAM-ARGS being its
arguments.

When the WAIT-P is NIL, `uiop:launch-program' is used instead of
`uiop:run-program'."))

@aadcg @aartaka ?

[jmercouris]

Of course we would then have to put guards in Nyxt to see if a function returns nil...

[aartaka]

Okay, another shot at a better design in dd2f0da. I haven't yet added the user-facing side of it, focusing on the right place for error signalling. Looks alright?

[aadcg]

The strategy is in line with what I had in mind. I still have to test it. Thanks.

Note the this PR needs to be rebased onto master.

[aartaka]

Note the this PR needs to be rebased onto master.

Done ✅

[aadcg]

In my understand the PR, as it stands, doesn't answer to the issue at hand.

Populate the config file with the snippet below and ensure that keepassxc isn't installed in the system, meaning that (password:executable (nyxt/mode/password:password-interface (current-mode :password))) returns nil. However, commands such as copy-password do not catch the fact that no executable was found.

(define-configuration nyxt/mode/password:password-mode
  ((nyxt/mode/password:password-interface (make-instance 'password:keepassxc-interface))))

[aartaka]

It errors on me with this piece of config (ensuring NIL value):

(define-configuration :password-mode
  ((password-interface (make-instance 'password:keepassxc-interface :executable nil))))

[aadcg]

And what conclusion can be drawn?

[aartaka]

Commands that use password interface will get an error and warn the user about it. Good pattern? Good pattern.

[aadcg]

Commands that use password interface will get an error and warn the user about it.

As explained above, I don't think that this assertion is correct. Did you try the recipe?

[aartaka]

I tried it with the snippet I provided, which should be equivalent to the environment you described. I've also tried to reproduce on binary build. This is what I got:

What exactly looks off to you?

[aadcg]

I'm taking over this @jmercouris.