[bug#48463] gnu: Add j.

[Liliana Marie Prikler]

Hi,

> +(define-module (gnu packages jsoftware)
> +  #:use-module ((guix build utils))
> + [...]

Double bracketing is pointless, use it only when needed.

> +(define* (make-j #:key
> +                 (builder "guix.gnu.org")
> +                 vername
> +                 revision
> +                 hash
> +                 (type 'release)
> +                 commit
> +                 (patches '())
> +                 (extra-inputs '())
> +                 (extra-envars '()))
> + (package
> +   (name (jname "jsoftware-j" type))
> +   (version (jversion->string vername revision))
> +   (source
> +    (origin
> +      (method git-fetch)
> +      (uri (git-reference
> +            (url "https://github.com/jsoftware/jsource";)
> +            (commit (or commit (jinfo->git-tag vername type
> revision))))
Vername sounds a little weird, make that version-base or something
clearer.  Also, the argument #:commit is used in an unclear fashion --
if you were to pass an actual commit hash to it, it'd still be treated
as a release and not be using git-version. 

On a related note
> +(define (jversion->string version revision)
> +  "Return a string representation of a J version and (optional)
> revision pair."
> +  (let ((postfix (if (not revision) ""
> +                   (string-append "." revision))))
> +    (string-append version postfix)))
should also take commit and revision should probably be dashed.  In
that way, when packaging commits between releases we can use
"jrevision.guix-revision" as the complete revision.

In short, I'd add a #:tag argument to override the tag and treat commit
like a let-bound commit.

> +    `(#:modules (((ice-9 ftw) #:select (scandir))
> +                 ((ice-9 popen) #:select (open-pipe* close-pipe))
> +                 ((ice-9 regex) #:select (match:substring string-
> match))
> +                 ((ice-9 threads) #:select (parallel par-for-each))
> +                 ((srfi srfi-26) #:select (cut))
> +                 ((srfi srfi-1) #:select (fold))
> +                 ,@%gnu-build-system-modules)
It's nice that you annotated all those, but note that it probably
wouldn't have been needed.  If you notice this list getting longer and
longer as you update, consider dropping the #:selects.

> +        (replace 'build
> +          (lambda _
> +            (setenv "USE_OPENMP" "1")
> +            (setenv "USE_THREAD" "1")
> +            (for-each (lambda (var-val) (apply setenv var-val))
> +                      (quote ,extra-envars))
> +            ;; The build scripts assume that PWD is make2.
> +            (with-directory-excursion "make2"
> +              (let* ((platform ,(if (target-arm?) "raspberry"
> "linux"))
> +                     (jplat (string-append "jplatform=" platform))
> +                     (target-bit ,(if (target-64bit?) "64" "32"))
> +                     (jbit (string-append "j64x=" "j" target-bit))
> +                     (jbit-avx (string-append jbit "avx"))
> +                     (jbit-avx2 (string-append jbit "avx2")))
> +                (parallel
> +                  ;; Since jconsole doesn't depend on AVX features,
> we just
> +                  ;; build it once.
> +                  (invoke "env" jplat jbit "./build_jconsole.sh")
> +                  (invoke "env" jplat jbit "./build_libj.sh")
> +                  (if ,(target-64bit?)
> +                    (parallel
> +                      (invoke "env" jplat jbit-avx
> "./build_libj.sh")
> +                      (invoke "env" jplat jbit-avx2
> +                              "./build_libj.sh"))))))))
Maxime already made a comment w.r.t. 32bit AVX here, but I think this
would be a prime example to use the CPU tuning that was recently
landed.  

Most of the above (except the semantics of the make-j keyword
arguments) are not blockers in my opinion.

Cheers