Re: Patches for elpa-admin

[Philip Kaludercic <philipk@posteo.net>]

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