Subject: Re: [PATCH] package.el: Add built-in version check and active package marking

From: Philip Kaludercic <[email-masked]>
Date: Tue, 18 Mar 2025 18:42:19 +0000

david <[email-masked]> writes:

> Hello Philip and Emacs maintainers,
>
> I’m submitting this patch to enhance `describe-package` by adding
> information about the currently activated package and improving the
> built-in version check.  Since "Other versions" already displays the
> current version of the package, I’ve added an "activated" marker to
> indicate which version is active in the current Emacs session.
>
> This patch for `package.el` introduces two improvements:
>
> 1. Built-in version check: The package menu now compares the built-in
> version of a package with the available version.  If the available
> version is newer, the status is set to "available"; otherwise, it is
> marked as "built-in".  This helps users identify when updates are
> available for built-in packages.
>
> 2. Activated packages in "Other versions": Packages listed in the
> "Other versions" section are now marked with ", activated" if they are
> currently active in the Emacs session.  This makes it clear which
> versions are in use.
>
> I’d appreciate your feedback and suggestions for installing this patch
> into the master branch.
>
> Best regards,
> David D.
>
> From 8b552c182a0ba103fcad89f80abd4343b69bef9a Mon Sep 17 00:00:00 2001
> From: dimagid <[email-masked]>
> Date: Tue, 18 Mar 2025 13:02:40 -0400
> Subject: [PATCH] package.el: Add built-in version check and active package
>  marking
>
> This commit introduces two improvements:
>
> 1. Built-in version check: The package menu now compares the built-in
> version of a package with the available version. If the available
> version is newer, the status is set to "available"; otherwise, it is
> marked as "built-in". This helps users identify when an update is
> available for built-in packages.
>
> 2. Activated packages in "Other versions": Packages listed in the "Other
> versions" section are now marked with ", activated" if they are
> currently active in the Emacs session. This provides clarity on which
> versions are in use.

Could you rewrite this to fit the commit message style described in
the CONTRIBUTE file?

> ---
>  lisp/emacs-lisp/package.el | 72 ++++++++++++++++++++++++++++----------
>  1 file changed, 53 insertions(+), 19 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index b9a8dacab15..7a615be2461 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -104,7 +104,7 @@
>  ;; Vinicius Jose Latorre <[email-masked]>
>  ;; Phil Hagelberg <[email-masked]>
>  
> -;;; ToDo:
> +;;; TODO:

If possible, avoid unrelated changes in the same patch.

>  
>  ;; - putting info dirs at the start of the info path means
>  ;;   users see a weird ordering of categories.  OTOH we want to
> @@ -117,7 +117,6 @@
>  ;; - give users a way to view a package's documentation when it
>  ;;   only appears in the .el
>  ;; - use/extend checkdoc so people can tell if their package will work
> -;; - "installed" instead of a blank in the status column
>  ;; - tramp needs its files to be compiled in a certain order.
>  ;;   how to handle this?  fix tramp?
>  ;; - maybe we need separate .elc directories for various emacs
> @@ -1230,7 +1229,7 @@ package-tar-file-info
>                         ((filename (tar-header-name (car tar-parse-info))))
>                       (let ((dirname (file-name-directory filename)))
>                         ;; The first file can be in a subdir: look for the top.
> -                       (if dirname (loop (directory-file-name dirname))
> +                       (if dirname (cl-loop (directory-file-name dirname))

Watch out, this is not an occurrence of cl-lib's loop, but the function
bound by named-let!

And also, if it were a instance of the old loop, this should be changed
in a separate patch.

>                           (file-name-as-directory filename)))))
>           (desc-file (package--description-file dir-name))
>           (tar-desc (tar-get-file-descriptor (concat dir-name desc-file))))
> @@ -2977,25 +2976,34 @@ describe-package-1
>          (package--print-email-button author)))
>      (let* ((all-pkgs (append (cdr (assq name package-alist))
>                               (cdr (assq name package-archive-contents))
> -                             (let ((bi (assq name package--builtins)))
> -                               (if bi (list (package--from-builtin bi))))))
> -           (other-pkgs (delete desc all-pkgs)))
> +                             (when-let* ((bi (assq name package--builtins)))

I would prefer `and-let*' here.  Also the change confuses me a bit,
could you clarify what is going on with a comment?

> +                               (list (package--from-builtin bi)))))
> +           (other-pkgs (remove desc all-pkgs)))
>        (when other-pkgs
>          (package--print-help-section "Other versions"
>            (mapconcat (lambda (opkg)
>                         (let* ((ov (package-desc-version opkg))
> -                              (dir (package-desc-dir opkg))
>                                (from (or (package-desc-archive opkg)
> -                                        (if (stringp dir) "installed" dir))))
> -                         (if (not ov) (format "%s" from)
> -                           (format "%s (%s)"
> -                                   (make-text-button (package-version-join ov) nil
> -                                                     'font-lock-face 'link
> -                                                     'follow-link t
> -                                                     'action
> -                                                     (lambda (_button)
> -                                                       (describe-package opkg)))
> -                                   from))))
> +                                        (if (package-desc-dir opkg)
> +                                            "installed" "built-in")))
> +                              ;; Check if the package is currently activated and
> +                              ;; matches the described version.
> +                              (activated (and (member (package-desc-name opkg)
> +                                                      package-activated-list)
> +                                              (equal ov (package-desc-version desc)))))
> +                         (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)))
> +                                   from)
> +                                 from
> +                                 ;; Append ", activated" if the package is currently
> +                                 ;; activated.
> +                                 (if activated ", activated" ""))))

Will this formatting be clear, and not cause any confusion if there is a
package called "activated"?

>                       other-pkgs ", ")
>            ".")))
>  
> @@ -3640,11 +3648,37 @@ package-status-avail-obso
>  
>  ;;; Package menu printing
>  
> +(defun package-menu--check-built-in-version (pkg)
> +  "Check if PKG is a built-in package and compare versions.
> +Return the appropriate status for PKG."
> +  (let* ((status (package-desc-status pkg))
> +         (name (package-desc-name pkg))
> +         (version (package-desc-version pkg))
> +         ;; Check if the package is built-in by looking it up in
> +         ;; `package--builtins'.
> +         (built-in-pkg (assq name package--builtins))
> +         ;; If the package is built-in, get its version.
> +         (built-in-version (if built-in-pkg
> +                               (package-desc-version
> +                                (package--from-builtin built-in-pkg)))))
> +
> +    ;; If the package is built-in and marked as "available", compare
> +    ;; versions.
> +    (when (and built-in-pkg
> +               (equal status "available"))
> +      ;; If the built-in version is older than the available version,
> +      ;; keep status as "available".  Otherwise, set status to
> +      ;; "built-in".
> +      (if (version-list-< built-in-version version)
> +          (setq status "available")
> +        (setq status "built-in")))
> +    status))

I think you can simplify the code here by just returning the values
directly:

(cond
 ((not (and built-in-pkg (equal status "available")))
  status)
 ((version-list-< built-in-version version)
  "available")
 ("built-in"))

> +
>  (defun package-menu--print-info-simple (pkg)
>    "Return a package entry suitable for `tabulated-list-entries'.
>  PKG is a `package-desc' object.
>  Return (PKG-DESC [NAME VERSION STATUS DOC])."
> -  (let* ((status  (package-desc-status pkg))
> +  (let* ((status (package-menu--check-built-in-version pkg))
>           (face (pcase status
>                   ("built-in"  'package-status-built-in)
>                   ("external"  'package-status-external)
> @@ -3676,7 +3710,7 @@ package-menu--print-info-simple
>                'font-lock-face face)
>              ,(propertize status 'font-lock-face face)
>              ,(propertize (or (package-desc-archive pkg) "")
> -                                    'font-lock-face face)
> +                         'font-lock-face face)

Again, this looks like a unrelated change?

>              ,(propertize (package-desc-summary pkg)
>                           'font-lock-face 'package-description)])))