GNU bug report logs - #48463
gnu: Add j.

Previous Next

Full log

View this message in rfc822 format

From: elaexuotee <at> wilsonb.com
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: Maxime Devos <maximedevos <at> telenet.be>, 48463 <at> debbugs.gnu.org
Subject: [bug#48463] gnu: Add j.
Date: Thu, 13 Jan 2022 16:51:08 +0900

[Message part 1 (text/plain, inline)]

> > +(define-module (gnu packages jsoftware)
> > +  #:use-module ((guix build utils))
> > + [...]
> 
> Double bracketing is pointless, use it only when needed.

Ah, nice catch. I had a bunch of #:select keys and forgot to kill the parens
when removing.

> > +(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. 

Cool. I had a similar sense, but our ideas are a lot sharper than the ones I
had. This actually prompted me to do some code cleanup, leveraging now-me who
has a bit more Guile experience than past-me. At the very least, variable names
should be more descriptive and consistent, overall.

> 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.

Done.

> > +    `(#: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.  

Good idea. Upstream's build scripts condition a *lot* of behaviour on the
j64avx environment variable, so it might not be straightforward, but I will put
this on the to-do list of future improvements.

(Note, the code block quoted here got much simplified in the current patch.)

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

Cheers!

[0001-gnu-Add-j.patch (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.