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

[Philip Kaludercic]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Philip Kaludercic [2023-12-20 15:14 +0000] wrote:
>
>> "Basil L. Contovounesios" <contovob@tcd.ie> 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,