GNU bug report logs - #79188
31.0.50; Cannot build packages installed from VC

Previous Next

Full log

View this message in rfc822 format

From: Philip Kaludercic <philipk <at> posteo.net>
To: Przemysław Kryger <pkryger <at> gmail.com>
Cc: 79188 <at> debbugs.gnu.org
Subject: bug#79188: 31.0.50; Cannot build packages installed from VC
Date: Sat, 09 Aug 2025 11:43:59 +0000

Przemysław Kryger <pkryger <at> gmail.com> writes:

> tags 79188 + patch
> quit
>
> Przemysław Kryger <pkryger <at> gmail.com> writes:
>>> That seems reasonable, but in that case we should be able to reproduce
>>> the issue with a more simple example (especially one that doesn't involve
>>> use-package, MELPA and missing dependencies). [...]
>
> I managed to reproduce the issue with
> https://github.com/pkryger/basic-stats.el, which has no extra
> dependencies.  I could reproduce all the issues I described in previous
> message (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=79188#11).
>
> In addition I noticed, that `package-vc-upgrade' doesn't work as
> well. This one was trying to pull inside the package install directory,
> i.e, under `packgage-user-dir'.
>
>>> This should fix the first issue, but it probably won't change anything
>>> about the latter:
>>>
>>> diff --git a/lisp/package/package-vc.el b/lisp/package/package-vc.el
>>> index db12c76133e..f2c7c460f6d 100644
>>> --- a/lisp/package/package-vc.el
>>> +++ b/lisp/package/package-vc.el
>>> @@ -549,7 +549,14 @@ package-vc--unpack-1
>>>          ;; FIXME: Compilation should be done as a separate, optional, st=
>> ep.
>>>          ;; E.g. for multi-package installs, we should first install all =
>> packages
>>>          ;; and then compile them.
>>> -        (package--compile new-desc)
>>> +        (package--compile
>>> +         (if lisp-dir
>>> +             ;; In case we are installing a package from a local
>>> +             ;; checkout, we want to compile the checkout, not the
>>> +             ;; redirection!
>>> +             (package-desc-create :dir lisp-dir)
>>> +          new-desc))
>>> +
>>>          (when package-native-compile
>>>            (package--native-compile-async new-desc))
>>>          ;; After compilation, load again any files loaded by
>>>
>>
>> I don't have lisp/pakcage/package-vc.el, but I changed the file name to
>> lisp/emacs-lisp/packgage-vc.el and the patch applied cleanly.
>>
>> [...]
>>
>> After I repeated the experiment the package installed with just
>> `package-vc-install-from-checkout' (as described above) and
>> helm-projectile.elc file has been produced into package source
>> directory.
>
> Based on that work I developed a new patch.  With both of the attached
> patches applied I was able to run `package-vc-rebuild' and
> `package-vc-reinstall' on the aforementioned package basic-stats and
> observed that *.elc files were created in the source directory (i.e.,
> the local checkout) and not in package install directory.

Thanks, I have some comments below.

>> A question: shouldn't the newly generated *.elc files be put in
>> package install directory, just like the (non compiled) autoloads file
>> is?  In my - very na=C3=AFve - view this would not only remove burden
>> of doubling `load-path' entries (will that happen) but would also
>> allow for a complete separation of compiled files between multiple
>> Emacs versions coexisting on the same machine and reusing the same
>> package source directories.
>
> While the patch attached fixes basic workflows, I wonder idea of having
> *.elc in a package install directory is worth exploring.  Would that
> affect other functionalities?  For example `load' and `require' should
> just work (provided the package install directory is in front of package
> source directory in `load-pat'), but what will happen with
> `find-library'/`locate-library'?  Are there any others?

By package install directory you mean the ~/.emacs.d/elpa/... directory
right?  I get the advantage of not having incompatible .elc versions but
I am not an expert when it comes the load-order questions.  I think we
should discuss this in a separate feature request and then ask someone
who knows more about that to avoid making clumsy mistakes.  Does that
sound OK?

> From 56fa1014b1f8f2eb7f6d87304c3f31604fed48ba Mon Sep 17 00:00:00 2001
> From: Philip Kaludercic <philipk <at> posteo.net>
> Date: Fri, 8 Aug 2025 11:43:24 +0100
> Subject: [PATCH 1/2] Compile file in local checkout directory
>
> This partially fixes bug#79188.

We usually reference bug numbers at the end of a commit message in
parentheses, but I can take care of that.

> * lisp/emacs-lisp/package-vc.el (package-vc--unpack-1): Compile package
>   in a local checkout directory when it is installed from such a
>   location, for example with `package-vc-install-from-checkout'.
> ---
>  lisp/emacs-lisp/package-vc.el | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 7433fce2d89..9e118c7af02 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -546,7 +546,14 @@ package-vc--unpack-1
>          ;; FIXME: Compilation should be done as a separate, optional, step.
>          ;; E.g. for multi-package installs, we should first install all packages
>          ;; and then compile them.
> -        (package--compile new-desc)
> +        (package--compile
> +         (if lisp-dir
> +             ;; In case we are installing a package from a local
> +             ;; checkout, we want to compile the checkout, not the
> +             ;; redirection!
> +             (package-desc-create :dir lisp-dir)
> +          new-desc))
> +
>          (when package-native-compile
>            (package--native-compile-async new-desc))
>          ;; After compilation, load again any files loaded by
> -- 
> 2.50.1
>
>
> From 07596327df8bee5831003b62c811dd3fc53f2a88 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Przemys=C5=82aw=20Kryger?= <pkryger <at> gmail.com>
> Date: Fri, 8 Aug 2025 11:43:52 +0100
> Subject: [PATCH 2/2] Store local checkout directory in autoloads indirection
>
> * lisp/emacs-lisp/package-vc.el (package-vc--unpack-1): When installing
>   from a local checkout, store the value of `:lisp-dir' property of
>   `pkg-spec' in a "Package-spec" header in a generated autoloads
>   indirection file.
> * lisp/emacs-lisp/package-vc.el (package-vc--pkg-spec-from-autoloads):
>   New function to retrieve package spec from a "Package-spec" header
>   from autoloads indirection file (if any).
> * lisp/emacs-lisp/package-vc.el (package-vc-rebuild): Retrieve package
>   spec from autoloads indirection file (if any) and use it while calling
>   `package-vc--unpack-1'.
> * lisp/emacs-lisp/package-vc.el (package-vc-upgrade): Retrieve package
>   spec from autoloads indirection file (if any) and use it while calling
>   `package-vc--unpack-1' and for `vc-pull'.
>
> fixes: bug#79188

(The formatting is off here as well, but again, I can take care of that.
In case you did not know, in log-edit-mode, you can use the
`log-edit-generate-changelog-from-diff' command (bound to C-c C-w) to
generate a changelog from the current diff).

> ---
>  lisp/emacs-lisp/package-vc.el | 46 +++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 9e118c7af02..400e648b8f5 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -508,7 +508,14 @@ package-vc--unpack-1
>          (when lisp-dir
>            (write-region
>             (with-temp-buffer
> -             (insert ";; Autoload indirection for package-vc\n\n")
> +             (insert ";; Autoload indirection for package-vc\n")
> +             (insert ";; Package-spec: ")
> +             ;; Store the pkg-spec such that it can be reused by
> +             ;; `package-rebuild' and `package-vc-upgrade' to restore
> +             ;; the same conditions as were when the indirection has
> +             ;; been created for the first time.
> +             (prin1 pkg-spec (current-buffer))

This is strictly speaking not robust, as doesn't assure us that
something like (prin1 "one\ntwo") will result in a string without
newlines and not "break out" of the comment.  If anything, we should
consider storing this in a separate file, but more on that below.

> +             (insert "\n\n")
>               (prin1 `(load (expand-file-name
>                              ,(expand-file-name auto-name lisp-dir)
>                              (or (and load-file-name
> @@ -589,6 +596,19 @@ package-vc--unpack-1
>  
>  (declare-function project-remember-projects-under "project" (dir &optional recursive))
>  
> +(defun package-vc--pkg-spec-from-autoloads (pkg-desc)
> +  "Read a \"Packcage-spec\" header from autoloads file for PKG-DESC."
> +  (when-let* ((name (package-desc-name pkg-desc))
> +              (autoloads (expand-file-name (format "%s-autoloads.el" name)
> +                                           (package-desc-dir pkg-desc)))
> +              ((file-exists-p autoloads))
> +              (spec (with-temp-buffer
> +                      (insert-file-contents autoloads)
> +                      (ignore-errors
> +                        (read (lm-header "package-spec"))))))
> +    (cons (symbol-name name)
> +          spec)))

Generally speaking I am not sure if this approach is necessary, as we
already have `package-vc--desc->spec' and introducing this hack would
introduce an ambiguity as to where the authoritative package
specification is stored.

> +
>  (defun package-vc--clone (pkg-desc pkg-spec dir rev)
>    "Clone the package PKG-DESC whose spec is PKG-SPEC into the directory DIR.
>  REV specifies a specific revision to checkout.  This overrides the `:branch'
> @@ -756,7 +776,10 @@ package-vc-upgrade
>    ;;
>    ;; If there is a better way to do this, it should be done.
>    (cl-assert (package-vc-p pkg-desc))
> -  (letrec ((pkg-dir (package-desc-dir pkg-desc))
> +  (letrec ((pkg-spec (package-vc--pkg-spec-from-autoloads pkg-desc))

So we are just shadowing the pkg-spec passed as an argument?

> +           (pkg-dir (package-desc-dir pkg-desc))
> +           (source-dir (or (plist-get (cdr pkg-spec) :lisp-dir)
> +                           pkg-dir))

Just an an example of what I was talking about above: Here for instance
we already have an inconstancy.  Everywhere else in the file, we assume
that a package specification is just a plist, and not a (PACKAGE-NAME
. SPEC-PLIST) that requires a `cdr'. to access.

>             (vc-flags)
>             (vc-filter-command-function
>              (lambda (command file-or-list flags)
> @@ -764,18 +787,22 @@ package-vc-upgrade
>                (list command file-or-list flags)))
>             (post-upgrade
>              (lambda (_command _file-or-list flags)
> -              (when (and (file-equal-p pkg-dir default-directory)
> +              (when (and (file-equal-p source-dir default-directory)
>                           (eq flags vc-flags))
>                  (unwind-protect
>                      (with-demoted-errors "Failed to activate: %S"
> -                      (package-vc--unpack-1 pkg-desc pkg-dir))
> +                      (let ((package-vc-selected-packages
> +                             (if pkg-spec
> +                                 (cons pkg-spec package-vc-selected-packages)
> +                               package-vc-selected-packages)))
> +                        (package-vc--unpack-1 pkg-desc pkg-dir)))

You'll have yo remind me (i.e. add a comment) why you are binding the
variable here and why you are distinguishing between pkg-spec being nil
or not?

>                    (remove-hook 'vc-post-command-functions post-upgrade))))))
>      (add-hook 'vc-post-command-functions post-upgrade)
>      (with-demoted-errors "Failed to fetch: %S"
>        (require 'vc-dir)
>        (with-current-buffer (vc-dir-prepare-status-buffer
> -                            (format " *package-vc-dir: %s*" pkg-dir)
> -                            pkg-dir (vc-responsible-backend pkg-dir))
> +                            (format " *package-vc-dir: %s*" source-dir)
> +                            source-dir (vc-responsible-backend source-dir))
>          (vc-pull)))))
>  
>  (defun package-vc--archives-initialize ()
> @@ -966,7 +993,12 @@ package-vc-rebuild
>  is the responsibility of `package-vc-upgrade'.  Interactively,
>  prompt for the name of the package to rebuild."
>    (interactive (list (package-vc--read-package-desc "Rebuild package: " t)))
> -  (package-vc--unpack-1 pkg-desc (package-desc-dir pkg-desc)))
> +  (let* ((pkg-spec (package-vc--pkg-spec-from-autoloads pkg-desc))
> +         (package-vc-selected-packages
> +          (if pkg-spec
> +              (cons pkg-spec package-vc-selected-packages)
> +            package-vc-selected-packages)))
> +    (package-vc--unpack-1 pkg-desc (package-desc-dir pkg-desc))))
>  
>  ;;;###autoload
>  (defun package-vc-prepare-patch (pkg-desc subject revisions)

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.