[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#52189] [PATCH] gnu: Add notcurses
From: |
Blake Shaw |
Subject: |
[bug#52189] [PATCH] gnu: Add notcurses |
Date: |
Fri, 03 Dec 2021 05:20:01 +0700 |
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”