diff mbox series

[bug#78308,9/9] services: Add vte-integration-service-type to %desktop-services.

Message ID cad4115ef18b9c48219a8e8c3e62b2c1d284afd6.1746682207.git.maxim.cournoyer@gmail.com
State New
Headers
Series VTE integration support / Shell startup files refactor |

Commit Message

Maxim Cournoyer May 8, 2025, 6:02 a.m. UTC 
  * gnu/services/desktop.scm (desktop-services-for-system):

Fixes: <https://issues.guix.gnu.org/72172>
Change-Id: Ib29468468e327801a4e95361610159de61f7e8d6
---
 gnu/services/desktop.scm | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Rutherther May 12, 2025, 8:52 a.m. UTC | #1 
Hi Maxim,

thank you for this. I think it's important for users to be able to
extend their system profile and bashrc files, currently it was hard to
do. Basically the only way was to modify etc-service-type in
essential-services, removing the profile and bashrc completely, and
adding your own contents.

I have a few comments that I am posting before I actually tested it just
based on the changes:

I think that the docs for etc-profile-d-service-type could be more
specific about the files accepted - that only .sh files are going to
have some effect. Additionally the example plain-file is wrong,
plain-file accepts two arguments, name and content. Here the name is
quite important as it has to have the .sh extension for the file to be sourced.

As for the bash modification, is is a good idea to go through grafts?
Aren't grafts mainly meant for security fixes as to no changes are
introduced? How can we make sure this change is not going to break any
of the packages using bash? I understand we cannot rebuild the whole
world via this patch series if it is to be applied to master, but this
doesn't feel right, maybe those changes to bash should go on a team branch?

It's not super important, but do you happen to know why bash-completion
package provides profile.d directory for bash_completion.sh that should
actually be bashrc? Maybe it should be in etc/bashrc.d output dir
instead - that the package should be adapted?

I don't think removing the PS1 from guix home bashrc is appropriate. I think
the PS1 is mostly for foreign distros, not for Guix System, it's even
mentioned in the comment that it's for distros that don't provide a good PS1.
Similar question goes to removing sourcing of /etc/bashrc. Was this
because of Guix System or because of foreign distros? I don't know here.

Note that users not using %base-services are going to get no bash
completions after this update. Maybe there should be news entry about
the bashrc-d and profile-d services?

Regards
Rutherther
Maxim Cournoyer May 12, 2025, 1:12 p.m. UTC | #2 
Hi,

Rutherther <rutherther@ditigal.xyz> writes:

> Hi Maxim,
>
> thank you for this. I think it's important for users to be able to
> extend their system profile and bashrc files, currently it was hard to
> do. Basically the only way was to modify etc-service-type in
> essential-services, removing the profile and bashrc completely, and
> adding your own contents.

Glad to know that'll be useful!

> I have a few comments that I am posting before I actually tested it just
> based on the changes:
>
> I think that the docs for etc-profile-d-service-type could be more
> specific about the files accepted - that only .sh files are going to
> have some effect.  Additionally the example plain-file is wrong,
> plain-file accepts two arguments, name and content. Here the name is
> quite important as it has to have the .sh extension for the file to be sourced.

Thanks for spotting this.  I'll fix it, and emphasize more that only
files of the @file{.sh} extension are considered.

> As for the bash modification, is is a good idea to go through grafts?
> Aren't grafts mainly meant for security fixes as to no changes are
> introduced? How can we make sure this change is not going to break any
> of the packages using bash? I understand we cannot rebuild the whole
> world via this patch series if it is to be applied to master, but this
> doesn't feel right, maybe those changes to bash should go on a team branch?

A full world rebuild seems very expensive for this change alone.  The
only thing this switch does is cause Bash to source /etc/bashrc when it
also sources also ~/.bashrc.  Since there is no /etc/bashrc in the build
container, we shoud be good in terms of not breaking anything build wise.

> It's not super important, but do you happen to know why bash-completion
> package provides profile.d directory for bash_completion.sh that should
> actually be bashrc? Maybe it should be in etc/bashrc.d output dir
> instead - that the package should be adapted?

That seems to come from Fedora, and maybe other distributions, that have
this in their /etc/bashrc file:

--8<---------------cut here---------------start------------->8---
    # Only display echos from profile.d scripts if we are no login shell
    # and interactive - otherwise just process them to set envvars
    for i in /etc/profile.d/*.sh; do
        if [ -r "$i" ]; then
            if [ "$PS1" ]; then
                . "$i"
            else
                . "$i" >/dev/null
            fi
        fi
    done

    unset i
--8<---------------cut here---------------end--------------->8---

So on these systems, /etc/profile.d/ is used to extend Bash too.  That
means these scripts are limited to be POSIX-compliant, as /etc/profile
also source these, which seems an odd requirement to me for configuring
Bash :-).  In practice, what vte.sh does for example is check if the
Shell is Bash or Zsh, and return early if it isn't.  Not as clean as
having a /etc/bashrc.d, I'd say.  Let's start some new trend ;-).

> I don't think removing the PS1 from guix home bashrc is appropriate. I think
> the PS1 is mostly for foreign distros, not for Guix System, it's even
> mentioned in the comment that it's for distros that don't provide a good PS1.

It's not removed, but moved, as the change log says for the "system:
Factorize bashrc default configuration." commit:

--8<---------------cut here---------------start------------->8---
* gnu/home/services/shells.scm (add-bash-configuration): Do not set PS1, now
part of %default-bashrc.
--8<---------------cut here---------------end--------------->8---

Guix home uses the %default-bashrc, so it's covered.  It's something
I've tried paying attention to, it's good you saw that too.

> Similar question goes to removing sourcing of /etc/bashrc. Was this
> because of Guix System or because of foreign distros? I don't know here.

Both, I think!  The home-shell-service-type uses the bash package from
Guix, so it's safe to assume /etc/bashrc will now be sourced anytime
~/.bashrc is.

> Note that users not using %base-services are going to get no bash
> completions after this update. Maybe there should be news entry about
> the bashrc-d and profile-d services?

Good idea too!  I'll write one and send in a v2.
diff mbox series

Patch

diff --git a/gnu/services/desktop.scm b/gnu/services/desktop.scm
index 2127c2d389c..ffa02063514 100644
--- a/gnu/services/desktop.scm
+++ b/gnu/services/desktop.scm
@@ -2458,6 +2458,10 @@  (define* (desktop-services-for-system #:optional
          ;; to avoid GDM stale cache and permission issues.
          gdm-file-system-service
 
+         ;; Provides a nicer terminal emulator experience for VTE-using
+         ;; terminal emulators such as GNOME Console, Xfce Terminal, etc.
+         (service vte-integration-service-type)
+
          ;; The global fontconfig cache directory can sometimes contain
          ;; stale entries, possibly referencing fonts that have been GC'd,
          ;; so mount it read-only.