Subject: Re: bug#67916: 30.0.50; No lexical-binding directive warning in -pkg.el files

From: Stefan Kangas <[email-masked]>
Date: Tue, 11 Feb 2025 20:02:30 -0800

Philip Kaludercic <[email-masked]> writes:

> "Basil L. Contovounesios" <[email-masked]> writes:
>
>> Philip Kaludercic [2023-12-20 15:14 +0000] wrote:
>>
>>> "Basil L. Contovounesios" <[email-masked]> writes:
>>>
>>>> package-vc--generate-description-file currently passes:
>>>>   :kind 'vc
>>>> unquoted to define-package, which results in the -pkg.el contents:
>>>>   (define-package ... :kind vc ... :keywords '(...) ...)
>>>> Note the difference in quoting between :kind and :keywords.  Is this
>>>> intentional?  Or can/should :kind pass through macroexp-quote as well?
>>>
>>> This is not intentional, if anything a lucky oversight.
>>
>> Lucky in the sense that it's preferable this way, or just an accident? ;)
>
> The latter.
>
>>>> Questions for Stefan:
>>>>
>>>> - Which version of Emacs can/does elpa-admin.el assume?
>>>
>>> All I can say is that the ELPA build server, the main user of
>>> elpa-admin.el, has Emacs 28.2 installed.
>>
>> I'm wondering because elpa-admin.el seems to contain some compatibility
>> code for Emacs 26 (elpaa--select-revision, elpaa--write-pkg-file) and
>> Emacs versions <28 (elpaa--get-section, elpaa--html-build-doc).
>
> My guess is that it was just not removed, but I'll just let Stefan
> explain that.
>
>>>>  (defgroup package nil
>>>>    "Manager for Emacs Lisp packages."
>>>>    :group 'applications
>>>> -  :version "24.1")
>>>> +  :group 'tools
>>>> +  :version "30.1")
>>>
>>> I am not sure if bumping :version is necessary (here and above).
>>
>> I think it's unnecessary in the sense that I don't know of any place
>> where this information is displayed, but otherwise I thought it was good
>> form to do this since any change in :groups is user-visible.
>
> My understanding was that this information is supposed to indicate when
> the group was added, while each member of a group that has since been
> changed would have a newer :version tag.
>
>>>>  (define-inline package-vc-p (pkg-desc)
>>>>    "Return non-nil if PKG-DESC is a VC package."
>>>> -  (inline-letevals (pkg-desc)
>>>> -    (inline-quote (eq (package-desc-kind ,pkg-desc) 'vc))))
>>>> +  (inline-quote (eq (package-desc-kind ,pkg-desc) 'vc)))
>>>> +
>>>> +(define-inline package--unquote (arg)
>>>> +  "Return ARG without its surrounding `quote', if any."
>>>> +  (inline-letevals (arg)
>>>> +    (inline-quote (if (eq (car-safe ,arg) 'quote) (cadr ,arg) ,arg))))
>>>
>>> Honestly, the usage of define-inline was probably a premature
>>> optimisation on my part, I don't think we really need it here either.
>>
>> You mean, you prefer package--unquote being a plain function?
>
> Yes.
>
>> [ To be honest, I'm slightly inclined to add this to macroexp.el
>>   instead, since it's a somewhat common operation. ]
>
> My only concern is that this would slightly complicate the initiative of
> adding package.el to GNU ELPA, if that is to proceed at all.
>
>>> You removed (require 'inline) anyway, or is it now preloaded?
>>
>> define-inline is autoloaded, and there are no other in-tree occurrences
>> of (require 'inline).
>
> Nevermind then.
>
>>>> +;; Potentially also used in elpa.git.
>>>> +(defun package--write-description-file ( file name version doc reqs extras
>>>> +                                         &optional extra-props verbose)
>>>> +  "Write a `define-package' declaration to FILE.
>>>> +Absolute FILE names the -pkg.el description file.
>>>> +NAME, VERSION, and DOC are the leading, and EXTRA-PROPS the
>>>> +trailing, arguments of `define-package'.
>>>> +REQS and EXTRAS are the eponymous `package-desc' slots.
>>>> +Non-nil VERBOSE means display \"Wrote file\" message."
>>>> +  (let* ((src (replace-regexp-in-string (rx "-pkg.el" eos) ".el"
>>>> +                                        (file-name-nondirectory file) t t))
>>>> +         (def `(define-package ,name ,version ,doc
>>>> +                 ,(macroexp-quote
>>>> +                   ;; Turn requirement version lists into string form.
>>>> +                   (mapcar (lambda (elt)
>>>> +                             (list (car elt)
>>>> +                                   (package-version-join (cadr elt))))
>>>> +                           reqs))
>>>> +                 ,@extra-props
>>>> +                 ,@(package--alist-to-plist-args extras)))
>>>> +         (print-cfg '((length . nil)
>>>> +                      (level . nil)
>>>> +                      (quoted . t)))
>>>> +         (str (concat ";;; Generated package description from " src
>>>> +                      "  -*- no-byte-compile: t; lexical-binding: t -*-\n"
>>>> +                      (prin1-to-string def nil print-cfg)
>>>> +                      "\n")))
>>>> +    (write-region str nil file nil (unless verbose 'silent))))
>>>
>>> I like this, but we should really make sure that there are no hidden
>>> edge-cases that might cause problems.
>>
>> How do we find them if they're hidden? ;)
>> Did you have something specific in mind?
>
> IIRC there were some bugs related to the generation of -pkg.el files,
> but I can't find them right now.  I'll ping you if I find anything.
>
>> Thanks,

So what's the conclusion here?  Are these patches good to go, or is more
work needed?