[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#31395] [PATCH 2/2] gnu: Add snap.
From: |
Nicolas Goaziou |
Subject: |
[bug#31395] [PATCH 2/2] gnu: Add snap. |
Date: |
Thu, 10 May 2018 15:10:00 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
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
- [bug#31395] [PATCH 0/2] Add Snap! educational programming language, Nicolas Goaziou, 2018/05/09
- [bug#31395] [PATCH 1/2] gnu: Add js-filesaver., Nicolas Goaziou, 2018/05/09
- [bug#31395] [PATCH 2/2] gnu: Add snap., Nicolas Goaziou, 2018/05/09
- [bug#31395] [PATCH 2/2] gnu: Add snap., Eric Bavier, 2018/05/09
- [bug#31395] [PATCH 2/2] gnu: Add snap.,
Nicolas Goaziou <=
- [bug#31395] [PATCH 2/2] gnu: Add snap., Eric Bavier, 2018/05/10
- [bug#31395] [PATCH 2/2] gnu: Add snap., Nicolas Goaziou, 2018/05/10
- [bug#31395] [PATCH 2/2] gnu: Add snap., Eric Bavier, 2018/05/10
- bug#31395: [PATCH 2/2] gnu: Add snap., Nicolas Goaziou, 2018/05/11