bug#60418: [PATCH] Add :vc keyword to use-package

[Philip Kaludercic <philipk@posteo.net>]

Tony Zorman <soliditsallgood@mailbox.org> writes:

> Hi,
>
> attached are the updated patches; sorry that this took so long.

No need to worry about that.

> From 2b5f99fcaec6c8f985c60ed3ff5e119a7fb58e39 Mon Sep 17 00:00:00 2001
> From: Tony Zorman <soliditsallgood@mailbox.org>
> Date: Thu, 29 Dec 2022 11:05:04 +0100
> Subject: [PATCH 1/2] Add :vc keyword to use-package
>
> * lisp/use-package/use-package-core.el (use-package-keywords): Add :vc.
> (use-package-handler/:load-path): Insert 'load-path' into 'state'.
> (use-package-vc-install): Install the package with package-vc.el.
> (use-package-handler/:vc): Handler for the :vc keyword.
> (use-package-normalize--vc-arg): Normalization for more complex
> arguments to 'use-package-normalize/:vc', in order to make them
> compatible with the specification of 'package-vc-selected-packages'.
> (use-package-normalize/:vc): Normalizer for the :vc keyword.
> (use-package): Document :vc.
>
> * lisp/use-package/use-package-ensure.el (use-package-handler/:ensure):
> Do not ensure a package when :vc is used in the declaration.
>
> * test/lisp/use-package/use-package-tests.el (use-package-test/:vc-1):
> (use-package-test/:vc-2):
> (use-package-test/:vc-3):
> (use-package-test/:vc-4):
> (use-package-test/:vc-5):
> (use-package-test-normalize/:vc):
> Add tests for :vc.
> ---
>  lisp/use-package/use-package-core.el       | 111 ++++++++++++++++++++-
>  lisp/use-package/use-package-ensure.el     |   3 +-
>  test/lisp/use-package/use-package-tests.el |  54 ++++++++++
>  3 files changed, 165 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/use-package/use-package-core.el b/lisp/use-package/use-package-core.el
> index 7ab5bdc276..e7e7ecc710 100644
> --- a/lisp/use-package/use-package-core.el
> +++ b/lisp/use-package/use-package-core.el
> @@ -76,6 +76,7 @@ use-package-keywords
>      :functions
>      :preface
>      :if :when :unless
> +    :vc
>      :no-require
>      :catch
>      :after
> @@ -1151,7 +1152,8 @@ use-package-normalize/:load-path
>      #'use-package-normalize-paths))
>  
>  (defun use-package-handler/:load-path (name _keyword arg rest state)
> -  (let ((body (use-package-process-keywords name rest state)))
> +  (let ((body (use-package-process-keywords name rest
> +                (plist-put state :load-path arg))))
>      (use-package-concat
>       (mapcar #'(lambda (path)
>                   `(eval-and-compile (add-to-list 'load-path ,path)))
> @@ -1577,6 +1579,109 @@ use-package-handler/:config
>       (when use-package-compute-statistics
>         `((use-package-statistics-gather :config ',name t))))))
>  
> +;;;; :vc
> +
> +(defun use-package-vc-install (arg &optional local-path)
> +  "Install a package with `package-vc.el'.
> +ARG is a list of the form (NAME OPTIONS REVISION), as returned by
> +`use-package-normalize--vc-arg'.  If LOCAL-PATH is non-nil, call
> +`package-vc-install-from-checkout'; otherwise, indicating a
> +remote host, call `package-vc-install' instead."
> +  (pcase-let* ((`(,name ,opts ,rev) arg)
> +               (spec (if opts (cons name opts) name)))
> +    (unless (package-installed-p name)
> +      (if local-path
> +          (package-vc-install-from-checkout local-path (symbol-name name))
> +        (package-vc-install spec rev)))))
> +
> +(defun use-package-handler/:vc (name _keyword arg rest state)
> +  "Generate code to install package NAME, or do so directly.
> +When the use-package declaration is part of a byte-compiled file,
> +install the package during compilation; otherwise, add it to the
> +macro expansion and wait until runtime.  The remaining arguments
> +are as follows:
> +
> +_KEYWORD is ignored.

I think we can remove this, the argument is explicitly marked as unused.

> +ARG is the normalized input to the `:vc' keyword, as returned by
> +the `use-package-normalize/:vc' function.
> +
> +REST is a plist of other (following) keywords and their
> +arguments, each having already been normalised by the respective
> +function.
> +
> +STATE is a plist of any state that keywords processed before
> +`:vc' (see `use-package-keywords') may have accumulated.
> +
> +Also see the Info node `(use-package) Creating an extension'."
> +  (let ((body (use-package-process-keywords name rest state))
> +        (local-path (car (plist-get state :load-path))))
> +    ;; See `use-package-handler/:ensure' for an explanation.
> +    (if (bound-and-true-p byte-compile-current-file)
> +        (funcall #'use-package-vc-install arg local-path)        ; compile time
> +      (push `(use-package-vc-install ',arg ,local-path) body)))) ; runtime
> +
> +(defun use-package-normalize--vc-arg (arg)
> +  "Normalize possible arguments to the `:vc' keyword.
> +ARG is a cons-cell of approximately the form that
> +`package-vc-selected-packages' accepts, plus an additional `:rev'
> +keyword.  If `:rev' is not given, it defaults to `:last-release'.
> +
> +Returns a list (NAME SPEC REV), where (NAME . SPEC) is compliant
> +with `package-vc-selected-packages' and REV is a (possibly nil,
> +indicating the latest commit) revision."
> +  (cl-flet* ((mk-string (s)
> +	       (if (and s (stringp s)) s (symbol-name s)))
> +             (mk-sym (s)
> +               (if (and s (stringp s)) (intern s) s))
> +	     (normalize (k v)

It seems there is inconsistent spacing here.

> +               (pcase k
> +                 (:rev (cond ((or (eq v :last-release) (not v)) :last-release)
> +                             ((eq v :newest) nil)
> +                             (t (mk-string v))))
> +                 (:vc-backend (mk-sym v))
> +                 (_ (mk-string v)))))
> +    (pcase-let ((valid-kws '(:url :branch :lisp-dir :main-file :vc-backend :rev))
> +                (`(,name . ,opts) arg))
> +      (if (stringp opts)                ; (NAME . VERSION-STRING) ?
> +          (list name opts)
> +        ;; Error handling
> +        (cl-loop for (k _) on opts by #'cddr
> +                 if (not (member k valid-kws))
> +                 do (use-package-error
> +                     (format "Keyword :vc received unknown argument: %s. Supported keywords are: %s"
> +                             k valid-kws)))
> +        ;; Actual normalization
> +        (list name
> +              (cl-loop for (k v) on opts by #'cddr
> +                       if (not (eq k :rev))
> +                       nconc (list k (normalize k v)))
> +              (normalize :rev (plist-get opts :rev)))))))
> +
> +(defun use-package-normalize/:vc (name _keyword args)
> +  "Normalize possible arguments to the `:vc' keyword.
> +NAME is the name of the `use-package' declaration, _KEYWORD is
> +ignored, and ARGS it a list of arguments given to the `:vc'
> +keyword, the cdr of which is ignored.
> +
> +See `use-package-normalize--vc-arg' for most of the actual
> +normalization work.  Also see the Info
> +node `(use-package) Creating an extension'."
> +  (let ((arg (car args)))
> +    (pcase arg
> +      ((or 'nil 't) (list name))                 ; guess name
> +      ((pred symbolp) (list arg))                ; use this name
> +      ((pred stringp) (list name arg))           ; version string + guess name
> +      ((pred plistp)                             ; plist + guess name
> +       (use-package-normalize--vc-arg (cons name arg)))
> +      (`(,(pred symbolp) . ,(or (pred plistp)    ; plist/version string + name
> +                                (pred stringp)))
> +       (use-package-normalize--vc-arg arg))
> +      (_ (use-package-error "Unrecognised argument to :vc.\
> + The keyword wants an argument of nil, t, a name of a package,\
> + or a cons-cell as accepted by `package-vc-selected-packages', where \
> + the accepted plist is augmented by a `:rev' keyword.")))))
> +
>  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>  ;;
>  ;;; The main macro
> @@ -1666,7 +1771,9 @@ use-package
>                   (compare with `custom-set-variables').
>  :custom-face     Call `custom-set-faces' with each face definition.
>  :ensure          Loads the package using package.el if necessary.
> -:pin             Pin the package to an archive."
> +:pin             Pin the package to an archive.
> +:vc              Install the package directly from a version control system
> +                 (using `package-vc.el')."
>    (declare (indent defun))
>    (unless (memq :disabled args)
>      (macroexp-progn
> diff --git a/lisp/use-package/use-package-ensure.el b/lisp/use-package/use-package-ensure.el
> index e0ea982594..395a0bbda0 100644
> --- a/lisp/use-package/use-package-ensure.el
> +++ b/lisp/use-package/use-package-ensure.el
> @@ -182,7 +182,8 @@ use-package-ensure-elpa
>  
>  ;;;###autoload
>  (defun use-package-handler/:ensure (name _keyword ensure rest state)
> -  (let* ((body (use-package-process-keywords name rest state)))
> +  (let* ((body (use-package-process-keywords name rest state))
> +         (ensure (and (not (plist-member rest :vc)) ensure)))
>      ;; We want to avoid installing packages when the `use-package' macro is
>      ;; being macro-expanded by elisp completion (see `lisp--local-variables'),
>      ;; but still install packages when byte-compiling, to avoid requiring
> diff --git a/test/lisp/use-package/use-package-tests.el b/test/lisp/use-package/use-package-tests.el
> index 6374a0d103..c8c20fc51c 100644
> --- a/test/lisp/use-package/use-package-tests.el
> +++ b/test/lisp/use-package/use-package-tests.el
> @@ -1951,6 +1951,60 @@ bind-key/845
>      (should (eq (nth 1 binding) 'ignore))
>      (should (eq (nth 2 binding) nil))))
>  
> +(ert-deftest use-package-test/:vc-1 ()
> +  (match-expansion
> +   (use-package foo :vc (:url "bar"))
> +   '(progn (use-package-vc-install '(foo (:url "bar") :last-release) nil)
> +           (require 'foo nil nil))))
> +
> +(ert-deftest use-package-test/:vc-2 ()
> +  (match-expansion
> +   (use-package foo
> +     :vc (baz . (:url "baz" :vc-backend "Git"
> +                 :main-file qux.el :rev "rev-string")))
> +   '(progn (use-package-vc-install '(baz
> +                                     (:url "baz" :vc-backend Git :main-file "qux.el")
> +                                     "rev-string")
> +                                   nil)
> +           (require 'foo nil nil))))
> +
> +(ert-deftest use-package-test/:vc-3 ()
> +  (match-expansion
> +   (use-package foo :vc (bar . "baz"))
> +   '(progn (use-package-vc-install '(bar "baz") nil)
> +           (require 'foo nil nil))))
> +
> +(ert-deftest use-package-test/:vc-4 ()
> +  (match-expansion
> +   (use-package foo :vc (bar . (:url "baz" :rev :newest)))
> +   '(progn (use-package-vc-install '(bar (:url "baz") nil) nil)
> +           (require 'foo nil nil))))
> +
> +(ert-deftest use-package-test/:vc-5 ()
> +  (let ((load-path? '(pred (apply-partially
> +                            #'string=
> +                            (expand-file-name "bar" user-emacs-directory)))))
> +    (match-expansion
> +     (use-package foo :vc other-name :load-path "bar")
> +     `(progn (eval-and-compile
> +               (add-to-list 'load-path ,load-path?))
> +             (use-package-vc-install '(other-name) ,load-path?)
> +             (require 'foo nil nil)))))
> +
> +(ert-deftest use-package-test-normalize/:vc ()
> +  (should (equal '(foo "version-string")
> +                 (use-package-normalize/:vc 'foo :vc '("version-string"))))
> +  (should (equal '(bar "version-string")
> +                 (use-package-normalize/:vc 'foo :vc '((bar . "version-string")))))
> +  (should (equal '(foo (:url "bar") "baz")
> +                 (use-package-normalize/:vc 'foo :vc '((:url "bar" :rev "baz")))))
> +  (should (equal '(foo)
> +                 (use-package-normalize/:vc 'foo :vc '(t))))
> +  (should (equal '(foo)
> +                 (use-package-normalize/:vc 'foo :vc nil)))
> +  (should (equal '(bar)
> +                 (use-package-normalize/:vc 'foo :vc '(bar)))))
> +
>  ;; Local Variables:
>  ;; no-byte-compile: t
>  ;; no-update-autoloads: t
> -- 
> 2.40.1
>
> From fba961176cd6ffb9998413f25956f7f6e044e064 Mon Sep 17 00:00:00 2001
> From: Tony Zorman <soliditsallgood@mailbox.org>
> Date: Thu, 29 Dec 2022 12:23:56 +0100
> Subject: [PATCH 2/2] ; Document use-package's :vc keyword
>
> * doc/misc/use-package.texi (Installing packages):
> (Install package):
> Add documentation for :vc and link to the related chapter in the Emacs
> manual.
>
> * etc/NEWS: Mention :vc keyword
> ---
>  doc/misc/use-package.texi | 50 +++++++++++++++++++++++++++++++++++++--
>  etc/NEWS                  | 13 ++++++++++
>  2 files changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/doc/misc/use-package.texi b/doc/misc/use-package.texi
> index 87105c4db0..d75cb67e08 100644
> --- a/doc/misc/use-package.texi
> +++ b/doc/misc/use-package.texi
> @@ -1554,8 +1554,11 @@ Installing packages
>  (@pxref{Package Installation,,, emacs, GNU Emacs Manual}).  The
>  @code{use-package} macro provides the @code{:ensure} and @code{:pin}
>  keywords that interface with that package manager to automatically
> -install packages.  This is particularly useful if you use your init
> -file on more than one system.
> +install packages.  The @code{:vc} keyword may be used to control how
> +package sources are downloaded; e.g., from remote hosts
> +(@pxref{Fetching Package Sources,,, emacs, GNU Emacs Manual}).  This
> +is particularly useful if you use your init file on more than one
> +system.
>  
>  @menu
>  * Install package::
> @@ -1607,6 +1610,49 @@ Install package
>  You can override the above setting for a single package by adding
>  @w{@code{:ensure nil}} to its declaration.
>  
> +@findex :vc
> +The @code{:vc} keyword can be used to control how packages are
> +downloaded and/or installed. More specifically, it allows one to fetch
> +and update packages directly from a version control system. This is
> +especially convenient when wanting to install a package that is not on
> +any package archive.
>
> +The keyword accepts the same arguments as specified in
> +@pxref{Fetching Package Sources,,, emacs, GNU Emacs Manual}, except
> +that a name need not explicitly be given: it is inferred from the
> +declaration.  The accepted property list is augmented by a @code{:rev}
> +keyword, which has the same shape as the @code{REV} argument to
> +@code{package-vc-install}.  Notably -- even when not specified --
> +@code{:rev} defaults to checking out the last release of the package.
> +You can use @code{:rev :newest} to check out the latest commit.
> +
> +For example,
> +
> +@example
> +@group
> +(use-package bbdb
> +  :vc (:url "https://git.savannah.nongnu.org/git/bbdb.git"
> +       :rev :newest))
> +@end group
> +@end example
> +
> +would try -- by invoking @code{package-vc-install} -- to install the
> +latest commit of the package @code{foo} from the specified remote.
> +
> +This can also be used for local packages, by combining it with the
> +@code{:load-path} (@pxref{Load path}) keyword:
> +
> +@example
> +@group
> +;; Use a local copy of BBDB instead of the one from GNU ELPA.
> +(use-package bbdb
> +  :vc t
> +  :load-path "/path/to/bbdb/dir/")
> +@end group
> +@end example
> +
> +The above dispatches to @code{package-vc-install-from-checkout}.
> +
>  @node Pinning packages
>  @section Pinning packages using @code{:pin}
>  @cindex installing package from specific archive
> diff --git a/etc/NEWS b/etc/NEWS
> index b989f80f3c..20f6751121 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -322,6 +322,19 @@ instead of:
>          and another_expression):
>          do_something()
>  
> +** use-package
> +
> ++++
> +*** New ':vc' keyword.
> +This keyword enables the user to control how packages are fetched by
> +utilising 'package-vc.el'.  By default, it relays its arguments to

I would not add the .el at the end.

> +'package-vc-install', but -- when combined with the ':load-path'
> +keyword -- it can also call upon 'package-vc-install-from-checkout'
> +instead.  If no revision is given via the ':rev' argument, use-package
> +falls back to the last release (via 'package-vc-install's
> +':last-release' argument).  To check out the last commit, use ':rev
> +:newest'.

I feel like this is too much information for the NEWS file, the first
sentence should be enough.

If you want to, I can make all these changes in your patch if you ack
them.  I'll try and test the code in the coming days to see if it all
works, and with John's and Eli's blessing will apply them to master.

>  \f
>  * New Modes and Packages in Emacs 30.1
>  
> -- 
> 2.40.1