[bug#31395] [PATCH 2/2] gnu: Add snap.

[Nicolas Goaziou]

Hello,

Eric Bavier <address@hidden> writes:

> This package looks fun, thanks for working on it, I played with it for
> a while in my browser :)

Heh.

> Just a few comments:

Thank you for the review.

>> +       (uri (string-append
>> +             
>> "https://github.com/jmoenig/Snap--Build-Your-Own-Blocks/archive/";
>> +             version ".tar.gz"))
>
> I think we're trying to stay away from Github's auto-generated tarballs
> now, because they are not guaranteed to remain the same over time.
>
> Unfortunately this project doesn't seem to upload its own release
> tarballs.  I would instead use a git checkout.

Version is a tag, therefore a commit, so I would think it should remain
identical over time. Besides a number of packages use this (e.g.,
audacity...)

Granted, I'm not well-versed in Github technology. Yet, using a git
checkout slightly complicates the package, and its subsequent updates,
so I'd rather only use it if absolutely necessary.

WDYT?

>> +           (mkdir-p share)
>> +           (with-directory-excursion root
>> +             (copy-recursively "." share))
>
> This could be simplified to '(copy-recursively root share)', and the
> mkdir-p can even be left out because copy-recursively will create it
> for you.

OK.

>> +           (let* ((bin (string-append out "/bin"))
>> +                  (script (string-append bin "/snap"))
>> +                  (bash (string-append (assoc-ref %build-inputs "bash")
>> +                                       "/bin/bash"))
>> +                  (xdg-open (string-append (assoc-ref %build-inputs 
>> "xdg-utils")
>> +                                           "/bin/xdg-open"))
>> +                  (snap (string-append share "/snap.html")))
>> +             (mkdir-p bin)
>> +             (call-with-output-file script
>> +               (lambda (port)
>> +                 (format port "#!~a\n~a '~a'" bash xdg-open snap)))
>
> You could maybe use '(which "sh")' and '(which xdg-open)'.  A
> matter of taste, I think.

So (which "sh") should be in the (format ...) call instead of the `bash'
binding above? This is shorter, indeed.

>> +    (native-inputs
>> +     `(("gzip" ,gzip)
>> +       ("tar" ,tar)
>> +       ("js-filesaver" ,js-filesaver)))
>
> js-filesave should go in "inputs", right?

Correct.

>> +    (inputs
>> +     `(("bash" ,bash-minimal)
>> +       ("xdg-utils" ,xdg-utils)))
>> +    (home-page "https://snap.berkeley.edu";)
>> +    (synopsis "Visual, blocks based programming language inspired by 
>> Scratch")
>
> We can leave out "inspired by Scratch" in the synopsis.

OK.

>> +This package provides a @command{snap} executable calling @command{xdg-open}
>> +to open a the application in a web browser.")
>            ^
> s/a the/the/

OK.

Regards,

-- 
Nicolas Goaziou                                                0x80A93738