[bug#43080] [PATCH] gnu: add j version 902.

[Ludovic Courtès]

Hi Joseph,

Joseph Novakovich <josephnovakovich@gmail.com> skribis:

> From 798d0e3c2b635cfe3a936937fd3dd3ea1b8c5812 Mon Sep 17 00:00:00 2001
> From: Joseph Novakovich <josephnovakovich@gmail.com>
> Date: Thu, 27 Aug 2020 19:16:16 -0400
> Subject: [PATCH] gnu: add j version 902.
>
> ---
>  gnu/packages/j.scm | 180 +++++++++++++++++++++++++++++++++++++++++++++

Sorry for the loooong delay!

A few comments—if needed we can make those changes on your behalf, let
us know.

Could you add j.scm to ‘gnu/local.mk’?

> +    (name "j")
> +    (version "902")
> +    (source
> +     (origin
> +       (method git-fetch)
> +       (uri
> +        (git-reference
> +         (url "https://github.com/jsoftware/jsource";)
> +         (commit "59324abbc6c9c3709d39096f5a41e0a4ef28e9f6")))

Why this commit?  If there’s a tag, it would be clearer to refer to the
tag (the ‘commit’ field can name a tag).

> +    (outputs '("out"))

Unnecessary, you can remove it.

> +     `(#:modules
> +       ((guix build gnu-build-system)
> +        (guix build utils))

Likewise.

> +           (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (let ((jplatform "linux")
> +                   (j64x "j64avx2"))
> +               (chdir "make2")
> +               (system
> +                (format #f
> +                        "jplatform=~a j64x=~a USE_SLEEF=1 
> ./build_jconsole.sh"
> +                        jplatform j64x))

Note that we cannot assume AVX2 support (on x86_64, but especially on
non-Intel platforms).  Can you check this doesn’t create binaries that
require AVX2?

Also, it would be more robust to replace ‘system’ by something like:

  (Seton "jplatform" "linux")
  (setenv "USE_SLEEF" "1")
  …
  (invoke "./build_jconsole.sh")

since ‘invoke’ checks the exit code of the given command.

> +           (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (let* ((bin-out (string-append (assoc-ref %outputs "out") 
> "/bin"))
> +                    (share-out (string-append (assoc-ref %outputs "out")
> +                                              "/share/j"))
> +                    (jconsole "jlibrary/bin/jconsole")
> +                    (libj.so  "jlibrary/bin/libj.so"))

I’d omit ‘-out’ from variable names.

> +    (synopsis "APL Dialect")

What about “Dialect of the APL programming language” for clarity?

> +    (description "J is a programming language that works with arrays,
> +verbs, adverbs, and conjunctions.  For example, +/x sums array x and
> +/:~x sorts it.")

Please use @code markup for code snippets.  Bonus points if you can
expound a bit!

  https://guix.gnu.org/manual/en/html_node/Synopses-and-Descriptions.html

> +    (license gpl3)))

I confirm that this is “version 3 only” per ‘license.txt’.

Could you send an updated patch?

Thanks,
Ludo’.