GNU bug report logs - #76978
31.0.50; Archive information not displayed for installed packages in *Packages* buffer

Previous Next

Full log

Message #29 received at 76978 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: david <davidimagid <at> gmail.com>
Cc: Philip Kaludercic <philipk <at> posteo.net>, Eli Zaretskii <eliz <at> gnu.org>,
 76978 <at> debbugs.gnu.org, Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: [PATCH] package.el: Add built-in version check and loaded
 package marking
Date: Tue, 18 Mar 2025 18:28:11 -0400

> I also changed "activated" to "loaded", as the latter is more accurate
> for marking packages in the "Other versions" section.

Hmm... the two terms do not mean the same thing.  "Loaded" refers to the
package's `.el` files having been `load`ed, whereas "activated" means
that the package's directory has been added to `load-path` and its
`<PKG>-autoloads.el` file has been loaded.

The variable `package-activated-list` keeps track, as the name suggests,
of packages that have been activated, not loaded.

> (package-menu--check-builtin-version): New function to compare
> built-in and available versions.

You can just say "New function" here.  🙂

> +                             ;; Add built-in packages to the list if they exist.
> +                             (and-let* ((bi (assq name package--builtins)))
> +                               (list (package--from-builtin bi)))))
> +           (other-pkgs (remove desc all-pkgs)))

I didn't see an explanation in the commit message for why you replaced
`delete` with `remove`.  AFAICT, `all-pkgs` is a fresh new list so
`delete` looks safe here (as long as we don't use `all-pkgs` later).

> -                              (from (or (package-desc-archive opkg)
> -                                        (if (stringp dir) "installed" dir))))
> +                    (from (or (package-desc-archive opkg)
> +                              (if (stringp dir) "installed" "built-in")))

With the current code I see "builtin" displayed.  I don't understand
what `dir` does in the current code, but that means I also don't
understand what this change is supposed to do.

> +               (format "%s (%s%s)"
> +                       (if ov
> +                           (make-text-button
> +                            (package-version-join ov) nil
> +                            'font-lock-face 'link
> +                            'follow-link t
> +                            'action
> +                            (lambda (_button)
> +                              (describe-package opkg)))
> +                         "n/a")
> +                       from
> +                       (if loaded ", loaded" ""))))
> +           other-pkgs ", ")

I think when `ov` is nil, we're better off using the same output as
before, i.e. just (format "%s" from).  This said, I can't remember when
this can happen.

Also, I had questions in my previous review and I haven't seen any
answer to them (most importantly about copyright paperwork).


        Stefan

Previous Next

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