Re: feature/package+vc 04c4c578c7 3/4: Allow for packages to be installed directly from VCS

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>>> AFAIK this is sufficiently hypothetical that I'm really not convinced
>>>> it's worth the extra work (and the risk of incompatibilities between
>>>> the `elpa-admin.el` behavior and that if `package-vc`, e.g. where they
>>>> might disagree on which commit is "the" release).
>>>
>>> I don't know, to me this sounds more like an argument against merging
>>> elpa-admin.el and package-vc.el...
>>
>> It's not about sharing the code here (for which I have lobbied in other
>> messages), but about sharing the behavior.
>> `elpa-admin.el` de facto defines a "protocol" that upstream maintainers
>> have to follow.  If `package-vc` doesn't follow the same protocol,
>> end-users will suffer unexpected discrepancies.
>
> That is sort of the question I was implying previously (when mentioning
> adding meta data), as to whether or not there is a "protocol" or if the
> two format just coincidentally share a part of a common structure.

Here is what I had in mind.  With the following patch for
elpa-admin.git:

>From dd2bbf65ce18cebc28b1118a31ca5d117df1b5c0 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Fri, 28 Oct 2022 19:19:21 +0200
Subject: [PATCH] * elpa-admin.el (elpaa--publish-package-specs): Update file
 format

---
 elpa-admin.el | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/elpa-admin.el b/elpa-admin.el
index 81eb0b9f91..f45a596671 100644
--- a/elpa-admin.el
+++ b/elpa-admin.el
@@ -792,18 +792,25 @@ of the current `process-environment'.  Return the 
modified copy."
   "Process and publish SPECS in elpa-packages.eld files."
   (with-temp-buffer
     ;; Remove :core packages, handle :url nil and and compress the
-    ;; contents of the "elpa-packages"
+    ;; contents of the "elpa-packages".  Note that elpa-packages.eld
+    ;; does not use the same format as "elpa-packages" in
+    ;; {nongnu,elpa}.git.  The file is intended to be used by
+    ;; package-vc.el.
     (prin1
-     (mapcan
-      (lambda (spec)
-        (pcase spec
-          (`(,name :url nil . ,rest)
-           `((,name :url ,(concat "https://git.sv.gnu.org/git/"; elpaa--gitrepo)
-                    :branch ,(concat elpaa--branch-prefix (car spec))
-                    ,@rest)))
-          (`(,_ :core ,_ . ,_) nil)       ;not supported
-          (_ (list spec))))
-      specs)
+     (list (mapcan
+            (lambda (spec)
+              (let ((rel (ignore-errors (elpaa--get-last-release spec))))
+                (when rel
+                  (nconc spec (list :release-rev (cdr rel)))))
+              (pcase spec
+                (`(,name :url nil . ,rest)
+                 `((,name :url ,(concat "https://git.sv.gnu.org/git/"; 
elpaa--gitrepo)
+                          :branch ,(concat elpaa--branch-prefix (car spec))
+                          ,@rest)))
+                (`(,_ :core ,_ . ,_) nil)       ;not supported
+                (_ (list spec))))
+            specs)
+           :version 1 :default-vc 'Git)
      (current-buffer))
     (write-region nil nil (expand-file-name "elpa-packages.eld" 
elpaa--release-subdir))
     (write-region nil nil (expand-file-name "elpa-packages.eld" 
elpaa--devel-subdir))))
-- 
2.38.0

and these changes to package-vc.el:

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 8e4f2819db..e995853768 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -56,6 +56,9 @@ package-vc
   :group 'package
   :version "29.1")
 
+(defconst package-vc-elpa-packages-version 1
+  "Version number of the package specification format understood by 
package-vc.")
+
 (defcustom package-vc-heuristic-alist
   `((,(rx bos "http" (? "s") "://"
           (or (: (? "www.") "github.com"
@@ -144,6 +147,34 @@ package-vc-archive-spec-alist
 
 All other values are ignored.")
 
+(defvar package-vc-archive-data-alist nil
+  "List of package specification archive metadata.
+Each element of the list has the form (ARCHIVE . PLIST), where
+PLIST keys are one of:
+
+        `:version' (integer)
+
+Indicating the version of the file formatting, to be compared
+with `package-vc-elpa-packages-version'.
+
+        `:vc-backend' (symbol)
+
+A symbol indicating what the default VC backend to use if a
+package specification does not indicate anything.  The value
+ought to be a member of `vc-handled-backends'.  If missing,
+`vc-clone' will fall back onto `package-vc-default-backend'.
+
+All other values are ignored.")
+
+(define-inline package-vc-query-archive-data (archive prop)
+  "Query the property PROP for the package specification for PKG-DESC.
+If no package specification can be determined, the function will
+return nil."
+  (inline-letevals (archive prop)
+    (inline-quote (plist-get
+                   (alist-get ,archive package-vc-archive-data-alist)
+                   ,prop))))
+
 (defun package-vc-desc->spec (pkg-desc &optional name)
   "Retrieve the package specification for PKG-DESC.
 The optional argument NAME can be used to override the default
@@ -171,9 +202,23 @@ package-vc--read-archive-data
     (when (file-exists-p contents-file)
       (with-temp-buffer
         (let ((coding-system-for-read 'utf-8))
-          (insert-file-contents contents-file))
-        (setf (alist-get (intern archive) package-vc-archive-spec-alist)
-              (read (current-buffer)))))))
+          (insert-file-contents contents-file)
+          ;; The response from the server is expected to have the form
+          ;;
+          ;;    ((("foo" :url "..." ...) ...)
+          ;;     :version 1
+          ;;     :default-vc Git)
+          (let ((spec (read (current-buffer))))
+            (when (= package-vc-elpa-packages-version
+                     (plist-get (cdr spec) :version))
+              (setf (alist-get (intern archive) package-vc-archive-spec-alist)
+                    (car spec)))
+            (setf (alist-get (intern archive) package-vc-archive-data-alist)
+                  (cdr spec))
+            (when-let ((default-vc (plist-get (cdr spec) :default-vc))
+                       ((not (memq default-vc vc-handled-backends))))
+              (warn "Archive `%S' expects missing VC backend %S"
+                    archive (plist-get (cdr spec) :default-vc)))))))))
 
 (defun package-vc--download-and-read-archives (&optional async)
   "Download specifications of all `package-archives' and read them.
@@ -374,6 +419,10 @@ package-vc-unpack
       (unless (file-exists-p repo-dir)
         (make-directory (file-name-directory repo-dir) t)
         (let ((backend (or (package-vc-guess-backend url)
+                           (plist-get (alist-get (package-desc-archive 
pkg-desc)
+                                                 package-vc-archive-data-alist
+                                                 nil nil #'string=)
+                                      :vc-backend)
                            package-vc-default-backend)))
           (unless (vc-clone url backend repo-dir (or rev branch))
             (error "Failed to clone %s from %s" name url))))

A more forwards-compatible file format would be defined that would
uncouple the format used by elpa-admin.el and package-vc.el the more I
think about this, the more I think it is better to take this path
instead of sharing code between the two.  Sure, this means that some
things have to be duplicated, but we are talking about two different
scenarios (activating source controlled code and packaging tarballs)
that might have a lot in common but still ought to be considered
separately.