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

[Blake Shaw via Guix-patches via <guix-patches@gnu.org>]

Hi, thanks for the feedback.

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

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

I was originally packaging it with ncurses, but when I asked on the IRC
I was advised by <notmaximed> to create a new package [1]. Let me know
which is preferred and I will change things accordingly. 

[1] https://logs.guix.gnu.org/guix/2021-10-24.log#201648
>
>> +(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.
>
noted.

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

The build type is recommend in INSTALL.md[2]. I tried building without
it, and it seems to work fine, in case you'd like me to remove it. 

[2] https://github.com/dankamongmen/notcurses/blob/master/INSTALL.md
>
>> +                  #: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=" ...))
Oh great, I did not know about this. I was trying to build for other targets
using QEMU and couldn't; this seems to be why. 
>
>> +                  #: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?

The configure flags bring down the package size by about 80mb, and
FSG_BUILD=ON is to insure only FSF approved programs are used in the
build. I wasn't sure about comment etiquette, but I  will add and return.

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

It seems it builds fine without either build phase, so I can remove them
in the future. I originally wrote this package back in October and it
had updated multiple times since then, so its difficult for me to recall the
purpose of these phases, although I do recall them solving some debugging issues early on.

>
>> +      (native-inputs
>> +       `(("ncurses" ,ncurses)
>> +         ("gcc-toolchain" ,gcc-10)
>
> Could you explain why gcc-10 must be used?

For some reason I had thought its important to pin a version for the sake of
reproducibility. If not, I will remove it.  

>
>> +         ("pkg-config" ,pkg-config)))
>> +      (inputs
>> +       `(("zlib" ,zlib)
>> +         ("ffmpeg" ,ffmpeg)
>> +         ("libunistring" ,libunistring)))
>
> Pleas order inputs alphabetically.
Got it
>
>> + (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.

Got it, I had just copied from their official release. 

>
> Could you send an updated patch?

I just found out that the newest version, which is a landmark version,
3.0, was released yesterday (I had thought it wasn't coming until
2022). so I'll write the new package with the above considerations,
after I hear back from you about the questions concerning putting it in
gnu/ncurses.scm.

Thanks!
Blake
>
> Regards,

-- 
“In girum imus nocte et consumimur igni”