Re: [gdm-list] A review of patches in Debian packaging

[Brian Cameron <brian cameron oracle com>]

Josselin:

> currently distributions need a large number of patches to get GDM to
> work. For example OpenSUSE includes 19 patches, Debian 24 and Ubuntu 29.
> This amount means we almost work on forked versions and this means more
> work for everyone.

Agreed.  It would be good to work together to get needed fixes upstream.

> Vincent (who maintains GDM in Suse) and Sébastien (ditto in Ubuntu)
> suggested we started with a review of patches on gdm-list before
> discussing them individually on Bugzilla.
>
> So here is the list for Debian. I am aware that many of them should have
> been forwarded earlier, but it’s better late than never.
>
> You can see the patches at
> http://patch-tracker.debian.org/package/gdm3/2.30.2-4
>
> 01_minimal_uid.patch
>          Debian-specific. Non-system UIDs start at 1000 for us.
>          What would be nice would be a way to specify this information as
>          a configure option.

The old GDM allowed sysadmins or distros to configure this in the GDM
daemon configuration.  Perhaps it makes sense to add this configuration
option back so distros can set it as needed in their default
configuration file.

> 02_xnest-wrapper.patch
>          Instead of hardcoding the Xnest/Xephyr choice at compile time,
>          we replace it with a script in /usr/share/gdm that checks for
>          their presence at run time.
>          Anyway this is useless currently since nested logins are still
>          not available.

Seems reasonable, but not sure how much value it has until the new GDM
supports Xnest/Xephyr style sessions.

> 05_stop_welcome_session.patch
>          Since the gnome-session and dbus-daemon processes for the login
>          manager are killed before starting the user session,
>          --exit-with-session is not appropriate. On the contrary it will
>          start a watcher process that remains after the user has logged
>          on.

+1 to get this patch upstream.  I'd recommend filing a bug in
bugzilla.gnome.org in the "gdm" category.

> 06_first_vt.patch
>          This patch reintroduces the TTY manager from GDM 2.20 instead of
>          letting X decide on which VT to run. Letting X decide makes it
>          start on tty1 and crash as soon as getty does the same. There
>          was broad consensus among Debian developers that neither GDM nor
>          graphical boot thingies should not interfere with tty1 to tty6.

Ray and Jon are more familiar with how the new GDM works with display
switching, so I'll defer to their thoughts about this.

> 07_libexec-paths.patch
>          Various things in the build system suppose that $libexecdir is
>          the same for GDM and other packages: ConsoleKit, g-s-d,
>          PolicyKit. This excludes FHS-compliant systems, which don’t have
>          a /usr/libexec. Currently we hardcode the paths instead, but
>          this should really be detected with pkg-config.

Yes, this should be detected with pkg-config.  Using configure options
should also be acceptable if the information about an install location
is not available via pkg-config.  I'd think that if this were made
appropriately configurable, it should go upstream.

> 08_frequent-users_greeter.patch
>          To be sure the greeter doesn’t show users that shouldn’t be here
>          for one reason or another, this patch sets the session type to
>          "gdm" for user sessions, and requests to CK only sessions
>          corresponding to this type.

I'm not clear about what this patch does, or what problem it is trying
to solve.  Is there a bug report that we could review, or any additional
information you could provide?

> 09_default_session.patch
>          Use a default session set through alternatives instead of
>          hardcoding gnome.session. See GNOME #594733

The bug report highlights that we really should have a configuration
option for this.  The latest patch for this report doesn't seem to
address these comments.

> 11_xephyr_nested.patch
>          This is a first attempt by Luca Bruno at re-introducing the
>          nested login functionality, but xauth is still not right so the
>          patch is disabled.

Would be cool to get Xnest/Xephyr support back into GDM, so I hope this
continues to be worked on.

> 12_polkit_settings.patch
>          Based on the Ubuntu patch with a handful of fixes. Allows to set
>          configuration options from the user’s session, using PolicyKit.

Again, I think Ray or Jon would be the best to review this.

> 13_gdmsetup_desktop.patch
> 13_gdmsetup_ui.patch
> 13_gdmsetup.patch
>          The configuration tool. Taken from Ubuntu with lots of cleanups.

Bug #587750 was the bug about adding back a gdmsetup like program.  Jon
McCann closed this as WONTFIX without really providing much information
why or how we plan to move forward.  He only said "I think migrating
our settings to dconf and adding some settings controls to the account
dialog is the way forward."

So, not sure what to do here.  If your improvements to gdmsetup are
significantly better than the patches already in bug #587750, then
it might be worth attaching the updated patches and re-opening the
bug to spur some more discussion.

> 16_xserver_path.patch
>          Specify the X server location through the environment. See GNOME
>          #588848

This seems reasonable to go upstream, once the discussion in the bug
resolves.

> 18_switch_kill_greeter.patch
> 20_endsession_respawn.patch
>          As explained in GNOME #618047, user switching leaves a X server
>          behind, which is not acceptable. It also has inconsistencies in
>          terms of interface. All of it is built on the premise that X
>          switches VTs by itself when dying, which is not right when
>          launched from a display manager. Luckily there is a X server
>          flag to disable this behavior (-novtswitch) so we have reworked
>          user experience around it.

I think Jon or Ray is the best person to look over this.

> 19_configure_xserver.patch
>          Allows to set X server arguments in the configuration file. See
>          GNOME #586777.

A nice feature, though as the bug report says we need to figure out what
is going on with MultiSeat.  The MultiSeat code already provides this
sort of configuration, so if MultiSeat goes upstream this patch is not
needed.

> 21_schemas_usr.patch
>          Put gdm.schemas in /usr, it has nothing to do in /etc. The patch
>          is probably missing the corresponding documentation update.

If there is a good reason to move the schemas to /usr, then I'd file a
bug report and explain the reasoning.

> 22_noconsole.patch
>          Allows to run GDM without a local X server. This was reported as
>          GNOME #567522 which was closed but not entirely fixed. Now it
>          accepts to run if the X server fails to start, but this patch is
>          more complete and allows to configure it in headless mode.

I'd file a new bug or reopen 567522 with the latest patch.

> 23_autologin_once.patch
>          Allow to log in someone after exiting from an autologin session.
>          See GNOME #587606

The bug seems to be blocking because it would be better if the behavior
were configurable.  Some users want autologin to work all the time,
while others want autologin to work on bootup and then switch to timed
login.

> 24_show_user_other.patch
>          On NIS/LDAP systems with no local user, no user shows in the
>          chooser, and in this case the “other…” choice disappears. I
>          can’t think of a reason why this would be useful, apart
>          preventing people from logging in. The patch makes the “other…”
>          choice always displayed.

This looks like a fix that should go upstream.  I'd file a bug.

Brian