Add el-get dependency to Package-Requires header

[aplaice]

Since el-get 5.1 (the latest) was released in 2014, it's probably a reasonable minimum version. el-get releases: https://github.com/dimitri/el-get/releases

See also: melpa/melpa#5341

Thanks for the package!

[coveralls]

Coverage remained the same at 29.6% when pulling e4c49f2 on aplaice:develop into 699d5aa on edvorg:develop.

[edvorg]

Hi @aplaice el-get functionality is no longer provided by req-package. If you'd like to update el-get version please do it in https://github.com/edvorg/use-package-el-get

[aplaice]

OK, I admit that I'm a bit confused. It appears (to me) that el-get is a dependency of req-package — req-package.el requires el-get, there's even a line to install el-get if it's not installed ((req-package-bootstrap 'el-get)) and README.org refers to an :el-get keyword.

Adding (el-get "5.1") to the Package-Requires: header would tell MELPA that el-get is indeed a dependency of req-package, as has been done for use-package, dash, log4e and ht. Strictly speaking this isn't necessary due to the (req-package-bootstrap 'el-get) line, but it has been done for the other packages, and is conventional (see the Emacs manual).

Regarding the minimum version of el-get that req-package depends on (if I'm not completely wrong regarding the fact of the dependence), I picked "5.1", as a) it's the latest, but b) has been around for > 3 years, so everyone should already have it. A version of "0" could also probably be specified (i.e. add (el-get "0")).

[DarwinAwardWinner]

In my Emacs config, somehow req-package is failing to load because it can't load el-get. I don't know why it's happening. Perhaps req-package-bootstrap is installing it but not adding it to load-path? Perhaps require forms are evaluated first when loading a compiled elc file? Whatever the case, I think it should be solved by adding el-get to Package-Requires, or modifying the code so that it does not unconditionally require el-get.

[aplaice]

Ping?

Starting with a fresh .emacs.d, adding MELPA to package-archives, installing req-package via MELPA and evaluating (require 'req-package), will result in el-get being installed (present in ~/.emacs.d/elpa/el-get-xxxxx/), so for all intents and purposes el-get is a dependency of req-package. Adding el-get to Package-Requires: would just inform MELPA of that fact (and help avoid breakage like that described above).

[edvorg]

Hi, sorry, merged it 👍

[aplaice]

Thanks!

[DarwinAwardWinner]

Ok, so it turns out that you can't actually declare a dependency on the el-get MELPA package, because this package is just for bootstrapping and loading it causes it to uninstall itself and install el-get outside of MELPA instead. This causes problems when other packages depend on it, as seen in dimitri/el-get#2618.

I think the way to handle this is to drop the el-get dependency, wrap-all code that uses el-get in something like (if (require 'el-get t) (progn ...) (error "Please install el-get"), and then maybe call (package-install 'el-get) as a convenience.

[aplaice]

Sorry for introducing the bug! The very particular nature of el-get's MELPA installation is not really documented in el-get's README.

The el-get dependency should indeed be removed from req-package's Package-requires: header and hence my commit fully reverted.

I'm not sure what would be the best way of resolving the previously existing bug reported above by DarwinAwardWinner, though.

(@DarwinAwardWinner: briefly looking at your config, it might be the case that your issue was caused by the fact that you use quelpa (rather than package.el fetching from MELPA), to install your packages. This might mean that req-package-bootstrap (which installs packages with package.el) could not install el-get — (AVAIL (assoc package ARCHIVES)) would return nil, and (package-install package) would not run.)

AFAICT the two possibilities are:

1. Leave the code alone (other than reverting my commit) and not worry too much about the rare case where three different package managers (package.el, quelpa and el-get) interact.

2. Replace (req-package-bootstrap 'el-get) with a function that installs el-get directly from source (à la https://github.com/dimitri/el-get/blob/e065feaa545087dd49f690a838237fe6239b00f6/README.md#basic-setup) if (require 'el-get) fails. (Probably over-kill, but if it's something desired, I could try to do it.)

[DarwinAwardWinner]

I think the most practical solution is to rewrite the req-package code such that el-get is an optional dependency. If it can be loaded, then the el-get related features work, otherwise they throw errors/warnings as appropriate.

[edvorg]

Hey @DarwinAwardWinner @aplaice I made a dependency optional

[DarwinAwardWinner]

Thanks for taking the time to work on this so quickly! I made some comments on the commit.

[edvorg]

@DarwinAwardWinner @aplaice do we really need el-get being enforced in req-package now that we have https://github.com/edvorg/use-package-el-get which is basically functionality extracted from req-package
I personally vote to completely remove everything related to el-get from req-package

[edvorg]

(replying to commit comment)

[edvorg]

Here is example how I set it up in my configuration https://github.com/edvorg/emacs-configs/blob/master/init-real.el

[aplaice]

@DarwinAwardWinner <https://github.com/DarwinAwardWinner> @aplaice

<https://github.com/aplaice> do we really need el-get being enforced in req-package now that we have https://github.com/edvorg/use-package-el-get which is basically functionality extracted from req-package

I personally vote to completely remove everything related to el-get from

req-package That's actually by far the best solution (especially as `use-package` handlers do definitely "belong" in a separate package and having duplicate code is not really necessary), so to the extent that I get a vote, I'm also in favour of the removal. Thanks!

[DarwinAwardWinner]

I personally vote to completely remove everything related to el-get from req-package

That's reasonable. But now the same problem still needs to be fixed in use-package-el-get. I think the best way is to do something like:

(defun use-package-handler/:el-get (name-symbol keyword archive-name rest state)
  "use-package :el-get keyword handler."
  (unless (require 'el-get nil t)
    (error "You must install el-get to use the `:el-get' keyword in `use-package'."))
  (let ((body (use-package-process-keywords name-symbol rest state)))
    ;; This happens at macro expansion time, not when the expanded code is
    ;; compiled or evaluated.
    (if (null archive-name)
        body
      (el-get-install archive-name)
      body)))

You can also add (eval-when-compile (require 'el-get nil t)) somewhere at the top level, so that the byte-compiler will be happy as long as el-get is installed, but won't error if it isn't.

(On an unrelated note, I'm not super familiar with how use-package works, but calling el-get-install at macro-expansion time seems odd to me. Wouldn't that mean that a pre-compiled init file won't install the package?)

[aplaice]

That's reasonable. But now the same problem still needs to be fixed in use-package-el-get

I'm not sure if that's necessary. IMO it's not unreasonable to expect somebody who wants to use use-package :el-get to have actually installed el-get. To look at the (comparable) case in quelpa-use-package, there is no special loading code of quelpa. Also, I'm not sure if the suggested error message is significantly more informative than the one directly produced by a failed (require 'el-get).