[bug#52189] [PATCH] gnu: Add notcurses

[Nicolas Goaziou]

Hello,

Blake Shaw via Guix-patches via <guix-patches@gnu.org> writes:

Thank you. Some comments follow.

>  gnu/packages/notcurses.scm | 71 ++++++++++++++++++++++++++++++++++++++

I don't think we should create a new file just for this package. Also,
new files need to be registered in "gnu/local.mk".

Maybe this should go into... "ncurses.scm" (!). At a later time, we may
rename ncurse.scm into tui.scm or some such.

> +(define-public notcurses
> +  (let ((commit "d15eb6003cbd65f11163916261cf6cd5600c77fa"))

Upstream use tags. It might be more readable. You'll need a variable for
the code name, tho. In any case, a comment is warranted explaining the
situation.

> +         (sha256
> +          (base32
> +           "10jf6iai1r0xafrgaz978y9bqlaw1gvd11gc0yynwwp6rcs97g17"))))

Nitpick: string should go on the same line as base32.

> +      (build-system cmake-build-system)
> +      (arguments
> +       `(#:tests? #f
> +                  #:build-type "-DVAR=val"

Indentation is off. You may want to use "etc/indent-code.el" script.
The build-type value above is suspicious.

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

This is not cross-compilation friendly. The above should be:

  (list ,(string-append "CC=" (cc-for-target))
        (string-append "prefix=" ...))

> +                  #:configure-flags
> +                  (map (lambda (s)
> +                         (string-append "-D" s))
> +                       '("USE_CPP=off"     "USE_COVERAGE=off"
> +                         "USE_DOXYGEN=off" "USE_DOCTEST=off"
> +                         "USE_GPM=off"     "USE_MULTIMEDIA=ffmpeg"
> +                         "USE_PANDOC=off"  "FSG_BUILD=ON"))
> +                  #:phases
> +                  (modify-phases %standard-phases
> +                    (add-before 'build 'patch-makefile-shell
> +                      (lambda _
> +                        (setenv "HOME" "/tmp")))

Is the phase above required for tests? If so, could you add a comment
about it?

> +                    (add-before 'build 'set-prefix-in-makefile
> +                      (lambda* (#:key outputs #:allow-other-keys)
> +                        (let ((out (assoc-ref outputs "out")))
> +                          (substitute* "Makefile"
> +                            (("PREFIX =.*")
> +                             (string-append "PREFIX = " out "\n")))
> +                          #true))))))

The trailing #true is not required anywore. You can drop it.

> +      (native-inputs
> +       `(("ncurses" ,ncurses)
> +         ("gcc-toolchain" ,gcc-10)

Could you explain why gcc-10 must be used?

> +         ("pkg-config" ,pkg-config)))
> +      (inputs
> +       `(("zlib" ,zlib)
> +         ("ffmpeg" ,ffmpeg)
> +         ("libunistring" ,libunistring)))

Pleas order inputs alphabetically.

> +      (synopsis "Not-ncurses: A library facilitating complex TUIs on modern 
> terminals")

I suggest:

  "Library for building textual user interfaces on modern terminals"

> +      (description "Supporting vivid colors, multimedia, threads, & Unicode 
> to the max.")

The description is not terribly useful, and sounds like an ad. Maybe:

  Notcurses is a library for building complex, textual user interfaces
  (TUIs) on modern terminal emulators. It does not use Ncurses, though
  it does make use of libtinfo from that package.

The second sentence above may even be dropped. Up to you.

Could you send an updated patch?

Regards,
-- 
Nicolas Goaziou