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