[bug#52189] gnu: Add notcurses

---
 gnu/packages/notcurses.scm | 71 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 gnu/packages/notcurses.scm

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,

Hi Nicolas,

Thanks for the review.

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

When I asked about where it should go in the IRC, specifically inquiring
if it should be placed with ncurses, <notmaximed> said that it shouldn't
go with Ncurses and it should be placed in its own file [1]. Are we now
sure that the opposite is the case? noted re: `gnu/local.mk` for the future. 

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

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

Noted.

> The build-type value above is suspicious.

It is recommended to set this value in `INSTALL.md`. What about it is suspicious?

> This is not cross-compilation friendly.

Noted. I'll change it and try running it on other architectures using
QEMU, I had tried without success before; hopefully that will get it
building faithfully across platforms :)

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

The configure flags are set to build a slimmed down version of
Notcurses; it shaves about 80mb off. I had hoped to make different
outputs with different options, but when I asked about it in IRC I
didn't get a response, and couldn't find any package that is
configurable based on outputs to reference.

I just checked and the phase configuration can go entirely actually, I
just checked. But the build will fail without the configure flags set.

But alas, I just found out the 3.0 release was yesterday, which is
said to be a big improvement on many levels, so it seems like I should
just go ahead and build that one now with your suggestions and introduce
the package from this version. This package has been driving me crazy
tbh, because it updates nearly everytime I prepare to send it up
stream. but I was under the impression the 3.0 wouldn't be ready until
maybe 2022.   

[1] https://logs.guix.gnu.org/guix/2021-10-24.log#201806

Thanks for the feedback and let me know about the above questions and
I'll send the new patch accordingly.

Best,
Blake

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,

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,

Hello,

Blake Shaw <blake@nonconstructivism.com> writes:

> When I asked about where it should go in the IRC, specifically inquiring
> if it should be placed with ncurses, <notmaximed> said that it shouldn't
> go with Ncurses and it should be placed in its own file [1]. Are we now
> sure that the opposite is the case?

No, I'm not sure. I guess <notmaximed> is right, then.

> It is recommended to set this value in `INSTALL.md`. What about it is
> suspicious?

"-DVAR=VAL" is neither common nor self-explaining. It looks like some
copy-pasta. I suggest to add a comment explaining it is per INSTALL.md.

Regards,

On 2021-11-30, Blake Shaw wrote:
> +(define-public notcurses
> +  (let ((commit "d15eb6003cbd65f11163916261cf6cd5600c77fa"))
> +    (package
> +      (name "notcurses")
> +      (version "2.4.9")

notcurses was added in commit 7022eb6ea0f3be2f0eb58617c607ce34dfbff90a,
and is currently updated to 3.0.9.

Marking as done.

live well,
  vagrant