diff mbox series

[bug#50793,1/2] gnu: Add JSON.sh

Message ID 20210925013934.2763-1-singpolyma@singpolyma.net
State New
Headers show
Series cloudflare-cli and dependency JSON.sh | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue

Commit Message

Stephen Paul Weber Sept. 25, 2021, 1:39 a.m. UTC
* gnu/packages/serialization.scm (JSON.sh): New variable.
---
 gnu/packages/serialization.scm | 45 ++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

Comments

Sarah Morgensen Sept. 25, 2021, 2:58 a.m. UTC | #1
Hi,

Thanks for the patches!  Just a couple quick comments:

Stephen Paul Weber <singpolyma@singpolyma.net> writes:

> * gnu/packages/serialization.scm (JSON.sh): New variable.
> ---
>  gnu/packages/serialization.scm | 45 ++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/gnu/packages/serialization.scm b/gnu/packages/serialization.scm
> index 196141ace8..f5677d9e5f 100644
> --- a/gnu/packages/serialization.scm
> +++ b/gnu/packages/serialization.scm
> @@ -34,10 +34,13 @@
>    #:use-module (guix git-download)
>    #:use-module (guix utils)
>    #:use-module (guix build-system cmake)
> +  #:use-module (guix build-system copy)
>    #:use-module (guix build-system gnu)
>    #:use-module (guix build-system python)
>    #:use-module (gnu packages)
>    #:use-module (gnu packages autotools)
> +  #:use-module (gnu packages base)
> +  #:use-module (gnu packages bash)
>    #:use-module (gnu packages boost)
>    #:use-module (gnu packages check)
>    #:use-module (gnu packages compression)
> @@ -458,6 +461,48 @@ it a convenient format to store user input files.")
>                 (base32
>                  "1180ln8blrb0mwzpcf78k49hlki6di65q77rsvglf83kfcyh4d7z"))))))
>  
> +(define-public JSON.sh
> +  (package
> +    (name "JSON.sh")

I can't find a rule for this per se, but it's convention that package
names to be lowercase (and for the variable name to match).

> +    (version "0d5e5c7")

Since this isn't a tagged version, this should follow the 'git-version'
pattern (search for usages of 'git-version' for examples).

> +    (source
> +     (origin
> +       (method git-fetch)
> +       (uri (git-reference
> +             (url "https://github.com/dominictarr/JSON.sh")
> +             (commit version)))
> +       (file-name (git-file-name name version))
> +       (sha256
> +        (base32
> +         "14lxvp5xbdk0dcwkjbdp098z1108j8z48zaibndh4i731kkcz43i"))))
> +    (build-system copy-build-system)
> +    (arguments
> +     `(#:install-plan '(("JSON.sh" "bin/"))
> +       #:phases
> +       (modify-phases %standard-phases
> +         (add-before 'install 'check
> +           (lambda _
> +             (invoke "./all-tests.sh")
> +             #t))

Check phases should respect #:tests?, like so:

  (lambda* (#:key tests? #:allow-other-keys)
    (when tests?
      [...]))

Also, it doesn't hurt, but phases no longer have to end in #t. :)

> +         (add-after 'install 'wrap-program
> +           (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (wrap-program (string-append (assoc-ref outputs "out") "/bin/JSON.sh")
> +               `("PATH" ":" prefix
> +                 (,(string-join
> +                    (map (lambda (in) (string-append (assoc-ref inputs in) "/bin"))
> +                         '("grep" "coreutils"))

Does this script actually use coreutils?  On the other hand, it looks
like it does use gawk and sed.

> +                    ":")))))))))
> +    (inputs
> +     `(("bash-minimal" ,bash-minimal)
> +       ("grep" ,grep)
> +       ("coreutils" ,coreutils)))
> +    (synopsis
> +      "Pipeable JSON parser written in shell")
> +    (description
> +      "JSON parser written in shell, compatible with ash, bash, dash and zsh")

This is a bit nit-picky since it's just a dependency, but descriptions
should use full sentences, and be a bit more descriptive than this (see
"Synopses and Descriptions" in the Guix manual).

> +    (home-page "https://github.com/dominictarr/JSON.sh")
> +    (license license:asl2.0)))

This is actually dual-licensed with expat, so:

  (license (list license:expat license:asl2.0))

Could you please send an updated patch?

--
Sarah
Stephen Paul Weber Sept. 26, 2021, 1:08 a.m. UTC | #2
>> +(define-public JSON.sh
>> +  (package
>> +    (name "JSON.sh")
>
>I can't find a rule for this per se, but it's convention that package
>names to be lowercase (and for the variable name to match).

Ok. Should I keep the . for "json.sh" or go with "jsonsh" ?  Should I change the
script/command name to match?

>> +    (version "0d5e5c7")
>
>Since this isn't a tagged version, this should follow the 'git-version'
>pattern (search for usages of 'git-version' for examples).

Will do.

>> +         (add-before 'install 'check
>> +           (lambda _
>> +             (invoke "./all-tests.sh")
>> +             #t))
>
>Check phases should respect #:tests?, like so:
>
>  (lambda* (#:key tests? #:allow-other-keys)
>    (when tests?
>      [...]))

Ok

>> +                         '("grep" "coreutils"))
>
>Does this script actually use coreutils?

It uses printf

> On the other hand, it looks >like it does use gawk

gawk is only used if egrep is not present

> and sed.

Missed this, will add.
Sarah Morgensen Sept. 26, 2021, 3:02 a.m. UTC | #3
Hi,

Stephen Paul Weber <singpolyma@singpolyma.net> writes:

>>> +(define-public JSON.sh
>>> +  (package
>>> +    (name "JSON.sh")
>>
>>I can't find a rule for this per se, but it's convention that package
>>names to be lowercase (and for the variable name to match).
>
> Ok. Should I keep the . for "json.sh" or go with "jsonsh" ?  Should I change the
> script/command name to match?

I'd keep the dot in the package name.  We don't want to modify the
actual script name in the package, though (we try to leave the actual
contents of packages as unmodified as possible).

>
>>> +                         '("grep" "coreutils"))
>>
>>Does this script actually use coreutils?
>
> It uses printf

'printf' should just be calling the shell builtin:

--8<---------------cut here---------------start------------->8---
$ type printf
printf is a shell builtin
--8<---------------cut here---------------end--------------->8---

--
Sarah
Stephen Paul Weber Sept. 26, 2021, 11:32 p.m. UTC | #4
>'printf' should just be calling the shell builtin:
>
>--8<---------------cut here---------------start------------->8---
>$ type printf
>printf is a shell builtin
>--8<---------------cut here---------------end--------------->8---

Oh of course, silly me, I'll submit a version without coreutils input.
diff mbox series

Patch

diff --git a/gnu/packages/serialization.scm b/gnu/packages/serialization.scm
index 196141ace8..f5677d9e5f 100644
--- a/gnu/packages/serialization.scm
+++ b/gnu/packages/serialization.scm
@@ -34,10 +34,13 @@ 
   #:use-module (guix git-download)
   #:use-module (guix utils)
   #:use-module (guix build-system cmake)
+  #:use-module (guix build-system copy)
   #:use-module (guix build-system gnu)
   #:use-module (guix build-system python)
   #:use-module (gnu packages)
   #:use-module (gnu packages autotools)
+  #:use-module (gnu packages base)
+  #:use-module (gnu packages bash)
   #:use-module (gnu packages boost)
   #:use-module (gnu packages check)
   #:use-module (gnu packages compression)
@@ -458,6 +461,48 @@  it a convenient format to store user input files.")
                (base32
                 "1180ln8blrb0mwzpcf78k49hlki6di65q77rsvglf83kfcyh4d7z"))))))
 
+(define-public JSON.sh
+  (package
+    (name "JSON.sh")
+    (version "0d5e5c7")
+    (source
+     (origin
+       (method git-fetch)
+       (uri (git-reference
+             (url "https://github.com/dominictarr/JSON.sh")
+             (commit version)))
+       (file-name (git-file-name name version))
+       (sha256
+        (base32
+         "14lxvp5xbdk0dcwkjbdp098z1108j8z48zaibndh4i731kkcz43i"))))
+    (build-system copy-build-system)
+    (arguments
+     `(#:install-plan '(("JSON.sh" "bin/"))
+       #:phases
+       (modify-phases %standard-phases
+         (add-before 'install 'check
+           (lambda _
+             (invoke "./all-tests.sh")
+             #t))
+         (add-after 'install 'wrap-program
+           (lambda* (#:key inputs outputs #:allow-other-keys)
+             (wrap-program (string-append (assoc-ref outputs "out") "/bin/JSON.sh")
+               `("PATH" ":" prefix
+                 (,(string-join
+                    (map (lambda (in) (string-append (assoc-ref inputs in) "/bin"))
+                         '("grep" "coreutils"))
+                    ":")))))))))
+    (inputs
+     `(("bash-minimal" ,bash-minimal)
+       ("grep" ,grep)
+       ("coreutils" ,coreutils)))
+    (synopsis
+      "Pipeable JSON parser written in shell")
+    (description
+      "JSON parser written in shell, compatible with ash, bash, dash and zsh")
+    (home-page "https://github.com/dominictarr/JSON.sh")
+    (license license:asl2.0)))
+
 (define-public capnproto
   (package
     (name "capnproto")