search

melpa / package-build

Tools for assembling a package archive
https://github.com/melpa/melpa
25 stars 34 forks source link

Do not run shell commands on load #24

Closed Fuco1 closed 5 years ago

Fuco1 commented 6 years ago

This defcustom runs shell commands when I load the file

(defcustom package-build-timeout-executable
  (let ((prog (or (executable-find "timeout")
                  (executable-find "gtimeout"))))
    (when (and prog
               (string-match-p "^ *-k"
                               (shell-command-to-string (concat prog " --help"))))
      prog))
  "Path to a GNU coreutils \"timeout\" command if available.
This must be a version which supports the \"-k\" option."
  :group 'package-build
  :type '(file :must-match t))

It takes about 0.4s on my machine. This file is loaded among others during cask bootstrap which makes it extremly slow: cask emacs --batch --eval '(kill-emacs)' takes 1 second, from which 2x0.4s is spent in the form above (cask calls emacs twice to get load path and exec path).

When I put (setq-default package-build-timeout-executable "/usr/bin/timeout") in cask-bootstrap.el that alone makes it 4 times faster to start up. Not requiring package.el would save another 2x0.1s for almost instant startup.

Is there anything that can be done here, i.e. lazily determining the binary instead of doing this during load time?

See https://github.com/cask/cask/issues/361

purcell commented 6 years ago

Is there anything that can be done here, i.e. lazily determining the binary instead of doing this during load time?

We could defer checking that the executable supports -k, or skip that check altogether. But it was added for a reason - most likely that the BSD/MacOS timeout doesn't support -k - so if we remove the check completely then this will lead to bug reports I'd have to deal with.

Fuco1 commented 6 years ago

What happens now if they have a version which does not support it? The form evals to nil and the defcustom would check that it's not valid file and fail? (I'm not sure what :must-match does).

Can we have this done during runtime lazily? I.e. a simple wrapper around this variable + a cache. I can do the refactoring. This would speed up cask startup 5 times by itself and it is not a very drastic trade-off to have a tiny bit of complexity here. Plus running binaries on load time probably isn't good form anyway.

Fuco1 commented 6 years ago

@purcell Hi Steve. Should I go ahead and make the change and PR? I would like not to waste time on this if it's something you would not accept then.

milkypostman commented 6 years ago

Could we change the check so that if the command fails then we check and then we can print a more informative error?

On Fri, Sep 14, 2018 at 06:48 Matus Goljer notifications@github.com wrote:

@purcell https://github.com/purcell Hi Steve. Should I go ahead and make the change and PR? I would like not to waste time on this if it's something you would not accept then.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/melpa/package-build/issues/24#issuecomment-421334234, or mute the thread https://github.com/notifications/unsubscribe-auth/AACUspuN7MdApkuEyneIPfBdPtm2Rd0Wks5ua5dygaJpZM4WXaNV .

purcell commented 6 years ago

I actually suspect that the best solution might be to disable the use of a timeout executable by default. It's there mainly for MELPA after all. So it might be that making this var default to nil would be the best fix.

purcell commented 5 years ago

I actually suspect that the best solution might be to disable the use of a timeout executable by default. It's there mainly for MELPA after all. So it might be that making this var default to nil would be the best fix.

^ I went ahead and did this.