Subject: Re: [bug#54021] [PATCH] Add rhino javascript package

From: Maxime Devos <[email-masked]>
Date: Wed, 16 Feb 2022 18:21:38 +0100

Frank Pursel schreef op di 15-02-2022 om 17:58 [-0800]:
> +(define-public rhino
> +  (let* ((rel-ver "1.7.7.2") (rel-git-tag "Rhino1_7_7_2_Release")
> +         (git-commitv "935942527ff434b205e797df4185518e5369466e")
> +         (git-short-commit (substring git-commitv 0 6))
> +         (hash "09i4yr98hs6855fs7fhgmrpiwpr90lhxdv2bvfj97nn4rv1d7wl8"))

This is quite a bit more complicated than necessary ...

 * 'rel-git-tag' is unused.
 * Conventionally, a let-bound commit string has as name 'commit', not
   'git-commitv'.  (guix upstream) expects 'commit', not 'git-commitv',
   and will fail at auto-updating if a different name is used.
 * (see later)

> +    (package
> +      (name "rhino")
> +      (version git-short-commit)

'git-short-commit' is a (shortened) commit string, not a version
number.  This needs to be (version "1.7.7.2") instead.

> +      (source (origin
> +                (method git-fetch)
> +                (uri (git-reference
> +                      (url "https://github.com/mozilla/rhino.git")
> +                      (commit git-short-commit)))

There is no need to shorten the commit string, you can use the full
"git-commitv" here.

> +                (file-name (git-file-name name git-short-commit))

We have a version number, so you can do (git-file-name name version)
here.

> +                (sha256
> +                 (base32
> +                  hash))))

The hash is only used in one place, so you can write it here directly.
This has as benefit that (guix packages) can do some checks on the hash
at compile time.

[...]

> +      (arguments
> +       `(#:phases (modify-phases %standard-phases
> +                    (add-after 'unpack 'clean-jars
> +                      (lambda _
> +                        (for-each (lambda (jarf)
> +                                    (delete-file jarf)
> +                                    (format #t "Deleted: ~s
> +" jarf))
> +                                  (find-files "." ".*\\.jar$")) #t))

Cleaning the source code of binaries, making sure that the source code
actually consists of source code, seems more something for origin
snippets.  It's not really explicitly mentioned anywhere I think,
but the following from ‘(guix)Snipepts versus Phases’ seems close:

The boundary between using an origin snippet versus a build phase to
modify the sources of a package can be elusive.  Origin snippets are
typically used to remove unwanted files such as bundled libraries,
nonfree sources, or to apply simple substitutions.  [...]

> +                    (replace 'install
> +                      (lambda* (#:key inputs outputs #:allow-other-keys)
> +                        (let* ((out (assoc-ref outputs "out"))
> +                               (pkg+ver (string-append ,name ,rel-ver))

You can refer to the 'version' field of the package here:

  (pkg+ver (string-append ,name ,version))

> +                               (bin (string-append out "/bin"))
> +                               (rhino (string-append bin "/rhino")))
> +                          (mkdir-p bin)
> +                          (install-file (string-append "build/" pkg+ver
> +                                                       "/js.jar")
> +                                        (string-append out "/share/java"))
> +                          (with-output-to-file rhino
> +                            (lambda _
> +                              (format #t "#!~a
> +~a -jar ~a $@
> +"
> +                                      (search-input-file inputs "/bin/bash")

'bash-minimal' is missing from 'inputs'.  Not including it only works
when compiling natively.

> +                                      (which "java")
> +                                      (string-append out "/share/java/js.jar"))))

'format' has a port argument, so you could do

(call-with-output-file rhino
  (lambda (port)
    (format port "#!~a ~a -jar -a $@"
            (search-input-file [...]) [...])))

Also, what's this "$@"?

Greetings,
Maxime.