Re: package-vc support for :files keyword

[Philip Kaludercic <philipk@posteo.net>]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

Tony Zorman <tonyzorman@mailbox.org> writes:

> On Tue, Sep 19 2023 08:47, Philip Kaludercic wrote:
>> Tony Zorman <tonyzorman@mailbox.org> writes:
>>
>> [… 42 lines elided …]
>>
>>> This is not just for multiple packages in a single repository—at least
>>> one has to somewhat broaden what "multiple packages" means. Some
>>> packages include small shims for bigger projects, and inadvertently
>>> require them as dependencies. The original issue[1] on the
>>> vc-use-package repo mentions org-ql[2], more specifically its helm
>>> integration in the form of helm-org-ql.el. Some people might not want to
>>> pull down helm as a dependency just for one file that they are not going
>>> to use anyways.
>>>
>>> I'm not sure how common of a situation this actually is, but at least
>>> for the big completion frameworks—helm and ivy—it's not totally unheard
>>> of.
>>
>> Hmm, this is interesting example that I was not familiar with.  As an
>> alternative idea, do you think that using `:ignored-files' like
>> elpa-admin.el could be useful?  You could exclude all the files with
>> "soft-dependencies", that wouldn't be scraped in `package-vc--unpack-1'
>> when looking for dependency files.
>
> Overall, I think this would be the better (even best) approach, yes.

Here is a sketch of how that could look like.  Can you test it to see if
it works:


[-- Attachment #2: Type: text/plain, Size: 3169 bytes --]

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index a8393cb7e75..07e660d9f33 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -213,7 +213,7 @@ package-vc--desc->spec
 name for PKG-DESC."
   (alist-get
    (setq name (or name (package-desc-name pkg-desc)))
-   (if (and (package-desc-archive pkg-desc)
+   (if (and pkg-desc (package-desc-archive pkg-desc)
             (not (alist-get name package-vc-selected-packages
                             nil nil #'string=)))
        (alist-get (intern (package-desc-archive pkg-desc))
@@ -501,7 +501,8 @@ package-vc--unpack-1
 autoloads, generating a package description file (used to
 identify a package as a VC package later on), building
 documentation and marking the package as installed."
-  (let (missing)
+  (let ((pkg-spec (package-vc--desc->spec pkg-desc))
+        missing)
     ;; Remove any previous instance of PKG-DESC from `package-alist'
     (let ((pkgs (assq (package-desc-name pkg-desc) package-alist)))
       (when pkgs
@@ -510,17 +511,27 @@ package-vc--unpack-1
     ;; In case the package was installed directly from source, the
     ;; dependency list wasn't know beforehand, and they might have
     ;; to be installed explicitly.
-    (let ((deps '()))
+    (let ((ignored-files
+           (mapconcat
+            (lambda (ignore)
+              (wildcard-to-regexp
+               (if (string-match-p "\\`/" ignore)
+                   (concat pkg-dir ignore)
+                 (concat "*/" ignore))))
+            (plist-get pkg-spec :ignored-files)
+            "\\|"))
+          (deps '()))
       (dolist (file (directory-files pkg-dir t "\\.el\\'" t))
-        (with-temp-buffer
-          (insert-file-contents file)
-          (when-let* ((require-lines (lm-header-multiline "package-requires")))
-            (thread-last
-              (mapconcat #'identity require-lines " ")
-              package-read-from-string
-              package--prepare-dependencies
-              (nconc deps)
-              (setq deps)))))
+        (unless (string-match-p ignored-files file)
+          (with-temp-buffer
+            (insert-file-contents file)
+            (when-let* ((require-lines (lm-header-multiline "package-requires")))
+              (thread-last
+                (mapconcat #'identity require-lines " ")
+                package-read-from-string
+                package--prepare-dependencies
+                (nconc deps)
+                (setq deps))))))
       (dolist (dep deps)
         (cl-callf version-to-list (cadr dep)))
       (setf missing (package-vc-install-dependencies (delete-dups deps)))
@@ -529,8 +540,7 @@ package-vc--unpack-1
                           missing)))
 
     (let ((default-directory (file-name-as-directory pkg-dir))
-          (pkg-file (expand-file-name (package--description-file pkg-dir) pkg-dir))
-          (pkg-spec (package-vc--desc->spec pkg-desc)))
+          (pkg-file (expand-file-name (package--description-file pkg-dir) pkg-dir)))
       ;; Generate autoloads
       (let* ((name (package-desc-name pkg-desc))
              (auto-name (format "%s-autoloads.el" name))

[-- Attachment #3: Type: text/plain, Size: 250 bytes --]


> (I also consider executing external shell commands during build-time an
> anti pattern, I just got the feeling that making package-vc support
> `:ignored-files' wasn't on the table—glad to realise that I was wrong!)

Why did you think so?