Subject: Re: bug#47459: [PATCH] gnu: Add brython.

From: Ludovic Courtès <[email-masked]>
Date: Thu, 01 Apr 2021 14:26:46 +0200

Hi,

"jgart" <[email-masked]> skribis:

> This was an effort of today's Guix Packaging Meetup.

Nice!

> From 519c8d5e5369744cadbe4bc7607b0c4c8f05128a Mon Sep 17 00:00:00 2001
> From: LibreMiami <[email-masked]>
> Date: Sun, 28 Mar 2021 21:13:31 -0400
> Subject: [PATCH 2/2] gnu: Add brython.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> * gnu/packages/web.scm (brython): New variable.
>
> Co-authored-by: Léo Le Bouter <[email-masked]>
> Co-authored-by: jgart <[email-masked]>
> Co-authored-by: Ryan Prior <[email-masked]>
> Co-authored-by: Andrea Pierré <[email-masked]>

[...]

> +    (native-inputs
> +     `(; 'install phase needs the python command
> +       ("python" ,((package-input-rewriting
> +                    `((,python . ,python-3.9))
> +                    #:deep? #f) python-wrapper))))

That’s likely to perform very poorly.  Normally, you’d save the result
of ‘package-input-rewriting’ (a procedure) in a variable and reuse it
anytime you need to make that transformation, so that caching can kick
in.

> +    (arguments
> +     `(#:python ,python-3.9 ; required during build

In current ‘master’, all Python 3.x package are built against 3.8.  I
think we should stick to that and not introduce different dependencies
as this can only lead to bad surprises.

Can Brython be built against 3.8?  If not, I think it’ll have to go to
the ‘core-updates’ branch, which may be merged soon after the release
(end of April).

WDYT?

> +           (lambda* (#:key outputs #:allow-other-keys)
> +             ;; write access is required to .js files for minifying

Nitpick: Please capitalize sentences and end with a period (here and
elsewhere).

> +             (for-each
> +              (lambda (vv) (chmod vv #o644))

Use ‘make-file-writable’ instead of this lambda.

> +             (invoke "python3" "scripts/make_dist.py")
> +             #t))

No need for the trailing #t.

> +    (synopsis
> +      "Run python code in a web browser")

This can be on a single line.

> +    (description
> +      "@code{Brython} is a compliant implementation of Python 3 that allows

You can remove @code here (@code is only used for inline code snippets
and similar things).

Thanks!

Ludo’.