Patches for elpa-admin

Patches for elpa-admin

[Philip Kaludercic]

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


Hi,

I attach two patches for elpa-admin:

The somewhat recent addition to render markdown caused an error when
building a package locally on my system, as the executable "markdown"
was not installed.  So I gathered a number of implementations and
had had `elpaa--section-to-html' use whatever is installed:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-failure-if-no-markdown-compiler-is-installed.patch --]
[-- Type: text/x-diff, Size: 1937 bytes --]

From 1cf5914c41dc73f2e62d04f2ee866a352006806c Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Wed, 13 Apr 2022 09:21:34 +0200
Subject: [PATCH 1/2] Avoid failure if no markdown compiler is installed

* elpa-admin.el (elpaa--markdown-candidates): Add list of markdown implementations
(elpa--markdown-executable): Add function to detect available implementations
(elpaa--section-to-html): Use elpa--markdown-executable
---
 elpa-admin.el | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/elpa-admin.el b/elpa-admin.el
index d570c3c6aa..e86b8d3196 100644
--- a/elpa-admin.el
+++ b/elpa-admin.el
@@ -1257,6 +1257,22 @@ which see."
                      :ext-plist (append '(:html-toplevel-hlevel 3)
                                         elpaa--org-export-options)))
 
+(defvar elpaa--markdown-candidates
+  '("pandoc" "cmark" "marked" "discount"      ;ideally
+    "lowdown" "hoedown" "sundown" "kramdown"  ;backup
+    "markdown.pl" "markdown_py" "md2html.awk" ;fallback
+    "markdown"                                ;despair
+    ))
+
+(defun elpa--markdown-executable ()
+  (catch 'exists
+    (dolist (cmd elpaa--markdown-candidates)
+      (when (executable-find cmd)
+        (throw 'exists cmd)))
+    ;; If no markdown compiler is installed, just display
+    ;; the source without rendering it.
+    "cat"))
+
 (cl-defmethod elpaa--section-to-html ((section (head text/markdown)))
   (with-temp-buffer
     (let ((input-filename
@@ -1264,7 +1280,7 @@ which see."
       (unwind-protect
           (progn
             (write-region (cdr section) nil input-filename)
-            (elpaa--call-sandboxed t "markdown" input-filename))
+            (elpaa--call-sandboxed t (elpa--markdown-executable) input-filename))
         (delete-file input-filename)))
     ;; Adjust headings since this HTML fragment will be inserted
     ;; inside an <h2> section.
-- 
2.30.2


[-- Attachment #3: Type: text/plain, Size: 195 bytes --]


As an alternative to bubblewrap, GNU Guix can also be used for
sandboxing.  It doesn't take much to get it to work and after the
package profile has been prepared it is just as quick as bwrap:


[-- Attachment #4: 0002-Support-sandboxing-using-Guix.patch --]
[-- Type: text/x-diff, Size: 4753 bytes --]

From a571b7bd8b6a48b1343f159287bbc8287c0e8b20 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Wed, 13 Apr 2022 10:29:58 +0200
Subject: [PATCH 2/2] Support sandboxing using Guix

* elpa-admin.el (elpaa--sandbox-mechanism): Add new variable.
(elpaa-read-config): Allow configuring elpaa--sandbox-mechanism.
(elpaa--guix-args): Add new variable, listing all the necessary
  packages for sandboxing.
(elpaa--sandbox-args): Add new generic function to prepare a command.
(elpaa--call-sandboxed): Call elpaa--sandbox-args.
(elpa--markdown-executable): Check elpaa--sandbox-mechanism to set
  what markdown compiler to use.
---
 elpa-admin.el | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/elpa-admin.el b/elpa-admin.el
index e86b8d3196..8efec7bfcf 100644
--- a/elpa-admin.el
+++ b/elpa-admin.el
@@ -55,6 +55,11 @@
 
 (defvar elpaa--sandbox-extra-ro-dirs nil)
 
+(defvar elpaa--sandbox-mechanism
+  (cond ((executable-find "guix") 'guix)
+        ((executable-find "bwrap") 'bwrap))
+  "What mechanism to use for sandboxing.")
+
 (defvar elpaa--sandbox
   ;; Currently sandboxing is implemented using `bwrap' which AFAIK doesn't
   ;; exist for w32 nor for macos, so there's no point defaulting to non-nil
@@ -112,6 +117,7 @@ See variable `org-export-options-alist'.")
               ('email-from		elpaa--email-from)
               ('email-reply-to		elpaa--email-reply-to)
               ('sandbox			elpaa--sandbox)
+              ('sandbox-mechanism       elpaa--sandbox-mechanism)
               ('sandbox-extra-ro-dirs	elpaa--sandbox-extra-ro-dirs)
               ('doc-dir                 elpaa--doc-subdirectory)
               ('debug			elpaa--debug))
@@ -954,9 +960,28 @@ The INFILE and DISPLAY arguments are fixed as nil."
     "--proc" "/proc"
     "--tmpfs" "/tmp"))
 
+(defconst elpaa--guix-args
+  '("shell" "--container" "--pure"
+    ;; List of required packages
+    "coreutils" "emacs-minimal" "cmark" "texinfo" "make"))
+
 (defvar elpaa--sandbox-ro-binds
   '("/lib" "/lib64" "/bin" "/usr" "/etc/alternatives" "/etc/emacs"))
 
+(cl-defmethod elpaa--sandbox-args ((_mechaism (eql bwrap)) args)
+  (let ((dd (expand-file-name default-directory))) ;No `~' allowed!
+    (setq args (cl-list* "--bind" dd dd args)))
+  ;; Add read-only dirs in reverse order.
+  (dolist (b (append elpaa--sandbox-ro-binds elpaa--sandbox-extra-ro-dirs))
+    (when (file-exists-p b)         ;`brwap' burps on binds that don't exist!
+      (setq b (expand-file-name b))
+      (setq args (cl-list* "--ro-bind" b b args))))
+  (append (list "bwrap") elpaa--bwrap-args args))
+
+(cl-defmethod elpaa--sandbox-args ((_mechaism (eql guix)) args)
+  ;; Indicate the remaining arguments are the command to be executed.
+  (append (list "guix") elpaa--guix-args (cons "--" args)))
+
 (defun elpaa--call-sandboxed (destination &rest args)
   "Like ‘elpaa--call’ but sandboxed.
 More specifically, uses Bubblewrap such that the command is
@@ -964,18 +989,9 @@ confined to only have write access to the `default-directory'.
 Signal an error if the command did not finish with exit code 0."
   (if (not elpaa--sandbox)
       (apply #'elpaa--call destination args)
-    (elpaa--message "call-sandboxed %S" args)
-    (let ((dd (expand-file-name default-directory))) ;No `~' allowed!
-      (setq args (nconc `("--bind" ,dd ,dd) args)))
-    ;; Add read-only dirs in reverse order.
-    (dolist (b (append elpaa--sandbox-ro-binds
-                       elpaa--sandbox-extra-ro-dirs))
-      (when (file-exists-p b)         ;`brwap' burps on binds that don't exist!
-        (setq b (expand-file-name b))
-        (setq args (nconc `("--ro-bind" ,b ,b) args))))
-    (let ((exitcode
-           (apply #'elpaa--call destination "bwrap"
-                  (append elpaa--bwrap-args args))))
+    (elpaa--message "call-sandboxed %S [%S]" args elpaa--sandbox-mechanism)
+    (let ((exitcode (apply #'elpaa--call destination
+                           (elpaa--sandbox-args elpaa--sandbox-mechanism args))))
       (unless (eq exitcode 0)
         (if (eq destination t)
             (error "Error-indicating exit code in elpaa--call-sandboxed:\n%s"
@@ -1266,6 +1282,10 @@ which see."
 
 (defun elpa--markdown-executable ()
   (catch 'exists
+    (when (eq elpaa--sandbox-mechanism 'guix)
+      ;; When using Guix, we can ensure what markdown implementation
+      ;; we want to use, so we just return a fixed one here.
+      (throw 'exists "cmark"))
     (dolist (cmd elpaa--markdown-candidates)
       (when (executable-find cmd)
         (throw 'exists cmd)))
-- 
2.30.2


[-- Attachment #5: Type: text/plain, Size: 459 bytes --]


This approach could also be extended to Nix/nix-shell, but I have no
experience with that tool.  This might be of interest to anyone using a
Macintosh system and interested in isolation, as AFAIK Nix works on
those.

(On a related note, is it necessary to sandbox markdown rendering?  I
understand why org can be dangerous, but markdown shouldn't be able to
have any side effects?)

If these changes are fine, I can push them myself.

-- 
	Philip Kaludercic

Re: Patches for elpa-admin

[Stefan Monnier]

> The somewhat recent addition to render markdown caused an error when
> building a package locally on my system, as the executable "markdown"
> was not installed.  So I gathered a number of implementations and
> had had `elpaa--section-to-html' use whatever is installed:

I wouldn't work that hard at it: I think the only important part of the
behavior is either "reproduces faithfully what will happen on
elpa.gnu.org" or "doesn't fail stupidly", so rather than try out all the
possible alternatives the most important part is to fail gracefully.

> +(defvar elpaa--markdown-candidates
> +  '("pandoc" "cmark" "marked" "discount"      ;ideally
> +    "lowdown" "hoedown" "sundown" "kramdown"  ;backup
> +    "markdown.pl" "markdown_py" "md2html.awk" ;fallback
> +    "markdown"                                ;despair
> +    ))

I think this list is "a bit over the top", but feel free to keep it.
OTOH, I think "markdown" should come first since that's what's used on
`elpa.gnu.org`.

> -            (elpaa--call-sandboxed t "markdown" input-filename))
> +            (elpaa--call-sandboxed t (elpa--markdown-executable) input-filename))

I think this is more than 80 columns, please wrap.

> +(cl-defmethod elpaa--sandbox-args ((_mechaism (eql bwrap)) args)
> +  (let ((dd (expand-file-name default-directory))) ;No `~' allowed!
> +    (setq args (cl-list* "--bind" dd dd args)))
> +  ;; Add read-only dirs in reverse order.
> +  (dolist (b (append elpaa--sandbox-ro-binds elpaa--sandbox-extra-ro-dirs))
> +    (when (file-exists-p b)         ;`brwap' burps on binds that don't exist!
> +      (setq b (expand-file-name b))
> +      (setq args (cl-list* "--ro-bind" b b args))))
> +  (append (list "bwrap") elpaa--bwrap-args args))
> +
> +(cl-defmethod elpaa--sandbox-args ((_mechaism (eql guix)) args)
> +  ;; Indicate the remaining arguments are the command to be executed.
> +  (append (list "guix") elpaa--guix-args (cons "--" args)))

There's a typo "mechaism".

More importantly, I understand that `elpaa--sandbox-ro-binds` doesn't
need to be passed to `guix shell`, but I don't understand why
`elpaa--sandbox-extra-ro-dirs` would not be needed either.

>  (defun elpa--markdown-executable ()
>    (catch 'exists
> +    (when (eq elpaa--sandbox-mechanism 'guix)
> +      ;; When using Guix, we can ensure what markdown implementation
> +      ;; we want to use, so we just return a fixed one here.
> +      (throw 'exists "cmark"))
>      (dolist (cmd elpaa--markdown-candidates)
>        (when (executable-find cmd)
>          (throw 'exists cmd)))

I think this optimization is not worth its cost.

> This approach could also be extended to Nix/nix-shell, but I have no
> experience with that tool.

To be honest, I'm not thrilled about adding support for more container
systems to `elpa-admin.el` because it's not its focus.  The containers
are important on `elpa.gnu.org` because I'm a bit paranoid about running
arbitrary software on a central server that could conceivably be
a target for an attack.  But for most other uses it seems that setting
`elpaa--sandbox` to nil will do the job just fine if you don't want to
install `bwrap`.

More interesting would be to add support for this kind of sandboxing in
Emacs itself (so ELisp's Flymake support can use it); and some years
from now `elpa-admin.el` will then be able to ditch its own sandboxing.

> (On a related note, is it necessary to sandbox markdown rendering?

It's necessary if (like me) you don't want to try and convince yourself
that it's currently unnecessary nor want to depend on that fact
remaining true in the future.

> I understand why org can be dangerous, but markdown shouldn't be able
> to have any side effects?)

Yes, and 640kB should be enough for everyone.

> If these changes are fine, I can push them myself.

Feel free,


        Stefan

Re: Patches for elpa-admin

[Philip Kaludercic]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The somewhat recent addition to render markdown caused an error when
>> building a package locally on my system, as the executable "markdown"
>> was not installed.  So I gathered a number of implementations and
>> had had `elpaa--section-to-html' use whatever is installed:
>
> I wouldn't work that hard at it: I think the only important part of the
> behavior is either "reproduces faithfully what will happen on
> elpa.gnu.org" or "doesn't fail stupidly", so rather than try out all the
> possible alternatives the most important part is to fail gracefully.

I guess to that end it should be enough to use cat if markdown isn't
found.

>> +(defvar elpaa--markdown-candidates
>> +  '("pandoc" "cmark" "marked" "discount"      ;ideally
>> +    "lowdown" "hoedown" "sundown" "kramdown"  ;backup
>> +    "markdown.pl" "markdown_py" "md2html.awk" ;fallback
>> +    "markdown"                                ;despair
>> +    ))
>
> I think this list is "a bit over the top", but feel free to keep it.
> OTOH, I think "markdown" should come first since that's what's used on
> `elpa.gnu.org`.

On that topic, what program is markdown?  On Debian there seem to be a
number of alternatives:

        $ apt-file search /usr/bin/markdown | grep /markdown$
        discount: /usr/bin/markdown
        libtext-markdown-perl: /usr/bin/markdown
        markdown: /usr/bin/markdown

Considering that more often than not what we are actually rendering is
"GitHub flavoured markdown" (https://github.github.com/gfm/), it might
make sense to use an implementation that takes this into consideration.

>> -            (elpaa--call-sandboxed t "markdown" input-filename))
>> +            (elpaa--call-sandboxed t (elpa--markdown-executable) input-filename))
>
> I think this is more than 80 columns, please wrap.

Will do.

>> +(cl-defmethod elpaa--sandbox-args ((_mechaism (eql bwrap)) args)
>> +  (let ((dd (expand-file-name default-directory))) ;No `~' allowed!
>> +    (setq args (cl-list* "--bind" dd dd args)))
>> +  ;; Add read-only dirs in reverse order.
>> +  (dolist (b (append elpaa--sandbox-ro-binds elpaa--sandbox-extra-ro-dirs))
>> +    (when (file-exists-p b)         ;`brwap' burps on binds that don't exist!
>> +      (setq b (expand-file-name b))
>> +      (setq args (cl-list* "--ro-bind" b b args))))
>> +  (append (list "bwrap") elpaa--bwrap-args args))
>> +
>> +(cl-defmethod elpaa--sandbox-args ((_mechaism (eql guix)) args)
>> +  ;; Indicate the remaining arguments are the command to be executed.
>> +  (append (list "guix") elpaa--guix-args (cons "--" args)))
>
> There's a typo "mechaism".
>
> More importantly, I understand that `elpaa--sandbox-ro-binds` doesn't
> need to be passed to `guix shell`, but I don't understand why
> `elpaa--sandbox-extra-ro-dirs` would not be needed either.

I think that this is a mistake on my end.

>>  (defun elpa--markdown-executable ()
>>    (catch 'exists
>> +    (when (eq elpaa--sandbox-mechanism 'guix)
>> +      ;; When using Guix, we can ensure what markdown implementation
>> +      ;; we want to use, so we just return a fixed one here.
>> +      (throw 'exists "cmark"))
>>      (dolist (cmd elpaa--markdown-candidates)
>>        (when (executable-find cmd)
>>          (throw 'exists cmd)))
>
> I think this optimization is not worth its cost.

The reason I had to add this was that if using Guix, I cannot rely on
`elpa--markdown-executable' to find what is installed, since that is
evaluated on the host system.

In the end, the entire markdown patch isn't really necessary, if one can
make use of Guix.  All that is needed is to change the markdown package
from "cmark" to "markdown" (which would use Gruber's implementation).

>> This approach could also be extended to Nix/nix-shell, but I have no
>> experience with that tool.
>
> To be honest, I'm not thrilled about adding support for more container
> systems to `elpa-admin.el` because it's not its focus.  The containers
> are important on `elpa.gnu.org` because I'm a bit paranoid about running
> arbitrary software on a central server that could conceivably be
> a target for an attack.  But for most other uses it seems that setting
> `elpaa--sandbox` to nil will do the job just fine if you don't want to
> install `bwrap`.

It is not only that, it is also the dependencies that someone might or
might not have installed when wanting to contribute to ELPA.  By using
Guix, the right tools are automatically provided, which could also be
extended for non-sandbox elpa--call invocations.  The error messages
generated if something goes wrong (e.g. missing makeinfo/install-info,
imagemagick), can be hard to parse, and this approach just circumvents
the issue entirely.

> More interesting would be to add support for this kind of sandboxing in
> Emacs itself (so ELisp's Flymake support can use it); and some years
> from now `elpa-admin.el` will then be able to ditch its own sandboxing.

I have actually been looking into implementing a TRAMP backend that
would tunnel all communication through a Guix shell.  I think in that
case Flymake should be able to make use of that.

>> (On a related note, is it necessary to sandbox markdown rendering?
>
> It's necessary if (like me) you don't want to try and convince yourself
> that it's currently unnecessary nor want to depend on that fact
> remaining true in the future.
>
>> I understand why org can be dangerous, but markdown shouldn't be able
>> to have any side effects?)
>
> Yes, and 640kB should be enough for everyone.

Ok, I see.

>> If these changes are fine, I can push them myself.
>
> Feel free,

I will wait a bit to implement the changes i mentioned.

-- 
	Philip Kaludercic

Re: Patches for elpa-admin

[Stefan Monnier]

>> I think this list is "a bit over the top", but feel free to keep it.
>> OTOH, I think "markdown" should come first since that's what's used on
>> `elpa.gnu.org`.
>
> On that topic, what program is markdown?  On Debian there seem to be a
> number of alternatives:
>
>         $ apt-file search /usr/bin/markdown | grep /markdown$
>         discount: /usr/bin/markdown
>         libtext-markdown-perl: /usr/bin/markdown
>         markdown: /usr/bin/markdown

It's `markdown` :-)

> Considering that more often than not what we are actually rendering is
> "GitHub flavoured markdown" (https://github.github.com/gfm/), it might
> make sense to use an implementation that takes this into consideration.

There are indeed a few pages which rely on features not supported by
`markdown` :-(

`elpa.gnu.org` mostly uses `markdown` because I have no idea which
implementation is better/worse, so I went with the "obvious" choice.

>> I think this optimization is not worth its cost.
> The reason I had to add this was that if using Guix, I cannot rely on
> `elpa--markdown-executable' to find what is installed, since that is
> evaluated on the host system.

Oh, right, I see, sorry.  But I guess this argues against
auto-discovering which executable to use and just rely on
an `elpaa-markdown-program` configuration variable instead (and catch an
execution error, for graceful degradation).

>> To be honest, I'm not thrilled about adding support for more container
>> systems to `elpa-admin.el` because it's not its focus.  The containers
>> are important on `elpa.gnu.org` because I'm a bit paranoid about running
>> arbitrary software on a central server that could conceivably be
>> a target for an attack.  But for most other uses it seems that setting
>> `elpaa--sandbox` to nil will do the job just fine if you don't want to
>> install `bwrap`.
>
> It is not only that, it is also the dependencies that someone might or
> might not have installed when wanting to contribute to ELPA.  By using
> Guix, the right tools are automatically provided, which could also be
> extended for non-sandbox elpa--call invocations.  The error messages
> generated if something goes wrong (e.g. missing makeinfo/install-info,
> imagemagick), can be hard to parse, and this approach just circumvents
> the issue entirely.

You mean to help the users get their local build to work even when they
don't have all the needed packages installed?
Or do you mean to help the users get their local build to fail when they rely
on packages not available in `elpa.gnu.org`?
Or both?

Maybe this can be obtained more reliably with a `guix shell` call
wrapping the whole Emacs invocation rather than only wrapping the
sandboxed subprocesses?
[ You might be able to specify the same Emacs version as the one
  installed on `elpa.gnu.org`.  ]

>> More interesting would be to add support for this kind of sandboxing in
>> Emacs itself (so ELisp's Flymake support can use it); and some years
>> from now `elpa-admin.el` will then be able to ditch its own sandboxing.
> I have actually been looking into implementing a TRAMP backend that
> would tunnel all communication through a Guix shell.  I think in that
> case Flymake should be able to make use of that.

That sounds nice.  We'd need a solution that can be made to work
"everywhere", so the API should be sufficiently generic that it can make
use of various sandboxing systems.

>>> I understand why org can be dangerous, but markdown shouldn't be able
>>> to have any side effects?)
>> Yes, and 640kB should be enough for everyone.
> Ok, I see.

BTW, if we do opt for a more fanciful markdown processor, I wouldn't be
surprised to hear that some of them do "funny" things like follow some
of your symlinks (e.g. to give you warnings if they point nowhere) ;-)

> I will wait a bit to implement the changes i mentioned.

Looking forward to them, thanks.


        Stefan

Re: Patches for elpa-admin

[Brian Cully]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I have actually been looking into implementing a TRAMP backend that
>> would tunnel all communication through a Guix shell.  I think in that
>> case Flymake should be able to make use of that.
>
> That sounds nice.  We'd need a solution that can be made to work
> "everywhere", so the API should be sufficiently generic that it can make
> use of various sandboxing systems.

	I would love a system that can handle this in a
container-agnostic manner. The primary wrinkle I can see right now is
that while ‘guix shell’ will happily spin up and use an environment from
scratch, other containers treat those as separate steps (eg, FreeBSD
jails need to be installed on disk before a ‘jexec’ can be issued in
them).

	I suppose, even Guix needs to have a manifest of some sort to
run ‘guix shell’ against, which is /sort of/ like the install phase of
jails, if you squint hard enough.

-bjc

Re: Patches for elpa-admin

[Philip Kaludercic]

Brian Cully <bjc@spork.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> I have actually been looking into implementing a TRAMP backend that
>>> would tunnel all communication through a Guix shell.  I think in that
>>> case Flymake should be able to make use of that.
>>
>> That sounds nice.  We'd need a solution that can be made to work
>> "everywhere", so the API should be sufficiently generic that it can make
>> use of various sandboxing systems.
>
> 	I would love a system that can handle this in a
> container-agnostic manner. The primary wrinkle I can see right now is
> that while ‘guix shell’ will happily spin up and use an environment from
> scratch, other containers treat those as separate steps (eg, FreeBSD
> jails need to be installed on disk before a ‘jexec’ can be issued in
> them).
>
> 	I suppose, even Guix needs to have a manifest of some sort to
> run ‘guix shell’ against, which is /sort of/ like the install phase of
> jails, if you squint hard enough.

Not necessarily, you can just list the packages you need when starting
the shell

    $ type markdown
    bash: type: markdown: not found
    $ guix shell markdown
    [env] $ which markdown
    markdown is /gnu/store/...

The question is if this requires some separate infrastructure, of if
container support can be supported via TRAMP (e.g. like
https://github.com/emacs-pe/docker-tramp.el does for Docker).

> -bjc

-- 
	Philip Kaludercic

Re: Patches for elpa-admin

[Philip Kaludercic]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> I think this list is "a bit over the top", but feel free to keep it.
>>> OTOH, I think "markdown" should come first since that's what's used on
>>> `elpa.gnu.org`.
>>
>> On that topic, what program is markdown?  On Debian there seem to be a
>> number of alternatives:
>>
>>         $ apt-file search /usr/bin/markdown | grep /markdown$
>>         discount: /usr/bin/markdown
>>         libtext-markdown-perl: /usr/bin/markdown
>>         markdown: /usr/bin/markdown
>
> It's `markdown` :-)
>
>> Considering that more often than not what we are actually rendering is
>> "GitHub flavoured markdown" (https://github.github.com/gfm/), it might
>> make sense to use an implementation that takes this into consideration.
>
> There are indeed a few pages which rely on features not supported by
> `markdown` :-(
>
> `elpa.gnu.org` mostly uses `markdown` because I have no idea which
> implementation is better/worse, so I went with the "obvious" choice.

One alternative would be "pandoc -f gfm".  One critical feature here are
tables (https://github.github.com/gfm/#tables-extension-) that are just
ignored by "markdown".  An advantage is that pandoc can then also be
used to render "plain text" for C-h P.

>>> I think this optimization is not worth its cost.
>> The reason I had to add this was that if using Guix, I cannot rely on
>> `elpa--markdown-executable' to find what is installed, since that is
>> evaluated on the host system.
>
> Oh, right, I see, sorry.  But I guess this argues against
> auto-discovering which executable to use and just rely on
> an `elpaa-markdown-program` configuration variable instead (and catch an
> execution error, for graceful degradation).

Sounds good.

>>> To be honest, I'm not thrilled about adding support for more container
>>> systems to `elpa-admin.el` because it's not its focus.  The containers
>>> are important on `elpa.gnu.org` because I'm a bit paranoid about running
>>> arbitrary software on a central server that could conceivably be
>>> a target for an attack.  But for most other uses it seems that setting
>>> `elpaa--sandbox` to nil will do the job just fine if you don't want to
>>> install `bwrap`.
>>
>> It is not only that, it is also the dependencies that someone might or
>> might not have installed when wanting to contribute to ELPA.  By using
>> Guix, the right tools are automatically provided, which could also be
>> extended for non-sandbox elpa--call invocations.  The error messages
>> generated if something goes wrong (e.g. missing makeinfo/install-info,
>> imagemagick), can be hard to parse, and this approach just circumvents
>> the issue entirely.
>
> You mean to help the users get their local build to work even when they
> don't have all the needed packages installed?
> Or do you mean to help the users get their local build to fail when they rely
> on packages not available in `elpa.gnu.org`?
> Or both?

I was thinking about the first thing, because I don't know what is and
isn't provided on elpa.gnu.org (nor what I insinuating that elpa.gnu.org
should switch to Guix-containers).

> Maybe this can be obtained more reliably with a `guix shell` call
> wrapping the whole Emacs invocation rather than only wrapping the
> sandboxed subprocesses?
> [ You might be able to specify the same Emacs version as the one
>   installed on `elpa.gnu.org`.  ]

If one were to use the package definitions from
https://git.sr.ht/~pkal/guix-emacs-historical, all you would need for
this would be to invoke make like

$ make -e EMACSBIN="guix shell -L path/to/guix-emacs-historical/ emacs-minimal@26.1 -- emacs"

>>> More interesting would be to add support for this kind of sandboxing in
>>> Emacs itself (so ELisp's Flymake support can use it); and some years
>>> from now `elpa-admin.el` will then be able to ditch its own sandboxing.
>> I have actually been looking into implementing a TRAMP backend that
>> would tunnel all communication through a Guix shell.  I think in that
>> case Flymake should be able to make use of that.
>
> That sounds nice.  We'd need a solution that can be made to work
> "everywhere", so the API should be sufficiently generic that it can make
> use of various sandboxing systems.

I'll start a new thread on the mailing list if I ever manage to get
something sensible to work.

>>>> I understand why org can be dangerous, but markdown shouldn't be able
>>>> to have any side effects?)
>>> Yes, and 640kB should be enough for everyone.
>> Ok, I see.
>
> BTW, if we do opt for a more fanciful markdown processor, I wouldn't be
> surprised to hear that some of them do "funny" things like follow some
> of your symlinks (e.g. to give you warnings if they point nowhere) ;-)

Yeah, something like pandoc is feature-rich enough to warrant
sandboxing.

>> I will wait a bit to implement the changes i mentioned.
>
> Looking forward to them, thanks.
>
>
>         Stefan
>

-- 
	Philip Kaludercic

Re: Patches for elpa-admin

[Philip Kaludercic]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I will wait a bit to implement the changes i mentioned.
>
> Looking forward to them, thanks.

I have been experimenting with buffer-env's recent Guix support, and I
think it supersedes everything I had proposed before:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-manifest.scm.patch --]
[-- Type: text/x-diff, Size: 948 bytes --]

From 9179865491899b6863967e0265afc50bf35f1f84 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sat, 21 May 2022 13:31:24 +0200
Subject: [PATCH] Add GNU Guix manifest.scm

---
 manifest.scm | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 manifest.scm

diff --git a/manifest.scm b/manifest.scm
new file mode 100644
index 0000000000..5dfc19941a
--- /dev/null
+++ b/manifest.scm
@@ -0,0 +1,19 @@
+;; GNU Guix manifest for (Non)GNU ELPA
+;;
+;; This file specifies all the packages that are required for the ELPA
+;; build system to function correctly.  You can either use the "guix
+;; shell" command to create an environment with everything prepared.
+
+(specifications->manifest
+ (list "bubblewrap"
+       "coreutils"
+       "emacs-minimal"
+       "git"
+       "grep"
+       "imagemagick"
+       "lzip"
+       "make"
+       "markdown"
+       "tar"
+       "texinfo"))
+
-- 
2.36.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Add-GNU-Guix-manifest.scm.patch --]
[-- Type: text/x-diff, Size: 948 bytes --]

From 6ecb36dbb9c2a87501f4411e29f76f694eca452d Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sat, 21 May 2022 13:32:04 +0200
Subject: [PATCH] Add GNU Guix manifest.scm

---
 manifest.scm | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 manifest.scm

diff --git a/manifest.scm b/manifest.scm
new file mode 100644
index 0000000000..5dfc19941a
--- /dev/null
+++ b/manifest.scm
@@ -0,0 +1,19 @@
+;; GNU Guix manifest for (Non)GNU ELPA
+;;
+;; This file specifies all the packages that are required for the ELPA
+;; build system to function correctly.  You can either use the "guix
+;; shell" command to create an environment with everything prepared.
+
+(specifications->manifest
+ (list "bubblewrap"
+       "coreutils"
+       "emacs-minimal"
+       "git"
+       "grep"
+       "imagemagick"
+       "lzip"
+       "make"
+       "markdown"
+       "tar"
+       "texinfo"))
+
-- 
2.36.1


[-- Attachment #4: Type: text/plain, Size: 122 bytes --]


It might also be possible to add this to the elpa-admin branch, and link
it into elpa/nongnu like like the GNUmakefile.


[-- Attachment #5: 0001-Bind-gnu-directory-as-a-read-only-mount-when-sandbox.patch --]
[-- Type: text/x-diff, Size: 937 bytes --]

From b9127e66e956c94ef30b5f3dd2d9a61d9d2c545b Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sat, 21 May 2022 13:29:19 +0200
Subject: [PATCH 1/2] Bind /gnu directory as a read-only mount when sandboxing

This allows for packages installed and made available using GNU Guix
to be used by bubblewrap.
---
 elpa-admin.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/elpa-admin.el b/elpa-admin.el
index d570c3c6aa..a546bb63ba 100644
--- a/elpa-admin.el
+++ b/elpa-admin.el
@@ -955,7 +955,7 @@ The INFILE and DISPLAY arguments are fixed as nil."
     "--tmpfs" "/tmp"))
 
 (defvar elpaa--sandbox-ro-binds
-  '("/lib" "/lib64" "/bin" "/usr" "/etc/alternatives" "/etc/emacs"))
+  '("/lib" "/lib64" "/bin" "/usr" "/etc/alternatives" "/etc/emacs" "/gnu"))
 
 (defun elpaa--call-sandboxed (destination &rest args)
   "Like ‘elpaa--call’ but sandboxed.
-- 
2.36.1


[-- Attachment #6: Type: text/plain, Size: 479 bytes --]


With a manifest file, all the packages necessary for the ELPA
build-system to work are provided within the new environment.  All that
has to be changed (see last patch) is to tell bubblewrap that the /gnu
directory should be visible, as "guix shell" adds a profile from within
that directory to the PATH.

I also noticed that there are issues when running "make -B" because
mkdir doesn't want to re-create the "packages" directory.  So this
change should be non-controversial:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #7: 0002-Tolerate-if-packages-already-exists.patch --]
[-- Type: text/x-diff, Size: 589 bytes --]

From 05edc183b771611e2e028d00bdb1de437a52b504 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sat, 21 May 2022 13:29:56 +0200
Subject: [PATCH 2/2] Tolerate if packages/ already exists

---
 GNUmakefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/GNUmakefile b/GNUmakefile
index a7d078a1a8..b3d2228900 100644
--- a/GNUmakefile
+++ b/GNUmakefile
@@ -135,7 +135,7 @@ packages/%.elc: packages/%.el
 # $(extra_elcs):; rm $@
 
 packages:
-	mkdir $@
+	mkdir -p $@
 
 include $(PKG_DESCS_MK)
 $(PKG_DESCS_MK): elpa-packages packages
-- 
2.36.1

Re: Patches for elpa-admin

[Philip Kaludercic]

ping?

Philip Kaludercic <philipk@posteo.net> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> I will wait a bit to implement the changes i mentioned.
>>
>> Looking forward to them, thanks.
>
> I have been experimenting with buffer-env's recent Guix support, and I
> think it supersedes everything I had proposed before:
>
> From 9179865491899b6863967e0265afc50bf35f1f84 Mon Sep 17 00:00:00 2001
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sat, 21 May 2022 13:31:24 +0200
> Subject: [PATCH] Add GNU Guix manifest.scm
>
> ---
>  manifest.scm | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 manifest.scm
>
> diff --git a/manifest.scm b/manifest.scm
> new file mode 100644
> index 0000000000..5dfc19941a
> --- /dev/null
> +++ b/manifest.scm
> @@ -0,0 +1,19 @@
> +;; GNU Guix manifest for (Non)GNU ELPA
> +;;
> +;; This file specifies all the packages that are required for the ELPA
> +;; build system to function correctly.  You can either use the "guix
> +;; shell" command to create an environment with everything prepared.
> +
> +(specifications->manifest
> + (list "bubblewrap"
> +       "coreutils"
> +       "emacs-minimal"
> +       "git"
> +       "grep"
> +       "imagemagick"
> +       "lzip"
> +       "make"
> +       "markdown"
> +       "tar"
> +       "texinfo"))
> +
> -- 
> 2.36.1
>
> From 6ecb36dbb9c2a87501f4411e29f76f694eca452d Mon Sep 17 00:00:00 2001
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sat, 21 May 2022 13:32:04 +0200
> Subject: [PATCH] Add GNU Guix manifest.scm
>
> ---
>  manifest.scm | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 manifest.scm
>
> diff --git a/manifest.scm b/manifest.scm
> new file mode 100644
> index 0000000000..5dfc19941a
> --- /dev/null
> +++ b/manifest.scm
> @@ -0,0 +1,19 @@
> +;; GNU Guix manifest for (Non)GNU ELPA
> +;;
> +;; This file specifies all the packages that are required for the ELPA
> +;; build system to function correctly.  You can either use the "guix
> +;; shell" command to create an environment with everything prepared.
> +
> +(specifications->manifest
> + (list "bubblewrap"
> +       "coreutils"
> +       "emacs-minimal"
> +       "git"
> +       "grep"
> +       "imagemagick"
> +       "lzip"
> +       "make"
> +       "markdown"
> +       "tar"
> +       "texinfo"))
> +
> -- 
> 2.36.1
>
>
> It might also be possible to add this to the elpa-admin branch, and link
> it into elpa/nongnu like like the GNUmakefile.
>
> From b9127e66e956c94ef30b5f3dd2d9a61d9d2c545b Mon Sep 17 00:00:00 2001
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sat, 21 May 2022 13:29:19 +0200
> Subject: [PATCH 1/2] Bind /gnu directory as a read-only mount when sandboxing
>
> This allows for packages installed and made available using GNU Guix
> to be used by bubblewrap.
> ---
>  elpa-admin.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/elpa-admin.el b/elpa-admin.el
> index d570c3c6aa..a546bb63ba 100644
> --- a/elpa-admin.el
> +++ b/elpa-admin.el
> @@ -955,7 +955,7 @@ The INFILE and DISPLAY arguments are fixed as nil."
>      "--tmpfs" "/tmp"))
>  
>  (defvar elpaa--sandbox-ro-binds
> -  '("/lib" "/lib64" "/bin" "/usr" "/etc/alternatives" "/etc/emacs"))
> +  '("/lib" "/lib64" "/bin" "/usr" "/etc/alternatives" "/etc/emacs" "/gnu"))
>  
>  (defun elpaa--call-sandboxed (destination &rest args)
>    "Like ‘elpaa--call’ but sandboxed.
> -- 
> 2.36.1
>
>
> With a manifest file, all the packages necessary for the ELPA
> build-system to work are provided within the new environment.  All that
> has to be changed (see last patch) is to tell bubblewrap that the /gnu
> directory should be visible, as "guix shell" adds a profile from within
> that directory to the PATH.
>
> I also noticed that there are issues when running "make -B" because
> mkdir doesn't want to re-create the "packages" directory.  So this
> change should be non-controversial:
>
> From 05edc183b771611e2e028d00bdb1de437a52b504 Mon Sep 17 00:00:00 2001
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sat, 21 May 2022 13:29:56 +0200
> Subject: [PATCH 2/2] Tolerate if packages/ already exists
>
> ---
>  GNUmakefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/GNUmakefile b/GNUmakefile
> index a7d078a1a8..b3d2228900 100644
> --- a/GNUmakefile
> +++ b/GNUmakefile
> @@ -135,7 +135,7 @@ packages/%.elc: packages/%.el
>  # $(extra_elcs):; rm $@
>  
>  packages:
> -	mkdir $@
> +	mkdir -p $@
>  
>  include $(PKG_DESCS_MK)
>  $(PKG_DESCS_MK): elpa-packages packages