Subject: Re: [bug#64891] [PATCH] gnu: Add hare

From: "(" <[email-masked]>
Date: Fri, 28 Jul 2023 14:14:46 +0100

Hi!

Ekaitz Zarraga <[email-masked]> writes:
> * gnu/packages/hare.scm (hare, harec): New variable.
> * gnu/local.mk: Add hare.scm

:)

> +          (sha256
> +            (base32 "1xdf9pdwy5dvxhgvw6hqkpnjl8z76xc413vsnbbqfvwmfing53yn"))))
              ^
Odd spacing here.

> +      (build-system gnu-build-system)
> +      (arguments
> +        `(#:phases

Please use gexps and LIST for any new packages.

> +          (modify-phases %standard-phases
> +            (add-before 'configure 'setenv
> +              (lambda _
> +                (setenv "CC" ,(cc-for-target))))

Hmm.  In my version of the package[1], I also have:

  (setenv "AR" #$(ar-for-target))
  (setenv "LD" #$(ld-for-target))

Not sure why I added those, though.  Maybe try cross-compiling both
packages?  Perhaps these lines are needed for that...

> +            (add-after 'configure 'configure-better
> +              (lambda* (#:key outputs #:allow-other-keys)
> +                (invoke "./configure"
> +                        (string-append "--prefix=" (assoc-ref outputs "out")))))
> +            (delete 'configure))))

You could just (replace 'configure ...) instead :)

> +simple, stable, and robust.  Hare uses a static type system, manual memory

s/Hare/It/ in this line, maybe?

> +(define-public hare

Each package should have its own commit, remember.

> +  (let ((revision "1")
> +        (commit  "112b75eb56d98c442094966fe59cbf3cd5feb909"))
                   ^
Rouge space.

> +          (sha256
> +            (base32 "0ilnz56757k4v0ggdc2v4xsjw0c944y8r289nfmqhw4hwzsrvb4j"))))
              ^
Another.

> +      (inputs (list scdoc))

scdoc should be a native-input.

> +      (propagated-inputs (list qbe harec))

Might be an idea to make QBE and harec regular inputs, using substitute*
to replace any invocations of their binaries within the code with the
full /gnu/store/.../bin/blah path.  Propagated inputs should be avoided
where possible, even if it makes the package definition a little more
complex.

> +      (arguments
> +        `(#:make-flags `("HARECACHE=./cache"
> +                         ,(string-append "PREFIX=" (assoc-ref %outputs "out")))

Again, please use gexps within LIST, and replace
``(assoc-ref %outputs "out")'' with ``#$output''.

> +          #:phases
> +          (modify-phases %standard-phases
> +            (add-before 'configure 'configure-make
> +              (lambda _
> +                ;; NOTE: We are not really using this because overwriting it via
> +                ;; `make-flags` is way easier with our tooling.
> +                ;; Maybe we should?

No, I think what you do here is fine.

> +                (copy-file "config.example.mk" "config.mk")))

You can use RENAME-FILE rather than COPY-FILE here.

> +            (delete 'configure))))

Again, please just use REPLACE; it's simpler.

> +      (synopsis "Hare build driver")

Maybe "Build driver for the Hare language"?  "Hare build driver" is
almost a bit *too* short, IMO.

> +      (description "Hare is a systems programming language designed to be
> +simple, stable, and robust.  Hare uses a static type system, manual memory
> +management, and a minimal runtime.  It is well-suited to writing operating
> +systems, system tools, compilers, networking software, and other low-level,
> +high performance tasks.")

Perhaps trim the language description down a bit and add a sentence or
two about what *this specific tool* does.

  -- (