| Message ID | 20220227183415.12487-1-kevinboulain@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#53257] gnu: foot: Wrap the program to expose TERMINFO_DIRS | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
I hope you don't mind, I went ahead and used wrap-program as discussed (I was encountering this issue and had a very similar patch as the OP's). Did I get the idea that was discussed in this thread right? If yes, should I send another patch to fix the other terminals (e.g.: https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/terminals.scm?id=85a5110de79f4fe9fd822ede3915654ee699d6c5#n220)? On Sun, 27 Feb 2022 at 19:32, Kevin Boulain <kevinboulain@gmail.com> wrote: > > As discussed in https://issues.guix.gnu.org/53257, it appears the > preferred way to expose TERMINFO_DIRS to programs running under a > terminal is to wrap the terminal with a script that sets TERMINFO_DIRS > before exec'ing it. > This is less invasive than unconditionally setting TERMINFO_DIRS in the > profile (via native-search-paths) as this particular environment > variable is only necessary when running the terminal itself. > > * gnu/packages/terminals.scm (foot): Export TERMINFO_DIRS. > > Tested: > ./pre-inst-env guix install foot > cat "$(which foot)" > #!/gnu/store/4y5m9lb8k3qkb1y9m02sw9w9a6hacd16-bash-minimal-5.1.8/bin/bash > export TERMINFO_DIRS="/gnu/store/6dwhps0cgzk0z7c3q2q596l52ibzdl5c-foot-1.11.0/share/terminfo${TERMINFO_DIRS:+:}$TERMINFO_DIRS" > exec -a "$0" "/gnu/store/6dwhps0cgzk0z7c3q2q596l52ibzdl5c-foot-1.11.0/bin/.foot-real" "$@" > --- > gnu/packages/terminals.scm | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/gnu/packages/terminals.scm b/gnu/packages/terminals.scm > index 06fa341472..65553ac295 100644 > --- a/gnu/packages/terminals.scm > +++ b/gnu/packages/terminals.scm > @@ -852,7 +852,19 @@ (define-public foot > ;; also to address a GCC 10 issue when doing PGO builds. > #:build-type "release" > ;; Enable LTO as recommended by INSTALL.md. > - #:configure-flags '("-Db_lto=true"))) > + #:configure-flags '("-Db_lto=true") > + ;; Ensure the terminfo database is available to programs spawned under > + ;; the terminal. > + #:phases > + (modify-phases %standard-phases > + (add-after 'install 'wrap-program > + (lambda* (#:key inputs outputs #:allow-other-keys) > + (let* ((out (assoc-ref outputs "out"))) > + ;; footclient executes programs under the server process, > + ;; there is no need to wrap it too. > + (wrap-program (string-append out "/bin/foot") > + `("TERMINFO_DIRS" ":" prefix > + (,(string-append out "/share/terminfo")))))))))) > (native-inputs > (list ncurses ;for 'tic' > pkg-config scdoc wayland-protocols)) > -- > 2.34.0 >
Kevin Boulain schreef op zo 27-02-2022 om 19:34 [+0100]: > + (modify-phases %standard-phases > + (add-after 'install 'wrap-program > + (lambda* (#:key inputs outputs #:allow-other-keys) > + (let* ((out (assoc-ref outputs "out"))) > + ;; footclient executes programs under the server process, > + ;; there is no need to wrap it too. > + (wrap-program (string-append out "/bin/foot") > + `("TERMINFO_DIRS" ":" prefix > + (,(string-append out "/share/terminfo")))))))))) You'll need to add 'bash-minimal' to 'inputs', such that this works even when cross-compiling. I think "./pre-inst-env guix lint foot" would warn about this. Also, this can be simplified a bit, to ,#(modify-phases %standard-phases (add-after 'install 'wrap-program (lambda _ (let ((out #$output)) ;; footclient executes programs under the server process, ;; there is no need to wrap it too. (wrap-program (string-append out "/bin/foot") `("TERMINFO_DIRS" ":" prefix (,(string-append out "/share/terminfo")))))))))) or, reducing the whitespace: ,#(modify-phases %standard-phases (add-after 'install 'wrap-program (lambda _ (define out #$output) ;; footclient executes programs under the server process, ;; there is no need to wrap it too. (wrap-program (string-append out "/bin/foot") `("TERMINFO_DIRS" ":" prefix (,(string-append out "/share/terminfo"))))))))) (you'll need to add (guix gexp) to the list of imports to do this) YMMV on whether this is an improvement. I hope you don't mind, I went ahead and used wrap-program as discussed (I was encountering this issue and had a very similar patch as the OP's). Did I get the idea that was discussed in this thread right? Yes, this was the idea, though to be 100% sure it would need to be tested (by running "guix shell --pure foot -- foot" and then running ~/.guix-profile/bin/SOME-NCURSES-APP in the terminal, or something like that. If yes, should I send another patch to fix the other terminals (e.g.: https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/terminals.scm?id=85a5110de79f4fe9fd822ede3915654ee699d6c5#n220 )? That would be nice, but keep in mind that this might not be needed for every terminal app -- some apps set TERMINFODIR automatically (I forgot which) and hence don't need any wrapping. Greetings, Maxime.
Thank you, I'll send a follow-up patch to resolve the concerns (that's greatly appreciated, I'm new at both Guix and Scheme). I did test it with an ncurses program so I can confirm it works (no complaints about an unknown terminal anymore). I only have a single concern about this approach: native-search-paths made the terminfo database somewhat 'global' (as long the shell sourced the Guix profile) and it made stuff like SSH less awkward: by installing foot on both the client and the server, ncurses applications could 'just work'. Now, by making the terminfo database only visible inside the terminal itself, SSH is pretty much never guaranteed to work seamlessly (however, foot appears to be compatible with xterm so it's not that much of an issue: https://codeberg.org/dnkl/foot/src/branch/master/INSTALL.md#terminfo). I agree this has always been a pain point with ncurses programs and the proper solution would be to upstream the terminal's description to terminfo so it's easily available everywhere (or people can always install it manually), but I prefer following the recommendations of the maintainers here :) On Sun, 27 Feb 2022 at 20:22, Maxime Devos <maximedevos@telenet.be> wrote: > > Kevin Boulain schreef op zo 27-02-2022 om 19:34 [+0100]: > > + (modify-phases %standard-phases > > + (add-after 'install 'wrap-program > > + (lambda* (#:key inputs outputs #:allow-other-keys) > > + (let* ((out (assoc-ref outputs "out"))) > > + ;; footclient executes programs under the server process, > > + ;; there is no need to wrap it too. > > + (wrap-program (string-append out "/bin/foot") > > + `("TERMINFO_DIRS" ":" prefix > > + (,(string-append out "/share/terminfo")))))))))) > > > You'll need to add 'bash-minimal' to 'inputs', such that this works > even when cross-compiling. I think "./pre-inst-env guix lint foot" > would warn about this. > > Also, this can be simplified a bit, to > > ,#(modify-phases %standard-phases > (add-after 'install 'wrap-program > (lambda _ > (let ((out #$output)) > ;; footclient executes programs under the server process, > ;; there is no need to wrap it too. > (wrap-program (string-append out "/bin/foot") > `("TERMINFO_DIRS" ":" prefix > (,(string-append out "/share/terminfo")))))))))) > > or, reducing the whitespace: > > > ,#(modify-phases %standard-phases > (add-after 'install 'wrap-program > (lambda _ > (define out #$output) > ;; footclient executes programs under the server process, > ;; there is no need to wrap it too. > (wrap-program (string-append out "/bin/foot") > `("TERMINFO_DIRS" ":" prefix > (,(string-append out "/share/terminfo"))))))))) > > (you'll need to add (guix gexp) to the list of imports to do this) > > YMMV on whether this is an improvement. > > I hope you don't mind, I went ahead and used wrap-program as > discussed (I was encountering this issue and had a very similar patch > as the OP's). Did I get the idea that was discussed in this thread > right? > > Yes, this was the idea, though to be 100% sure it would need to be tested (by running "guix shell --pure foot -- foot" and then running > ~/.guix-profile/bin/SOME-NCURSES-APP in the terminal, or something like that. > > If yes, should I send another patch to fix the other terminals > (e.g.: > > https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/terminals.scm?id=85a5110de79f4fe9fd822ede3915654ee699d6c5#n220 > )? > > That would be nice, but keep in mind that this might not be needed > for every terminal app -- some apps set TERMINFODIR automatically (I forgot which) and hence don't need any wrapping. > > Greetings, > Maxime.
> You'll need to add 'bash-minimal' to 'inputs', such that this works > even when cross-compiling. I think "./pre-inst-env guix lint foot" > would warn about this. It does warn about it, I only ran './pre-inst-env guix style foot' (but I limited the reformatting to my changes), my bad. > Also, this can be simplified a bit, to [...] Yup, thanks! > Yes, this was the idea, though to be 100% sure it would need to be tested (by running "guix shell --pure foot -- foot" and then running > ~/.guix-profile/bin/SOME-NCURSES-APP in the terminal, or something like that. I confirm it works (for example tput doesn't complain anymore and I can start emacs -nw).
diff --git a/gnu/packages/terminals.scm b/gnu/packages/terminals.scm index 06fa341472..65553ac295 100644 --- a/gnu/packages/terminals.scm +++ b/gnu/packages/terminals.scm @@ -852,7 +852,19 @@ (define-public foot ;; also to address a GCC 10 issue when doing PGO builds. #:build-type "release" ;; Enable LTO as recommended by INSTALL.md. - #:configure-flags '("-Db_lto=true"))) + #:configure-flags '("-Db_lto=true") + ;; Ensure the terminfo database is available to programs spawned under + ;; the terminal. + #:phases + (modify-phases %standard-phases + (add-after 'install 'wrap-program + (lambda* (#:key inputs outputs #:allow-other-keys) + (let* ((out (assoc-ref outputs "out"))) + ;; footclient executes programs under the server process, + ;; there is no need to wrap it too. + (wrap-program (string-append out "/bin/foot") + `("TERMINFO_DIRS" ":" prefix + (,(string-append out "/share/terminfo")))))))))) (native-inputs (list ncurses ;for 'tic' pkg-config scdoc wayland-protocols))
As discussed in https://issues.guix.gnu.org/53257, it appears the preferred way to expose TERMINFO_DIRS to programs running under a terminal is to wrap the terminal with a script that sets TERMINFO_DIRS before exec'ing it. This is less invasive than unconditionally setting TERMINFO_DIRS in the profile (via native-search-paths) as this particular environment variable is only necessary when running the terminal itself. * gnu/packages/terminals.scm (foot): Export TERMINFO_DIRS. Tested: ./pre-inst-env guix install foot cat "$(which foot)" #!/gnu/store/4y5m9lb8k3qkb1y9m02sw9w9a6hacd16-bash-minimal-5.1.8/bin/bash export TERMINFO_DIRS="/gnu/store/6dwhps0cgzk0z7c3q2q596l52ibzdl5c-foot-1.11.0/share/terminfo${TERMINFO_DIRS:+:}$TERMINFO_DIRS" exec -a "$0" "/gnu/store/6dwhps0cgzk0z7c3q2q596l52ibzdl5c-foot-1.11.0/bin/.foot-real" "$@" --- gnu/packages/terminals.scm | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)