Ensure cider-jack-in fails gracefully when commands are not found

[radhikalism]

Problem: Running cider-jack-in with a Boot project when boot or boot.sh is not found in exec-path yields an error like this:

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  locate-file-internal(nil ("/usr/local/bin" "/usr/bin" "/bin" "/usr/local/games" "/usr/games" "/usr/lib/emacs/24.5/x86_64-linux-gnu") ("") 1)
  locate-file(nil ("/usr/local/bin" "/usr/bin" "/bin" "/usr/local/games" "/usr/games" "/usr/lib/emacs/24.5/x86_64-linux-gnu") ("") 1)
  executable-find(nil)
  cider--boot-present-p()
  cider-jack-in(nil)
  call-interactively(cider-jack-in record nil)
  command-execute(cider-jack-in record)
  execute-extended-command(nil "cider-jack-in")
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)

Expected: a more user-friendly message like this:
The boot executable (specified by cider-lein-command' or cider-boot-command') isn't on your exec-path'`

The source of the bug seems to be that cider-boot-command can be nil since it has a non-literal value, whereas cider--boot-present-p assumes it is non-nil. I've added a fallback literal value "boot" to guard against this causing problems.

Affects cider-20160501.1921 (melpa) and git master.

---

Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

• The commits are consistent with our contribution guidelines
• You've added tests (if possible) to cover your change(s)
• All tests are passing (make test)
• The new code is not generating bytecode or M-x checkdoc warnings
• You've updated the changelog (if adding/changing user-visible functionality)
• You've updated the readme (if adding/changing user-visible functionality)
• You've updated the refcard (if you made changes to the commands listed there)

[Malabarba]

I like this solution. IIRC, there are situations in which the exec-path might get set after our defcustom is defined, so it's a good idea to have a sane fallback.
@arbscht Please also document this as a bug fix in Changelog.md

[bbatsov]

Is there the same problem with lein as well?

[bbatsov]

Btw, I guess you've seen the code here, right - https://github.com/clojure-emacs/cider/blob/master/cider.el#L497

[radhikalism]

@bbatsov I don't think lein is affected. lein and gradle are defined as literal values, whereas boot is subject to change due to invoking executable-find. Compare:

(defcustom cider-lein-command
  "lein"
...)

(defcustom cider-boot-command
  (or (executable-find "boot")
      (executable-find "boot.sh")) ;; <-- this can be nil
...)

The nil value of cider-boot-command trickles down to cider-present-p and raises an error before the branching in L476 can proceed.

This fix adds a default fallback literal value so that if executable-find returns nil, the cider-present-p check will still evaluate properly and return nil, so that the existing error handling code path in L497 can happen.

[bbatsov]

I get what the fix does. I simply dislike we're doing different things for lein, boot and gradle. Maybe we should just remove executable-find. If someone uses boot.sh they'll just update the defcustom. @Malabarba thoughts?

[bbatsov]

If we do something like this the first check would be redundant. As I mentioned earlier - I'd rather simplify this to be just a constant to keep in aligned with other similar values.

[radhikalism]

If not searching for "boot.sh", then there's no need to search for "boot" at this point either. Simply setting "boot" should be enough as executable-find is called later with this value. However, I note that cider--{cmd}-present-p and cider-jack-in-command actually do different things to locate an executable — this seems like a bug.

[Malabarba]

Ok. I'm with changing it to just boot.

[bbatsov]

One more thing - the real place to fix the bug is the place where a nil can be passed to executable-find in the code I linked to. This should definitely be addressed as well. You can also add a simple unit test.

[radhikalism]

I agree with changing the value to just "boot", and let "boot.sh" users simply customize the variable, instead of magically nesting executable-find in multiple layers.

I'm not sure if the main code path needs to protect itself against such nils. It seems like a reasonable assumption that a custom variable is non-nil, given that it is typed 'string and Emacs enforces or interprets the type at set-variable and customize-variable. This is only unreliable in case of initializing the value to nil (as currently the case), but the problem really is that nil was used for a string type custom variable (no fallback).

[bbatsov]

Fine. Let's just change the default and wrap this.

On Thursday, 5 May 2016, Abhishek Reddy notifications@github.com wrote:

I agree with changing the value to just "boot", and let "boot.sh" users
simply customize the variable, instead of magically nesting
executable-find in multiple layers.

I'm not sure if the main code path needs to protect itself against such
nils. It seems like a reasonable assumption that a custom variable is
non-nil, given that it is typed 'string and Emacs enforces or interprets
the type at set-variable and customize-variable. This is only unreliable
in case of initializing the value to nil (as currently the case), but the
problem really is that nil was used for a string type custom variable (no
fallback).

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1733 (comment)

Best Regards,
Bozhidar Batsov

http://www.batsov.com

[radhikalism]

Here's a somewhat bigger change to normalize behaviour between the present-p check and the command search code paths. This allows custom variables for cider-{lein,boot,gradle}-command to be simple string command names; the resolved location can safely be nil; the error message if a command is not found shows the configured name of the missing command.

[Malabarba]

user-error