Subject: Re: bug#65313: [PATCH 0/1] gnu: Add erlang-lfe.

From: Maxim Cournoyer <[email-masked]>
Date: Tue, 05 Sep 2023 09:41:58 -0400

tags 65313 + moreinfo
quit

Hello,

Tim Johann <[email-masked]> writes:

> ---
>  gnu/packages/erlang.scm | 77 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>
> diff --git a/gnu/packages/erlang.scm b/gnu/packages/erlang.scm
> index a7f161647b..df7212b0e5 100644
> --- a/gnu/packages/erlang.scm
> +++ b/gnu/packages/erlang.scm
> @@ -738,3 +738,80 @@ (define-public rebar3-proper
>      (description "This plugin allows running PropEr test suites from within
>  rebar3.")
>      (license license:bsd-3)))
> +
> +(define-public erlang-lfe
> +  (package
> +    (name "erlang-lfe")
> +    (version "2.1.2")
> +    (source
> +     (origin
> +       (method git-fetch)
> +       ;; The tarball from http://erlang.org/download contains many
> +       ;; pre-compiled files, so we use this snapshot of the source
> +       ;; repository.
> +       (uri (git-reference
> +             (url "https://github.com/lfe/lfe")
> +             (commit "v2.1.2")))
> +       (file-name (git-file-name name version))
> +       (sha256
> +        (base32
> +         "180hz1p2v3vb6yyzcfwircmljlnd86ln8z80lzy3mwlyrcxblvxy"))))
> +    (build-system gnu-build-system)
> +    (native-inputs
> +     (list rebar3 rebar3-proper erlang-proper))
> +    (propagated-inputs
> +     (list erlang))

Nitpick: less than 5 dependencies can fit be formatted on the same line,
if it fits, e.g.

  (native-inputs (list rebar3 rebar3-proper erlang-proper))

Otherwise the convention is to use something like

  (native-inputs
   (list package1
         package2
         ...))

Or let 'guix style' take care of it.

> +    (arguments
> +     `(#:phases

New packages should use gexps for arguments, so something like
       (arguments
        (list #:phases #~(modify-phases %standard-phases
                          ...)))

> +       (modify-phases %standard-phases
> +         (delete 'configure)
> +
> +         ;; The following is inspired by rebar-build-system.scm
> +         (add-before 'check 'erlang-depends
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             (use-modules (ice-9 ftw))
> +             (define input-directories
> +               (map (lambda (p) (cdr (assoc p inputs)))
> +                    '("rebar3-proper" "erlang-proper")))

It's not typically safe to call use-modules elsewhere than at the top
level (it would cause problems with macros).  For imports, you should
use the #:modules build argument.  Here you could also simply use

(define input-directories (list #$(this-package-input "rebar3-proper")
                                #$(this-package-input "erlang-proper")))

since there are only 2 items.

> +             (mkdir-p "_checkouts")
> +             (for-each
> +              (lambda (input-dir)
> +                (let ((elibdir (string-append input-dir "/lib/erlang/lib")))
> +                  (when (directory-exists? elibdir)
> +                    (for-each
> +                     (lambda (dirname)
> +                       (let ((src (string-append elibdir "/" dirname))
> +                             (dest (string-append "_checkouts/" dirname)))
> +                         (when (not (file-exists? dest))
> +                           ;; symlinking will not work, since rebar3 will try
> +                           ;; to overwrite the _build directory several times
> +                           ;; with the contents of _checkout, so we copy the
> +                           ;; directory tree to _checkout and make it writeable.
> +                           (copy-recursively src dest #:follow-symlinks? #t)
> +                           (for-each
> +                            (lambda (file)
> +                              (chmod file #o777))
> +                            (find-files dest)))))

If you add (srfi srfi-26) to #:modules, you can express this more
succintly with:

                              (for-each (cut chmod <> #o777)
                                        (find-files dest))

> +                     (scandir elibdir
> +                              (lambda (file)
> +                                (and (not (member file '("." "..")))
> +                                     (file-is-directory? (string-append elibdir "/" file)))))))))

Please format the code to stay within 80 columns of text.  'guix lint'
should report that.

> +              input-directories)))
> +
> +         (replace 'check
> +           (lambda* (#:key tests? #:allow-other-keys)
> +             (when tests?
> +               (begin
> +                 ;;(setenv "DEBUG" "1")
> +                 (setenv "REBAR_CACHE_DIR" "/tmp")
> +                 (invoke "make" "tests"))))))

If the tests take time and they can not in parallel, the "-j" option
should be provided with the (number->string (parallel-job-count)) value.

> +       #:make-flags
> +       (list (string-append "PREFIX=" (assoc-ref %outputs "out"))
> +             "CC=gcc")))

This should use gexps as well, replacing the assoc-ref above with just
#$output.

-- 
Thanks,
Maxim