GNU bug report logs - #63336
[PATCH] package-vc: Process :make and :shell-command spec args

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 63336 in the body.
You can then email your comments to 63336 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] package-vc: Process :make and :shell-command spec args
Date: Sat, 06 May 2023 13:39:52 -0700

[Message part 1 (text/plain, inline)]

Hello!

Here's a patch to support :make and :shell-command args as discussed:

https://lists.gnu.org/archive/html/help-gnu-emacs/2023-04/msg00263.html

Best,

Joseph

[0001-package-vc-Process-make-and-shell-command-spec-args.patch (text/x-diff, attachment)]

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63336 <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Sun, 07 May 2023 09:03:43 +0000

Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

> Hello!
>
> Here's a patch to support :make and :shell-command args as discussed:
>
> https://lists.gnu.org/archive/html/help-gnu-emacs/2023-04/msg00263.html

Thanks!

> Best,
>
> Joseph
>
> From c51161c51f11e6ffcba17758424596fe44f9d42a Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
> Date: Sat, 6 May 2023 13:44:32 -0700
> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>
> ---
>  lisp/emacs-lisp/package-vc.el | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 421947b528d..489610e2a1e 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -344,6 +344,35 @@ asynchronously."
>          "\n")
>         nil pkg-file nil 'silent))))
>  
> +(defcustom package-vc-process-make nil
> +  "If non-nil, process :make and :shell-command spec arguments.
> +Package specs are loaded from trusted package archives."
> +  :type 'boolean)

As this patch is going to be added to Emacs 30, we should add

  :version "30.1"

tags to this user option.

> +(defun package-vc--call (destination program &rest args)
> +  "Like ‘call-process’ for PROGRAM, DESTINATION, ARGS.
           ^
You should replace these quotation marks with regular ASCII `marks', so
avoid byte-compiler warnings.

> +The INFILE and DISPLAY arguments are fixed as nil."
> +  (apply #'call-process program nil destination nil (delq nil args)))

What is the motivation for this function?  Is this where
process-isolation would be added in the future?

> +(defun package-vc--make (pkg-spec dir)
> +  "Process :make and :shell-command spec arguments."
> +  (let ((target (plist-get pkg-spec :make))
> +        (cmd (plist-get pkg-spec :shell-command)))
> +    (when (or cmd target)
> +      (with-current-buffer (get-buffer-create " *package-vc make*")
                                                 ^
                                                 should the package name
                                                 be mentioned here?
> +        (erase-buffer)
> +        (when (and cmd
> +                   (/= 0 (package-vc--call t shell-file-name
> +                                           shell-command-switch
> +                                           cmd)))
> +          (message "Failed to run %s, see buffer %S"

Could `warn' be a better candidate here, instead of `message'?

> +                   cmd (buffer-name)))
> +        (when (and target
> +                   (/= 0 (apply #'package-vc--call t "make"
> +                                (if (consp target) target (list target)))))
> +          (message "Failed to make %s, see buffer %S"
> +                   target (buffer-name)))))))
> +
>  (declare-function org-export-to-file "ox" (backend file))
>  
>  (defun package-vc--build-documentation (pkg-desc file)
> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>        ;; Generate package file
>        (package-vc--generate-description-file pkg-desc pkg-file)
>  
> +      ;; Process :make and :shell-command arguments before building documentation
> +      (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))

Wasn't the plan to allow `package-vc-process-make' to either be a
generic "build-anything" or a selective listing of packages where we
allow :make and :shell-command to be executed?

> +
>        ;; Detect a manual
>        (when (executable-find "install-info")
>          (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))

Otherwise this looks good, but I haven't tried it out yet.

-- 
Philip Kaludercic

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

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63336 <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Sun, 07 May 2023 11:47:29 -0700

[Message part 1 (text/plain, inline)]

Thanks for the review!

Philip Kaludercic <philipk <at> posteo.net> writes:

> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>
>> Hello!
>>
>> Here's a patch to support :make and :shell-command args as discussed:
>>
>> https://lists.gnu.org/archive/html/help-gnu-emacs/2023-04/msg00263.html
>
> Thanks!
>
>> Best,
>>
>> Joseph
>>
>> From c51161c51f11e6ffcba17758424596fe44f9d42a Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
>> Date: Sat, 6 May 2023 13:44:32 -0700
>> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>>
>> ---
>>  lisp/emacs-lisp/package-vc.el | 32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index 421947b528d..489610e2a1e 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -344,6 +344,35 @@ asynchronously."
>>          "\n")
>>         nil pkg-file nil 'silent))))
>>
>> +(defcustom package-vc-process-make nil
>> +  "If non-nil, process :make and :shell-command spec arguments.
>> +Package specs are loaded from trusted package archives."
>> +  :type 'boolean)
>
> As this patch is going to be added to Emacs 30, we should add
>
>   :version "30.1"
>
> tags to this user option.

Fixed.

>> +(defun package-vc--call (destination program &rest args)
>> +  "Like ‘call-process’ for PROGRAM, DESTINATION, ARGS.
>            ^
> You should replace these quotation marks with regular ASCII `marks', so
> avoid byte-compiler warnings.

Good catch.

>> +The INFILE and DISPLAY arguments are fixed as nil."
>> +  (apply #'call-process program nil destination nil (delq nil args)))
>
> What is the motivation for this function?  Is this where
> process-isolation would be added in the future?

In the attached patch, package-vc--call is replaced with call-process.

>> +(defun package-vc--make (pkg-spec dir)
>> +  "Process :make and :shell-command spec arguments."
>> +  (let ((target (plist-get pkg-spec :make))
>> +        (cmd (plist-get pkg-spec :shell-command)))
>> +    (when (or cmd target)
>> +      (with-current-buffer (get-buffer-create " *package-vc make*")
>                                                  ^
>                                                  should the package name
>                                                  be mentioned here?

I like this idea, but IIUC package-vc--make would then need to take an
extra arg, since pkg-spec doesn't contain the :name of the package. We
could also add :name to the pkg-spec plist?

For comparison, package-vc--build-documentation creates a buffer called
" *package-vc doc*" without the package name.

>> +        (erase-buffer)
>> +        (when (and cmd
>> +                   (/= 0 (package-vc--call t shell-file-name
>> +                                           shell-command-switch
>> +                                           cmd)))
>> +          (message "Failed to run %s, see buffer %S"
>
> Could `warn' be a better candidate here, instead of `message'?

Done.

>> +                   cmd (buffer-name)))
>> +        (when (and target
>> +                   (/= 0 (apply #'package-vc--call t "make"
>> +                                (if (consp target) target (list target)))))
>> +          (message "Failed to make %s, see buffer %S"

And this message is changed to warn also.

>> +                   target (buffer-name)))))))
>> +
>>  (declare-function org-export-to-file "ox" (backend file))
>>
>>  (defun package-vc--build-documentation (pkg-desc file)
>> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>>        ;; Generate package file
>>        (package-vc--generate-description-file pkg-desc pkg-file)
>>
>> +      ;; Process :make and :shell-command arguments before building documentation
>> +      (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
>
> Wasn't the plan to allow `package-vc-process-make' to either be a
> generic "build-anything" or a selective listing of packages where we
> allow :make and :shell-command to be executed?

Let me know if the attached commit accomplishes what you had in mind.

>> +
>>        ;; Detect a manual
>>        (when (executable-find "install-info")
>>          (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>
> Otherwise this looks good, but I haven't tried it out yet.

I fixed up a couple other issues:

- removed unnecessary dir arg to package-vc--make
- added function arg to the docstring for package-vc--make

I'm not sure if the customization type for package-vc-process-make is
correct. Please double check that.

Also, should users be able to run :make and :shell-command args defined
in a spec passed into package-vc-install?

Best,

Joseph

[0001-package-vc-Process-make-and-shell-command-spec-args.patch (text/x-diff, attachment)]

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

From: Ruijie Yu <ruijie <at> netyu.xyz>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63336 <at> debbugs.gnu.org, Philip Kaludercic <philipk <at> posteo.net>
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Mon, 8 May 2023 16:42:12 +0800

Hello Joseph, 

On mobile so please excuse my brevity and top-posting. 

Minor remark on the defcustom type: I think you should move the current tag from "symbol" to its outer "repeat", and optionally tag "symbol" as something like "package name".  WDYT? 

--
Best, 


RY

> On May 8, 2023, at 04:29, Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors <bug-gnu-emacs <at> gnu.org> wrote:
> 
> Thanks for the review!
> 
> Philip Kaludercic <philipk <at> posteo.net> writes:
> 
>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>> 
>>> Hello!
>>> 
>>> Here's a patch to support :make and :shell-command args as discussed:
>>> 
>>> https://lists.gnu.org/archive/html/help-gnu-emacs/2023-04/msg00263.html
>> 
>> Thanks!
>> 
>>> Best,
>>> 
>>> Joseph
>>> 
>>> From c51161c51f11e6ffcba17758424596fe44f9d42a Mon Sep 17 00:00:00 2001
>>> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
>>> Date: Sat, 6 May 2023 13:44:32 -0700
>>> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>>> 
>>> ---
>>> lisp/emacs-lisp/package-vc.el | 32 ++++++++++++++++++++++++++++++++
>>> 1 file changed, 32 insertions(+)
>>> 
>>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>>> index 421947b528d..489610e2a1e 100644
>>> --- a/lisp/emacs-lisp/package-vc.el
>>> +++ b/lisp/emacs-lisp/package-vc.el
>>> @@ -344,6 +344,35 @@ asynchronously."
>>>         "\n")
>>>        nil pkg-file nil 'silent))))
>>> 
>>> +(defcustom package-vc-process-make nil
>>> +  "If non-nil, process :make and :shell-command spec arguments.
>>> +Package specs are loaded from trusted package archives."
>>> +  :type 'boolean)
>> 
>> As this patch is going to be added to Emacs 30, we should add
>> 
>>  :version "30.1"
>> 
>> tags to this user option.
> 
> Fixed.
> 
>>> +(defun package-vc--call (destination program &rest args)
>>> +  "Like ‘call-process’ for PROGRAM, DESTINATION, ARGS.
>>           ^
>> You should replace these quotation marks with regular ASCII `marks', so
>> avoid byte-compiler warnings.
> 
> Good catch.
> 
>>> +The INFILE and DISPLAY arguments are fixed as nil."
>>> +  (apply #'call-process program nil destination nil (delq nil args)))
>> 
>> What is the motivation for this function?  Is this where
>> process-isolation would be added in the future?
> 
> In the attached patch, package-vc--call is replaced with call-process.
> 
>>> +(defun package-vc--make (pkg-spec dir)
>>> +  "Process :make and :shell-command spec arguments."
>>> +  (let ((target (plist-get pkg-spec :make))
>>> +        (cmd (plist-get pkg-spec :shell-command)))
>>> +    (when (or cmd target)
>>> +      (with-current-buffer (get-buffer-create " *package-vc make*")
>>                                                 ^
>>                                                 should the package name
>>                                                 be mentioned here?
> 
> I like this idea, but IIUC package-vc--make would then need to take an
> extra arg, since pkg-spec doesn't contain the :name of the package. We
> could also add :name to the pkg-spec plist?
> 
> For comparison, package-vc--build-documentation creates a buffer called
> " *package-vc doc*" without the package name.
> 
>>> +        (erase-buffer)
>>> +        (when (and cmd
>>> +                   (/= 0 (package-vc--call t shell-file-name
>>> +                                           shell-command-switch
>>> +                                           cmd)))
>>> +          (message "Failed to run %s, see buffer %S"
>> 
>> Could `warn' be a better candidate here, instead of `message'?
> 
> Done.
> 
>>> +                   cmd (buffer-name)))
>>> +        (when (and target
>>> +                   (/= 0 (apply #'package-vc--call t "make"
>>> +                                (if (consp target) target (list target)))))
>>> +          (message "Failed to make %s, see buffer %S"
> 
> And this message is changed to warn also.
> 
>>> +                   target (buffer-name)))))))
>>> +
>>> (declare-function org-export-to-file "ox" (backend file))
>>> 
>>> (defun package-vc--build-documentation (pkg-desc file)
>>> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>>>       ;; Generate package file
>>>       (package-vc--generate-description-file pkg-desc pkg-file)
>>> 
>>> +      ;; Process :make and :shell-command arguments before building documentation
>>> +      (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
>> 
>> Wasn't the plan to allow `package-vc-process-make' to either be a
>> generic "build-anything" or a selective listing of packages where we
>> allow :make and :shell-command to be executed?
> 
> Let me know if the attached commit accomplishes what you had in mind.
> 
>>> +
>>>       ;; Detect a manual
>>>       (when (executable-find "install-info")
>>>         (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>> 
>> Otherwise this looks good, but I haven't tried it out yet.
> 
> I fixed up a couple other issues:
> 
> - removed unnecessary dir arg to package-vc--make
> - added function arg to the docstring for package-vc--make
> 
> I'm not sure if the customization type for package-vc-process-make is
> correct. Please double check that.
> 
> Also, should users be able to run :make and :shell-command args defined
> in a spec passed into package-vc-install?
> 
> Best,
> 
> Joseph
> 
> <0001-package-vc-Process-make-and-shell-command-spec-args.patch>

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

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Ruijie Yu <ruijie <at> netyu.xyz>
Cc: 63336 <at> debbugs.gnu.org, Philip Kaludercic <philipk <at> posteo.net>
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Mon, 08 May 2023 12:38:06 -0700

[Message part 1 (text/plain, inline)]

Ruijie Yu <ruijie <at> netyu.xyz> writes:

> Hello Joseph,
>
> On mobile so please excuse my brevity and top-posting.
>
> Minor remark on the defcustom type: I think you should move the current tag from "symbol" to its outer "repeat", and optionally tag "symbol" as something like "package name".  WDYT?

Thanks, Ruijie! I think the defcustom type is now correct.

Joseph

[0001-package-vc-Process-make-and-shell-command-spec-args.patch (text/x-diff, attachment)]

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

From: Richard Stallman <rms <at> gnu.org>
To: Ruijie Yu <ruijie <at> netyu.xyz>
Cc: 63336 <at> debbugs.gnu.org, philipk <at> posteo.net, joseph <at> breatheoutbreathe.in
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Mon, 08 May 2023 20:03:22 -0400

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > On mobile so please excuse my brevity and top-posting. 

I have a feeling that means you are running nonfree malware.
It's too bad.  But anyway, thanks for contributing to Emacs.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63336 <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Wed, 10 May 2023 06:35:03 +0000

Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

>>> +(defun package-vc--make (pkg-spec dir)
>>> +  "Process :make and :shell-command spec arguments."
>>> +  (let ((target (plist-get pkg-spec :make))
>>> +        (cmd (plist-get pkg-spec :shell-command)))
>>> +    (when (or cmd target)
>>> +      (with-current-buffer (get-buffer-create " *package-vc make*")
>>                                                  ^
>>                                                  should the package name
>>                                                  be mentioned here?
>
> I like this idea, but IIUC package-vc--make would then need to take an
> extra arg, since pkg-spec doesn't contain the :name of the package. We
> could also add :name to the pkg-spec plist?

I wouldn't be in favour of that, I think that passing the name as a
separate argument would be a better solution.

> For comparison, package-vc--build-documentation creates a buffer called
> " *package-vc doc*" without the package name.

The difference I see here is that documentation usually builds fine,
while :make or :shell-command have a higher chance of failing because
some software is missing, especially if people don't use :make the way
it is used on the ELPA server but to build external dependencies (I'm
thinking of mail clients like notmuch)

>>> +                   target (buffer-name)))))))
>>> +
>>>  (declare-function org-export-to-file "ox" (backend file))
>>>
>>>  (defun package-vc--build-documentation (pkg-desc file)
>>> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>>>        ;; Generate package file
>>>        (package-vc--generate-description-file pkg-desc pkg-file)
>>>
>>> +      ;; Process :make and :shell-command arguments before building documentation
>>> +      (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
>>
>> Wasn't the plan to allow `package-vc-process-make' to either be a
>> generic "build-anything" or a selective listing of packages where we
>> allow :make and :shell-command to be executed?
>
> Let me know if the attached commit accomplishes what you had in mind.

Yes, that (or rather the newest version from a different message) looks good.

>>> +
>>>        ;; Detect a manual
>>>        (when (executable-find "install-info")
>>>          (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>>
>> Otherwise this looks good, but I haven't tried it out yet.
>
> I fixed up a couple other issues:
>
> - removed unnecessary dir arg to package-vc--make
> - added function arg to the docstring for package-vc--make
>
> I'm not sure if the customization type for package-vc-process-make is
> correct. Please double check that.
>
> Also, should users be able to run :make and :shell-command args defined
> in a spec passed into package-vc-install?

Yes, is that currently not supported?

> Best,
>
> Joseph

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63336 <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Sat, 13 May 2023 17:18:24 +0000

ping?

Philip Kaludercic <philipk <at> posteo.net> writes:

> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>
>>>> +(defun package-vc--make (pkg-spec dir)
>>>> +  "Process :make and :shell-command spec arguments."
>>>> +  (let ((target (plist-get pkg-spec :make))
>>>> +        (cmd (plist-get pkg-spec :shell-command)))
>>>> +    (when (or cmd target)
>>>> +      (with-current-buffer (get-buffer-create " *package-vc make*")
>>>                                                  ^
>>>                                                  should the package name
>>>                                                  be mentioned here?
>>
>> I like this idea, but IIUC package-vc--make would then need to take an
>> extra arg, since pkg-spec doesn't contain the :name of the package. We
>> could also add :name to the pkg-spec plist?
>
> I wouldn't be in favour of that, I think that passing the name as a
> separate argument would be a better solution.
>
>> For comparison, package-vc--build-documentation creates a buffer called
>> " *package-vc doc*" without the package name.
>
> The difference I see here is that documentation usually builds fine,
> while :make or :shell-command have a higher chance of failing because
> some software is missing, especially if people don't use :make the way
> it is used on the ELPA server but to build external dependencies (I'm
> thinking of mail clients like notmuch)
>
>>>> +                   target (buffer-name)))))))
>>>> +
>>>>  (declare-function org-export-to-file "ox" (backend file))
>>>>
>>>>  (defun package-vc--build-documentation (pkg-desc file)
>>>> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>>>>        ;; Generate package file
>>>>        (package-vc--generate-description-file pkg-desc pkg-file)
>>>>
>>>> +      ;; Process :make and :shell-command arguments before building documentation
>>>> +      (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
>>>
>>> Wasn't the plan to allow `package-vc-process-make' to either be a
>>> generic "build-anything" or a selective listing of packages where we
>>> allow :make and :shell-command to be executed?
>>
>> Let me know if the attached commit accomplishes what you had in mind.
>
> Yes, that (or rather the newest version from a different message) looks good.
>
>>>> +
>>>>        ;; Detect a manual
>>>>        (when (executable-find "install-info")
>>>>          (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>>>
>>> Otherwise this looks good, but I haven't tried it out yet.
>>
>> I fixed up a couple other issues:
>>
>> - removed unnecessary dir arg to package-vc--make
>> - added function arg to the docstring for package-vc--make
>>
>> I'm not sure if the customization type for package-vc-process-make is
>> correct. Please double check that.
>>
>> Also, should users be able to run :make and :shell-command args defined
>> in a spec passed into package-vc-install?
>
> Yes, is that currently not supported?
>
>> Best,
>>
>> Joseph

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

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63336 <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Wed, 10 May 2023 18:37:15 -0700

[Message part 1 (text/plain, inline)]

Philip Kaludercic <philipk <at> posteo.net> writes:

> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>
>>>> +(defun package-vc--make (pkg-spec dir)
>>>> +  "Process :make and :shell-command spec arguments."
>>>> +  (let ((target (plist-get pkg-spec :make))
>>>> +        (cmd (plist-get pkg-spec :shell-command)))
>>>> +    (when (or cmd target)
>>>> +      (with-current-buffer (get-buffer-create " *package-vc make*")
>>>                                                  ^
>>>                                                  should the package name
>>>                                                  be mentioned here?
>>
>> I like this idea, but IIUC package-vc--make would then need to take an
>> extra arg, since pkg-spec doesn't contain the :name of the package. We
>> could also add :name to the pkg-spec plist?
>
> I wouldn't be in favour of that, I think that passing the name as a
> separate argument would be a better solution.

I agree.

>> For comparison, package-vc--build-documentation creates a buffer called
>> " *package-vc doc*" without the package name.
>
> The difference I see here is that documentation usually builds fine,
> while :make or :shell-command have a higher chance of failing because
> some software is missing, especially if people don't use :make the way
> it is used on the ELPA server but to build external dependencies (I'm
> thinking of mail clients like notmuch)

That makes sense to me. In the attached patch, I pass pkg-desc to
package-vc--make instead just name.

Want me to submit a separate patch which adds the package name to the
" *package-vc doc*" buffer name?

>>>> +                   target (buffer-name)))))))
>>>> +
>>>>  (declare-function org-export-to-file "ox" (backend file))
>>>>
>>>>  (defun package-vc--build-documentation (pkg-desc file)
>>>> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>>>>        ;; Generate package file
>>>>        (package-vc--generate-description-file pkg-desc pkg-file)
>>>>
>>>> +      ;; Process :make and :shell-command arguments before building documentation
>>>> +      (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
>>>
>>> Wasn't the plan to allow `package-vc-process-make' to either be a
>>> generic "build-anything" or a selective listing of packages where we
>>> allow :make and :shell-command to be executed?
>>
>> Let me know if the attached commit accomplishes what you had in mind.
>
> Yes, that (or rather the newest version from a different message) looks good.
>
>>>> +
>>>>        ;; Detect a manual
>>>>        (when (executable-find "install-info")
>>>>          (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>>>
>>> Otherwise this looks good, but I haven't tried it out yet.
>>
>> I fixed up a couple other issues:
>>
>> - removed unnecessary dir arg to package-vc--make
>> - added function arg to the docstring for package-vc--make
>>
>> I'm not sure if the customization type for package-vc-process-make is
>> correct. Please double check that.
>>
>> Also, should users be able to run :make and :shell-command args defined
>> in a spec passed into package-vc-install?
>
> Yes, is that currently not supported?

Nevermind! It is supported. I didn't notice that package-vc--unpack adds
the user-defined pkg-spec to package-vc-selected-packages just before
calling package-vc--unpack-1.

Best,

Joseph

[0001-package-vc-Process-make-and-shell-command-spec-args.patch (text/x-diff, attachment)]

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63336 <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Sun, 14 May 2023 07:44:16 +0000

Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>>
>>>>> +(defun package-vc--make (pkg-spec dir)
>>>>> +  "Process :make and :shell-command spec arguments."
>>>>> +  (let ((target (plist-get pkg-spec :make))
>>>>> +        (cmd (plist-get pkg-spec :shell-command)))
>>>>> +    (when (or cmd target)
>>>>> +      (with-current-buffer (get-buffer-create " *package-vc make*")
>>>>                                                  ^
>>>>                                                  should the package name
>>>>                                                  be mentioned here?
>>>
>>> I like this idea, but IIUC package-vc--make would then need to take an
>>> extra arg, since pkg-spec doesn't contain the :name of the package. We
>>> could also add :name to the pkg-spec plist?
>>
>> I wouldn't be in favour of that, I think that passing the name as a
>> separate argument would be a better solution.
>
> I agree.
>
>>> For comparison, package-vc--build-documentation creates a buffer called
>>> " *package-vc doc*" without the package name.
>>
>> The difference I see here is that documentation usually builds fine,
>> while :make or :shell-command have a higher chance of failing because
>> some software is missing, especially if people don't use :make the way
>> it is used on the ELPA server but to build external dependencies (I'm
>> thinking of mail clients like notmuch)
>
> That makes sense to me. In the attached patch, I pass pkg-desc to
> package-vc--make instead just name.
>
> Want me to submit a separate patch which adds the package name to the
> " *package-vc doc*" buffer name?

No, I don't think it is necessary.  But thanks.

>>>>> +                   target (buffer-name)))))))
>>>>> +
>>>>>  (declare-function org-export-to-file "ox" (backend file))
>>>>>
>>>>>  (defun package-vc--build-documentation (pkg-desc file)
>>>>> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>>>>>        ;; Generate package file
>>>>>        (package-vc--generate-description-file pkg-desc pkg-file)
>>>>>
>>>>> +      ;; Process :make and :shell-command arguments before building documentation
>>>>> +      (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
>>>>
>>>> Wasn't the plan to allow `package-vc-process-make' to either be a
>>>> generic "build-anything" or a selective listing of packages where we
>>>> allow :make and :shell-command to be executed?
>>>
>>> Let me know if the attached commit accomplishes what you had in mind.
>>
>> Yes, that (or rather the newest version from a different message) looks good.
>>
>>>>> +
>>>>>        ;; Detect a manual
>>>>>        (when (executable-find "install-info")
>>>>>          (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>>>>
>>>> Otherwise this looks good, but I haven't tried it out yet.
>>>
>>> I fixed up a couple other issues:
>>>
>>> - removed unnecessary dir arg to package-vc--make
>>> - added function arg to the docstring for package-vc--make
>>>
>>> I'm not sure if the customization type for package-vc-process-make is
>>> correct. Please double check that.
>>>
>>> Also, should users be able to run :make and :shell-command args defined
>>> in a spec passed into package-vc-install?
>>
>> Yes, is that currently not supported?
>
> Nevermind! It is supported. I didn't notice that package-vc--unpack adds
> the user-defined pkg-spec to package-vc-selected-packages just before
> calling package-vc--unpack-1.

1+

> Best,
>
> Joseph
>
> From b27724197acd4ee72f9d336843f0e6ed9fcee87b Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
> Date: Sat, 13 May 2023 10:05:04 -0700
> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>
> ---
>  lisp/emacs-lisp/package-vc.el | 37 +++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index beca0bd00e2..8529d1dad5c 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -344,6 +344,33 @@ asynchronously."
>          "\n")
>         nil pkg-file nil 'silent))))
>  
> +(defcustom package-vc-process-make nil

Have we discussed the name of this user option?  I feel it is too
immediate, and therefore not intuitively obvious what purpose it serves.
I would imagine something along the lines of
"package-vc-allow-side-effects" or "package-vc-permit-building" could be
better?  WDYT?

> +  "Whether to process :make and :shell-command spec arguments.

I guess here too an explanation would be warranted (and in the manual).
Explaining what the issue is, and why one might be wary to enable the option.

> +When set to a list of symbols (packages), run commands for only
> +packages in the list. When `nil', never run commands. Otherwise
> +when non-`nil', run commands for any package with :make or
> +:shell-command specified.
> +
> +Package specs are loaded from trusted package archives."
> +  :type '(choice (const :tag "Run for all packages" t)
> +                 (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
> +                 (const :tag "Never run" nil))
> +  :version "30.1")
> +
> +(defun package-vc--make (pkg-spec pkg-desc)
> +  "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
> +  (let ((target (plist-get pkg-spec :make))
> +        (cmd (plist-get pkg-spec :shell-command)))
> +    (when (or cmd target)
> +      (with-current-buffer (get-buffer-create

I'd format the buffer name in the top let to prevent this line-break here.

> +                            (format " *package-vc make %s*" (package-desc-name pkg-desc)))
> +        (erase-buffer)
> +        (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
> +          (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
> +        (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
> +          (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))

If :shell-command fails, do we really want to proceed to :make?

>  (declare-function org-export-to-file "ox" (backend file))
>  
>  (defun package-vc--build-documentation (pkg-desc file)
> @@ -486,6 +513,16 @@ documentation and marking the package as installed."
>        ;; Generate package file
>        (package-vc--generate-description-file pkg-desc pkg-file)
>  
> +      ;; Process :make and :shell-command arguments before building documentation
> +      (pcase package-vc-process-make
> +        ((pred consp) ; When non-`nil' list, check if package is on the list.
> +         (when (memq (package-desc-name pkg-desc) package-vc-process-make)
> +           (package-vc--make pkg-spec pkg-desc)))
> +        ('nil         ; When `nil', do nothing.
> +         nil)

Perhaps swap the two conditions, first checking nil then listp which I
think reads more natural.  Then again, is pcase actually serving
anything here?

> +        (_            ; When otherwise non-`nil', run commands.
> +         (package-vc--make pkg-spec pkg-desc)))
> +
>        ;; Detect a manual
>        (when (executable-find "install-info")
>          (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))

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

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63336 <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Sun, 14 May 2023 01:08:29 -0700

[Message part 1 (text/plain, inline)]

Philip Kaludercic <philipk <at> posteo.net> writes:

> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>
>>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>> From b27724197acd4ee72f9d336843f0e6ed9fcee87b Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
>> Date: Sat, 13 May 2023 10:05:04 -0700
>> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>>
>> ---
>>  lisp/emacs-lisp/package-vc.el | 37 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index beca0bd00e2..8529d1dad5c 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -344,6 +344,33 @@ asynchronously."
>>          "\n")
>>         nil pkg-file nil 'silent))))
>>
>> +(defcustom package-vc-process-make nil
>
> Have we discussed the name of this user option?  I feel it is too
> immediate, and therefore not intuitively obvious what purpose it serves.
> I would imagine something along the lines of
> "package-vc-allow-side-effects" or "package-vc-permit-building" could be
> better?  WDYT?

I like "package-vc-allow-side-effects". Changed in attached patch.

>> +  "Whether to process :make and :shell-command spec arguments.
>
> I guess here too an explanation would be warranted (and in the manual).
> Explaining what the issue is, and why one might be wary to enable the option.

Does my addition suffice?

We also might want to add another option for
package-vc-allow-side-effects like 'user-defined, which only runs :make
and :shell-command args which were specified by the user (as opposed to
those which were downloaded from elpa). WDYT?

To update the manual, shall I edit doc/emacs/package.texi directly or is
there another file to edit?

>> +When set to a list of symbols (packages), run commands for only
>> +packages in the list. When `nil', never run commands. Otherwise
>> +when non-`nil', run commands for any package with :make or
>> +:shell-command specified.
>> +
>> +Package specs are loaded from trusted package archives."
>> +  :type '(choice (const :tag "Run for all packages" t)
>> +                 (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
>> +                 (const :tag "Never run" nil))
>> +  :version "30.1")
>> +
>> +(defun package-vc--make (pkg-spec pkg-desc)
>> +  "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
>> +  (let ((target (plist-get pkg-spec :make))
>> +        (cmd (plist-get pkg-spec :shell-command)))
>> +    (when (or cmd target)
>> +      (with-current-buffer (get-buffer-create
>
> I'd format the buffer name in the top let to prevent this line-break here.

Done.

>> +                            (format " *package-vc make %s*" (package-desc-name pkg-desc)))
>> +        (erase-buffer)
>> +        (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
>> +          (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
>> +        (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
>> +          (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
>
> If :shell-command fails, do we really want to proceed to :make?

Up to you! I was following the lead of elpa-admin.el.

>>  (declare-function org-export-to-file "ox" (backend file))
>>
>>  (defun package-vc--build-documentation (pkg-desc file)
>> @@ -486,6 +513,16 @@ documentation and marking the package as installed."
>>        ;; Generate package file
>>        (package-vc--generate-description-file pkg-desc pkg-file)
>>
>> +      ;; Process :make and :shell-command arguments before building documentation
>> +      (pcase package-vc-process-make
>> +        ((pred consp) ; When non-`nil' list, check if package is on the list.
>> +         (when (memq (package-desc-name pkg-desc) package-vc-process-make)
>> +           (package-vc--make pkg-spec pkg-desc)))
>> +        ('nil         ; When `nil', do nothing.
>> +         nil)
>
> Perhaps swap the two conditions, first checking nil then listp which I
> think reads more natural.  Then again, is pcase actually serving
> anything here?

I switched the first two cases. I think pcase is readable here,
especially if we add an 'user-defined option. What would you use
instead?

>> +        (_            ; When otherwise non-`nil', run commands.
>> +         (package-vc--make pkg-spec pkg-desc)))
>> +
>>        ;; Detect a manual
>>        (when (executable-find "install-info")
>>          (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))

[0001-package-vc-Process-make-and-shell-command-spec-args.patch (text/x-diff, attachment)]

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63336 <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Sun, 14 May 2023 19:30:23 +0000

Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>>
>>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>>
>>>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>>> From b27724197acd4ee72f9d336843f0e6ed9fcee87b Mon Sep 17 00:00:00 2001
>>> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
>>> Date: Sat, 13 May 2023 10:05:04 -0700
>>> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>>>
>>> ---
>>>  lisp/emacs-lisp/package-vc.el | 37 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 37 insertions(+)
>>>
>>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>>> index beca0bd00e2..8529d1dad5c 100644
>>> --- a/lisp/emacs-lisp/package-vc.el
>>> +++ b/lisp/emacs-lisp/package-vc.el
>>> @@ -344,6 +344,33 @@ asynchronously."
>>>          "\n")
>>>         nil pkg-file nil 'silent))))
>>>
>>> +(defcustom package-vc-process-make nil
>>
>> Have we discussed the name of this user option?  I feel it is too
>> immediate, and therefore not intuitively obvious what purpose it serves.
>> I would imagine something along the lines of
>> "package-vc-allow-side-effects" or "package-vc-permit-building" could be
>> better?  WDYT?
>
> I like "package-vc-allow-side-effects". Changed in attached patch.
>
>>> +  "Whether to process :make and :shell-command spec arguments.
>>
>> I guess here too an explanation would be warranted (and in the manual).
>> Explaining what the issue is, and why one might be wary to enable the option.
>
> Does my addition suffice?
>
> We also might want to add another option for
> package-vc-allow-side-effects like 'user-defined, which only runs :make
> and :shell-command args which were specified by the user (as opposed to
> those which were downloaded from elpa). WDYT?

That sounds like a good idea, but let us do that in a separate patch.

> To update the manual, shall I edit doc/emacs/package.texi directly or is
> there another file to edit?

Yes, just update the table under the "Specifying Package Sources" subsection.

>>> +When set to a list of symbols (packages), run commands for only
>>> +packages in the list. When `nil', never run commands. Otherwise
>>> +when non-`nil', run commands for any package with :make or
>>> +:shell-command specified.
>>> +
>>> +Package specs are loaded from trusted package archives."
>>> +  :type '(choice (const :tag "Run for all packages" t)
>>> +                 (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
>>> +                 (const :tag "Never run" nil))
>>> +  :version "30.1")
>>> +
>>> +(defun package-vc--make (pkg-spec pkg-desc)
>>> +  "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
>>> +  (let ((target (plist-get pkg-spec :make))
>>> +        (cmd (plist-get pkg-spec :shell-command)))
>>> +    (when (or cmd target)
>>> +      (with-current-buffer (get-buffer-create
>>
>> I'd format the buffer name in the top let to prevent this line-break here.
>
> Done.
>
>>> +                            (format " *package-vc make %s*" (package-desc-name pkg-desc)))
>>> +        (erase-buffer)
>>> +        (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
>>> +          (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
>>> +        (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
>>> +          (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
>>
>> If :shell-command fails, do we really want to proceed to :make?
>
> Up to you! I was following the lead of elpa-admin.el.

In that case let us do that too, unless there is a good reason not to.

>>>  (declare-function org-export-to-file "ox" (backend file))
>>>
>>>  (defun package-vc--build-documentation (pkg-desc file)
>>> @@ -486,6 +513,16 @@ documentation and marking the package as installed."
>>>        ;; Generate package file
>>>        (package-vc--generate-description-file pkg-desc pkg-file)
>>>
>>> +      ;; Process :make and :shell-command arguments before building documentation
>>> +      (pcase package-vc-process-make
>>> +        ((pred consp) ; When non-`nil' list, check if package is on the list.
>>> +         (when (memq (package-desc-name pkg-desc) package-vc-process-make)
>>> +           (package-vc--make pkg-spec pkg-desc)))
>>> +        ('nil         ; When `nil', do nothing.
>>> +         nil)
>>
>> Perhaps swap the two conditions, first checking nil then listp which I
>> think reads more natural.  Then again, is pcase actually serving
>> anything here?
>
> I switched the first two cases. I think pcase is readable here,
> especially if we add an 'user-defined option. What would you use
> instead?

I would have just used a regular cond.

--8<---------------cut here---------------start------------->8---
(cond
 ((null package-vc-process-make)
  ...)
 ((listp package-vc-process-make)
  ...)
 (...))
--8<---------------cut here---------------end--------------->8---

But this doesn't matter, do what you prefer.

>>> +        (_            ; When otherwise non-`nil', run commands.
>>> +         (package-vc--make pkg-spec pkg-desc)))
>>> +
>>>        ;; Detect a manual
>>>        (when (executable-find "install-info")
>>>          (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>
> From 3e7084e8e3e3ba142f383e90bfa656f59f3cc1ad Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
> Date: Sat, 13 May 2023 10:05:04 -0700
> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>
> ---
>  lisp/emacs-lisp/package-vc.el | 40 +++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index beca0bd00e2..8403add364c 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -344,6 +344,36 @@ asynchronously."
>          "\n")
>         nil pkg-file nil 'silent))))
>  
> +(defcustom package-vc-allow-side-effects nil
> +  "Whether to process :make and :shell-command spec arguments.
> +
> +Be careful when changing this option as processing :make and
> +:shell-command will run potentially harmful code.

Sounds scary.  I guess that is the point, but what do you think about
something like

  Be careful when changing this option, as installing and updating a
  package can potentially run harmful code.  If possible, allow packages
  you trust to run code, if it is necessary for a package to be properly
  initialised.

> +
> +When set to a list of symbols (packages), run commands for only
> +packages in the list. When `nil', never run commands.  Otherwise
> +when non-`nil', run commands for any package with :make or
> +:shell-command specified.

Watch out.  According to (elisp) Documentation Tips, nil is not quoted.

> +
> +Package specs are loaded from trusted package archives."
> +  :type '(choice (const :tag "Run for all packages" t)
> +                 (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
> +                 (const :tag "Never run" nil))
> +  :version "30.1")
> +
> +(defun package-vc--make (pkg-spec pkg-desc)
> +  "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
> +  (let ((target (plist-get pkg-spec :make))
> +        (cmd (plist-get pkg-spec :shell-command))
> +        (buf (format " *package-vc make %s*" (package-desc-name pkg-desc))))
> +    (when (or cmd target)
> +      (with-current-buffer (get-buffer-create buf)
> +        (erase-buffer)
> +        (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
> +          (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
> +        (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
> +          (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
> +
>  (declare-function org-export-to-file "ox" (backend file))
>  
>  (defun package-vc--build-documentation (pkg-desc file)
> @@ -486,6 +516,16 @@ documentation and marking the package as installed."
>        ;; Generate package file
>        (package-vc--generate-description-file pkg-desc pkg-file)
>  
> +      ;; Process :make and :shell-command arguments before building documentation
> +      (pcase package-vc-allow-side-effects
> +        ('nil         ; When `nil', do nothing.
> +         nil)
> +        ((pred consp) ; When non-`nil' list, check if package is on the list.
> +         (when (memq (package-desc-name pkg-desc) package-vc-allow-side-effects)
> +           (package-vc--make pkg-spec pkg-desc)))
> +        (_            ; When otherwise non-`nil', run commands.
> +         (package-vc--make pkg-spec pkg-desc)))
> +
>        ;; Detect a manual
>        (when (executable-find "install-info")
>          (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))

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

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63336 <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Sun, 14 May 2023 16:01:10 -0700

[Message part 1 (text/plain, inline)]

Philip Kaludercic <philipk <at> posteo.net> writes:

> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>
>>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>>>
>>>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>>>
>>>>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

>> We also might want to add another option for
>> package-vc-allow-side-effects like 'user-defined, which only runs :make
>> and :shell-command args which were specified by the user (as opposed to
>> those which were downloaded from elpa). WDYT?
>
> That sounds like a good idea, but let us do that in a separate patch.

Okay!

>> To update the manual, shall I edit doc/emacs/package.texi directly or is
>> there another file to edit?
>
> Yes, just update the table under the "Specifying Package Sources" subsection.

See patch.

>>> If :shell-command fails, do we really want to proceed to :make?
>>
>> Up to you! I was following the lead of elpa-admin.el.
>
> In that case let us do that too, unless there is a good reason not to.

+1

>> I switched the first two cases. I think pcase is readable here,
>> especially if we add an 'user-defined option. What would you use
>> instead?
>
> I would have just used a regular cond.
>
> --8<---------------cut here---------------start------------->8---
> (cond
>  ((null package-vc-process-make)
>   ...)
>  ((listp package-vc-process-make)
>   ...)
>  (...))
> --8<---------------cut here---------------end--------------->8---
>
> But this doesn't matter, do what you prefer.

Thank you! I like pcase here.

>> +Be careful when changing this option as processing :make and
>> +:shell-command will run potentially harmful code.
>
> Sounds scary.  I guess that is the point, but what do you think about
> something like
>
>   Be careful when changing this option, as installing and updating a
>   package can potentially run harmful code.  If possible, allow packages
>   you trust to run code, if it is necessary for a package to be properly
>   initialised.

Thank you! What do you think about the version in the attached patch?

>> +When set to a list of symbols (packages), run commands for only
>> +packages in the list. When `nil', never run commands.  Otherwise
>> +when non-`nil', run commands for any package with :make or
>> +:shell-command specified.
>
> Watch out.  According to (elisp) Documentation Tips, nil is not quoted.

Good to know! Fixed.

[0001-package-vc-Process-make-and-shell-command-spec-args.patch (text/x-diff, attachment)]

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63336 <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Mon, 15 May 2023 09:12:26 +0000

Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>>
>>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>>
>>>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>>>>
>>>>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>>>>
>>>>>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>
>>> We also might want to add another option for
>>> package-vc-allow-side-effects like 'user-defined, which only runs :make
>>> and :shell-command args which were specified by the user (as opposed to
>>> those which were downloaded from elpa). WDYT?
>>
>> That sounds like a good idea, but let us do that in a separate patch.
>
> Okay!
>
>>> To update the manual, shall I edit doc/emacs/package.texi directly or is
>>> there another file to edit?
>>
>> Yes, just update the table under the "Specifying Package Sources" subsection.
>
> See patch.
>
>>>> If :shell-command fails, do we really want to proceed to :make?
>>>
>>> Up to you! I was following the lead of elpa-admin.el.
>>
>> In that case let us do that too, unless there is a good reason not to.
>
> +1
>
>>> I switched the first two cases. I think pcase is readable here,
>>> especially if we add an 'user-defined option. What would you use
>>> instead?
>>
>> I would have just used a regular cond.
>>
>> --8<---------------cut here---------------start------------->8---
>> (cond
>>  ((null package-vc-process-make)
>>   ...)
>>  ((listp package-vc-process-make)
>>   ...)
>>  (...))
>> --8<---------------cut here---------------end--------------->8---
>>
>> But this doesn't matter, do what you prefer.
>
> Thank you! I like pcase here.
>
>>> +Be careful when changing this option as processing :make and
>>> +:shell-command will run potentially harmful code.
>>
>> Sounds scary.  I guess that is the point, but what do you think about
>> something like
>>
>>   Be careful when changing this option, as installing and updating a
>>   package can potentially run harmful code.  If possible, allow packages
>>   you trust to run code, if it is necessary for a package to be properly
>>   initialised.
>
> Thank you! What do you think about the version in the attached patch?
>
>>> +When set to a list of symbols (packages), run commands for only
>>> +packages in the list. When `nil', never run commands.  Otherwise
>>> +when non-`nil', run commands for any package with :make or
>>> +:shell-command specified.
>>
>> Watch out.  According to (elisp) Documentation Tips, nil is not quoted.
>
> Good to know! Fixed.
>
> From 812e32ea6c3f7b2d71174658db0e272b0b4fb84b Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
> Date: Sat, 13 May 2023 10:05:04 -0700
> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>
> ---
>  doc/emacs/package.texi        |  9 ++++++++
>  lisp/emacs-lisp/package-vc.el | 42 +++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/doc/emacs/package.texi b/doc/emacs/package.texi
> index 6722185cb20..4f606b22e54 100644
> --- a/doc/emacs/package.texi
> +++ b/doc/emacs/package.texi
> @@ -682,6 +682,15 @@ A string providing the repository-relative name of the documentation
>  file from which to build an Info file.  This can be a Texinfo file or
>  an Org file.
>  
> +@item :make
> +A string or list of strings providing the target or targets defined in
> +the repository Makefile which should run before building the Info
> +file. Only takes effect when package-vc-allow-side-effects is non-nil.

A @var is missing here

> +
> +@item :shell-command
> +A string providing the shell command to run before building the Info
> +file. Only takes effect when package-vc-allow-side-effects is non-nil.

and here.  I can take care of that.

> +
>  @item :vc-backend
>  A symbol naming the VC backend to use for downloading a copy of the
>  package's repository (@pxref{Version Control Systems,,,emacs, The GNU
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index beca0bd00e2..d2f6d287224 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -344,6 +344,38 @@ asynchronously."
>          "\n")
>         nil pkg-file nil 'silent))))
>  
> +(defcustom package-vc-allow-side-effects nil
> +  "Whether to process :make and :shell-command spec arguments.
> +
> +It may be necessary to run :make and :shell-command arguments in
> +order to initialize a package or build its documentation, but
> +please be careful when changing this option, as installing and
> +updating a package can run potentially harmful code.
> +
> +When set to a list of symbols (packages), run commands for only
> +packages in the list. When nil, never run commands.  Otherwise
> +when non-nil, run commands for any package with :make or
> +:shell-command specified.
> +
> +Package specs are loaded from trusted package archives."
> +  :type '(choice (const :tag "Run for all packages" t)
> +                 (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
> +                 (const :tag "Never run" nil))
> +  :version "30.1")
> +
> +(defun package-vc--make (pkg-spec pkg-desc)
> +  "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
> +  (let ((target (plist-get pkg-spec :make))
> +        (cmd (plist-get pkg-spec :shell-command))
> +        (buf (format " *package-vc make %s*" (package-desc-name pkg-desc))))
> +    (when (or cmd target)
> +      (with-current-buffer (get-buffer-create buf)
> +        (erase-buffer)
> +        (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
> +          (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
> +        (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
> +          (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
> +
>  (declare-function org-export-to-file "ox" (backend file))
>  
>  (defun package-vc--build-documentation (pkg-desc file)
> @@ -486,6 +518,16 @@ documentation and marking the package as installed."
>        ;; Generate package file
>        (package-vc--generate-description-file pkg-desc pkg-file)
>  
> +      ;; Process :make and :shell-command arguments before building documentation
> +      (pcase package-vc-allow-side-effects
> +        ('nil         ; When `nil', do nothing.
> +         nil)
> +        ((pred consp) ; When non-`nil' list, check if package is on the list.
> +         (when (memq (package-desc-name pkg-desc) package-vc-allow-side-effects)
> +           (package-vc--make pkg-spec pkg-desc)))
> +        (_            ; When otherwise non-`nil', run commands.
> +         (package-vc--make pkg-spec pkg-desc)))

Thinking about this again, I am still not convinced.  Isn't

--8<---------------cut here---------------start------------->8---
(when (or (eq package-vc-allow-side-effects t)
	  (memq (package-desc-name pkg-desc)
		package-vc-allow-side-effects))
  (package-vc--make pkg-spec pkg-desc))
--8<---------------cut here---------------end--------------->8---

much simpler?  Again, you don't have to prepare another patch, I'm just
interested in what you think.

> +
>        ;; Detect a manual
>        (when (executable-find "install-info")
>          (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))

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

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63336 <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Mon, 15 May 2023 12:03:14 -0700

Philip Kaludercic <philipk <at> posteo.net> writes:

> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>
>>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>>>
>>>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>>>
>>>>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>>>>>
>>>>>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>>>>>
>>>>>>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>>
>>>> We also might want to add another option for
>>>> package-vc-allow-side-effects like 'user-defined, which only runs :make
>>>> and :shell-command args which were specified by the user (as opposed to
>>>> those which were downloaded from elpa). WDYT?
>>>
>>> That sounds like a good idea, but let us do that in a separate patch.
>>
>> Okay!
>>
>>>> To update the manual, shall I edit doc/emacs/package.texi directly or is
>>>> there another file to edit?
>>>
>>> Yes, just update the table under the "Specifying Package Sources" subsection.
>>
>> See patch.
>>
>>>>> If :shell-command fails, do we really want to proceed to :make?
>>>>
>>>> Up to you! I was following the lead of elpa-admin.el.
>>>
>>> In that case let us do that too, unless there is a good reason not to.
>>
>> +1
>>
>>>> I switched the first two cases. I think pcase is readable here,
>>>> especially if we add an 'user-defined option. What would you use
>>>> instead?
>>>
>>> I would have just used a regular cond.
>>>
>>> --8<---------------cut here---------------start------------->8---
>>> (cond
>>>  ((null package-vc-process-make)
>>>   ...)
>>>  ((listp package-vc-process-make)
>>>   ...)
>>>  (...))
>>> --8<---------------cut here---------------end--------------->8---
>>>
>>> But this doesn't matter, do what you prefer.
>>
>> Thank you! I like pcase here.
>>
>>>> +Be careful when changing this option as processing :make and
>>>> +:shell-command will run potentially harmful code.
>>>
>>> Sounds scary.  I guess that is the point, but what do you think about
>>> something like
>>>
>>>   Be careful when changing this option, as installing and updating a
>>>   package can potentially run harmful code.  If possible, allow packages
>>>   you trust to run code, if it is necessary for a package to be properly
>>>   initialised.
>>
>> Thank you! What do you think about the version in the attached patch?
>>
>>>> +When set to a list of symbols (packages), run commands for only
>>>> +packages in the list. When `nil', never run commands.  Otherwise
>>>> +when non-`nil', run commands for any package with :make or
>>>> +:shell-command specified.
>>>
>>> Watch out.  According to (elisp) Documentation Tips, nil is not quoted.
>>
>> Good to know! Fixed.
>>
>> From 812e32ea6c3f7b2d71174658db0e272b0b4fb84b Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
>> Date: Sat, 13 May 2023 10:05:04 -0700
>> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>>
>> ---
>>  doc/emacs/package.texi        |  9 ++++++++
>>  lisp/emacs-lisp/package-vc.el | 42 +++++++++++++++++++++++++++++++++++
>>  2 files changed, 51 insertions(+)
>>
>> diff --git a/doc/emacs/package.texi b/doc/emacs/package.texi
>> index 6722185cb20..4f606b22e54 100644
>> --- a/doc/emacs/package.texi
>> +++ b/doc/emacs/package.texi
>> @@ -682,6 +682,15 @@ A string providing the repository-relative name of the documentation
>>  file from which to build an Info file.  This can be a Texinfo file or
>>  an Org file.
>>
>> +@item :make
>> +A string or list of strings providing the target or targets defined in
>> +the repository Makefile which should run before building the Info
>> +file. Only takes effect when package-vc-allow-side-effects is non-nil.
>
> A @var is missing here

Thank you!

>> +
>> +@item :shell-command
>> +A string providing the shell command to run before building the Info
>> +file. Only takes effect when package-vc-allow-side-effects is non-nil.
>
> and here.  I can take care of that.

Thank you!

>> +
>>  @item :vc-backend
>>  A symbol naming the VC backend to use for downloading a copy of the
>>  package's repository (@pxref{Version Control Systems,,,emacs, The GNU
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index beca0bd00e2..d2f6d287224 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -344,6 +344,38 @@ asynchronously."
>>          "\n")
>>         nil pkg-file nil 'silent))))
>>
>> +(defcustom package-vc-allow-side-effects nil
>> +  "Whether to process :make and :shell-command spec arguments.
>> +
>> +It may be necessary to run :make and :shell-command arguments in
>> +order to initialize a package or build its documentation, but
>> +please be careful when changing this option, as installing and
>> +updating a package can run potentially harmful code.
>> +
>> +When set to a list of symbols (packages), run commands for only
>> +packages in the list. When nil, never run commands.  Otherwise
>> +when non-nil, run commands for any package with :make or
>> +:shell-command specified.
>> +
>> +Package specs are loaded from trusted package archives."
>> +  :type '(choice (const :tag "Run for all packages" t)
>> +                 (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
>> +                 (const :tag "Never run" nil))
>> +  :version "30.1")
>> +
>> +(defun package-vc--make (pkg-spec pkg-desc)
>> +  "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
>> +  (let ((target (plist-get pkg-spec :make))
>> +        (cmd (plist-get pkg-spec :shell-command))
>> +        (buf (format " *package-vc make %s*" (package-desc-name pkg-desc))))
>> +    (when (or cmd target)
>> +      (with-current-buffer (get-buffer-create buf)
>> +        (erase-buffer)
>> +        (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
>> +          (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
>> +        (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
>> +          (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
>> +
>>  (declare-function org-export-to-file "ox" (backend file))
>>
>>  (defun package-vc--build-documentation (pkg-desc file)
>> @@ -486,6 +518,16 @@ documentation and marking the package as installed."
>>        ;; Generate package file
>>        (package-vc--generate-description-file pkg-desc pkg-file)
>>
>> +      ;; Process :make and :shell-command arguments before building documentation
>> +      (pcase package-vc-allow-side-effects
>> +        ('nil         ; When `nil', do nothing.
>> +         nil)
>> +        ((pred consp) ; When non-`nil' list, check if package is on the list.
>> +         (when (memq (package-desc-name pkg-desc) package-vc-allow-side-effects)
>> +           (package-vc--make pkg-spec pkg-desc)))
>> +        (_            ; When otherwise non-`nil', run commands.
>> +         (package-vc--make pkg-spec pkg-desc)))
>
> Thinking about this again, I am still not convinced.  Isn't
>
> --8<---------------cut here---------------start------------->8---
> (when (or (eq package-vc-allow-side-effects t)
> 	  (memq (package-desc-name pkg-desc)
> 		package-vc-allow-side-effects))
>   (package-vc--make pkg-spec pkg-desc))
> --8<---------------cut here---------------end--------------->8---
>
> much simpler?  Again, you don't have to prepare another patch, I'm just
> interested in what you think.

I take it all back and insist upon the opposite :)

You are right, that's much simpler.

>> +
>>        ;; Detect a manual
>>        (when (executable-find "install-info")
>>          (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))

Message #52 received at 63336-done <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63336-done <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Tue, 16 May 2023 19:29:52 +0000

I've pushed the changes to master.  If you are still interested in
improving the granularity of the issue, create a new report when you
have a patch we can discuss.

Message #55 received at 63336-done <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63336-done <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Tue, 16 May 2023 14:08:33 -0700

Philip Kaludercic <philipk <at> posteo.net> writes:

> I've pushed the changes to master.  If you are still interested in
> improving the granularity of the issue, create a new report when you
> have a patch we can discuss.

Thank you! I assume you're referring to something like a 'user-defined
option for package-vc-allow-side-effects. At some point, I may submit
another patch adding that feature!

Best,

Joseph

Message #58 received at 63336-done <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63336-done <at> debbugs.gnu.org
Subject: Re: bug#63336: [PATCH] package-vc: Process :make and :shell-command
 spec args
Date: Wed, 17 May 2023 14:07:03 +0000

Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> I've pushed the changes to master.  If you are still interested in
>> improving the granularity of the issue, create a new report when you
>> have a patch we can discuss.
>
> Thank you! I assume you're referring to something like a 'user-defined
> option for package-vc-allow-side-effects. 

Right,

>                                           At some point, I may submit
> another patch adding that feature!

but there is no hurry for that.

> Best,
>
> Joseph

Previous Next

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