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

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

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 167 bytes --]

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


[-- Attachment #2: 0001-package-vc-Process-make-and-shell-command-spec-args.patch --]
[-- Type: text/x-diff, Size: 2582 bytes --]

From c51161c51f11e6ffcba17758424596fe44f9d42a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@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)
+
+(defun package-vc--call (destination program &rest args)
+  "Like ‘call-process’ for PROGRAM, DESTINATION, ARGS.
+The INFILE and DISPLAY arguments are fixed as nil."
+  (apply #'call-process program nil destination nil (delq nil args)))
+
+(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*")
+        (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"
+                   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))
+
       ;; Detect a manual
       (when (executable-find "install-info")
         (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
-- 
2.39.2

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

[Philip Kaludercic]

Joseph Turner <joseph@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@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

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

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 4915 bytes --]

Thanks for the review!

Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@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@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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-vc-Process-make-and-shell-command-spec-args.patch --]
[-- Type: text/x-diff, Size: 2763 bytes --]

From f536b42492e1520cc8da4e92b5bb65552ba39e6a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@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 | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 421947b528d..476c38916a8 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -344,6 +344,31 @@ asynchronously."
         "\n")
        nil pkg-file nil 'silent))))
 
+(defcustom package-vc-process-make nil
+  "Whether to process :make and :shell-command spec arguments.
+
+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 (boolean :tag "Run for all packages")
+                 (repeat (symbol :tag "Run for only selected packages")))
+  :version "30.1")
+
+(defun package-vc--make (pkg-spec)
+  "Process :make and :shell-command PKG-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*")
+        (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 +511,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)))
+        ('nil         ; When `nil', do nothing.
+         nil)
+        (_            ; When otherwise non-`nil', run commands.
+         (package-vc--make pkg-spec)))
+
       ;; Detect a manual
       (when (executable-find "install-info")
         (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
-- 
2.39.2

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

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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@gnu.org> wrote:
> 
> Thanks for the review!
> 
> Philip Kaludercic <philipk@posteo.net> writes:
> 
>> Joseph Turner <joseph@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@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>

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

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

Ruijie Yu <ruijie@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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-vc-Process-make-and-shell-command-spec-args.patch --]
[-- Type: text/x-diff, Size: 2830 bytes --]

From 8f9238b841f6deec65fd52d696aa26caf1bf123a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@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 | 36 +++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 421947b528d..95e12fc829a 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -344,6 +344,32 @@ asynchronously."
         "\n")
        nil pkg-file nil 'silent))))
 
+(defcustom package-vc-process-make nil
+  "Whether to process :make and :shell-command spec arguments.
+
+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)
+  "Process :make and :shell-command PKG-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*")
+        (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 +512,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)))
+        ('nil         ; When `nil', do nothing.
+         nil)
+        (_            ; When otherwise non-`nil', run commands.
+         (package-vc--make pkg-spec)))
+
       ;; Detect a manual
       (when (executable-find "install-info")
         (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
-- 
2.39.2

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

[Richard Stallman]

[[[ 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)

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

[Philip Kaludercic]

Joseph Turner <joseph@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

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

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 3378 bytes --]


Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-vc-Process-make-and-shell-command-spec-args.patch --]
[-- Type: text/x-diff, Size: 2941 bytes --]

From b27724197acd4ee72f9d336843f0e6ed9fcee87b Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@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
+  "Whether to process :make and :shell-command spec arguments.
+
+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
+                            (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)))))))
+
 (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)
+        (_            ; 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)))
-- 
2.40.1

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

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@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@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)))

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

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 4703 bytes --]


Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>> From b27724197acd4ee72f9d336843f0e6ed9fcee87b Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@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)))


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-vc-Process-make-and-shell-command-spec-args.patch --]
[-- Type: text/x-diff, Size: 3065 bytes --]

From 3e7084e8e3e3ba142f383e90bfa656f59f3cc1ad Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@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.
+
+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 +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)))
-- 
2.40.1

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

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>
>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>> From b27724197acd4ee72f9d336843f0e6ed9fcee87b Mon Sep 17 00:00:00 2001
>>> From: Joseph Turner <joseph@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@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)))

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

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 2440 bytes --]


Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>
>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>>
>>>>> Joseph Turner <joseph@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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-vc-Process-make-and-shell-command-spec-args.patch --]
[-- Type: text/x-diff, Size: 4126 bytes --]

From 812e32ea6c3f7b2d71174658db0e272b0b4fb84b Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@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.
+
+@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.
+
 @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)))
+
       ;; Detect a manual
       (when (executable-find "install-info")
         (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
-- 
2.40.1

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

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>
>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>>
>>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>>>
>>>>>> Joseph Turner <joseph@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@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)))

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

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>
>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>>
>>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>>>
>>>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>>>>
>>>>>>> Joseph Turner <joseph@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@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)))

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

[Philip Kaludercic]

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.

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

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@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

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

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@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

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

[Philip Kaludercic]

ping?

Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@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