Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/gnome3: add GNOME Flashback sessions option #53695

Merged
merged 1 commit into from
Jan 21, 2019

Conversation

chpatrick
Copy link
Contributor

Motivation for this change

This allows you to enable GNOME Flashback sessions with a custom window manager.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@chpatrick
Copy link
Contributor Author

I'm not totally sure about the organization. Maybe gnome-flashback-session should be in gnome-flashback's passthru?

@GrahamcOfBorg GrahamcOfBorg added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 9, 2019
@jtojnar jtojnar requested a review from hedning January 9, 2019 17:36
@chpatrick chpatrick force-pushed the gnome-flashback-session branch 2 times, most recently from e26a011 to 4842da7 Compare January 9, 2019 20:30
@chpatrick
Copy link
Contributor Author

Done, please take a look

export XDG_CURRENT_DESKTOP="GNOME-Flashback:GNOME"
fi

exec ${gnome3.gnome-session}/bin/gnome-session --session=gnome-flashback-${wmName} "$@"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add an option for extra arguments like the acceleration disablement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works for me without it and it's not a documented gnome-session flag so I think we should add it if someone needs it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chpatrick
Copy link
Contributor Author

Hmm on my desktop it doesn't work yet because /nix/store/gnome-panel-.../share/applications isn't in some PATH. Do you know where I should add it?

@jtojnar
Copy link
Member

jtojnar commented Jan 11, 2019

That should go to XDG_DATA_DIRS and that is what wrapGAppsHook with

  preFixup = ''
    gappsWrapperArgs+=(
      # Session file requirements (for finding desktop files)
      --prefix XDG_DATA_DIRS : "${gnome-panel}/share"
    )
  '';

would do. But that is pretty pointless use of wrapGAppsHook.

I suggest that you just add the XDG_DATA_DIRS for the required desktop files into the session script like in the initial gist, and move wrapGAppsHook to the gnome-flashback package for finding the GSettings schemas.

@chpatrick
Copy link
Contributor Author

OK, please take a look.

cat << EOF > $out/libexec/gnome-flashback-${wmName}
#!${bash}/bin/sh

if [ -z \$XDG_CURRENT_DESKTOP ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using lib.writeScript would allow us to avoid the need to escape this.

Copy link
Contributor Author

@chpatrick chpatrick Jan 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would we put $out into it with writeScript?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It produces output which can be copied or linked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know but we want to add $out/share into XDG_DATA_DIRS, but we can't use $out in a writeScript.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let
  sessionScript = lib.writeScript ...;
in stdenv.mkDerivation {
  buildCommand = ''
    …
    mkdir -p $out/libexec
    cp ${sessionScript} $out/libexec/gnome-flashback-${wmName}
    …
  '';
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I understand that. The question is how you would do the following in the writeScript given that $out is not available:

export XDG_DATA_DIRS=$out/share:...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, sorry, you are right. I was thinking about passing a placeholder but those do not really work with writeScript, I think.

@chpatrick
Copy link
Contributor Author

I got it working with writeScript and co. I also needed to make some .desktop Execs absolute for it to work properly. Please take a look.

Copy link
Contributor

@hedning hedning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

@jtojnar
Copy link
Member

jtojnar commented Jan 19, 2019

I tried running

{ pkgs, config, ... }: {
  services.xserver = {
    displayManager.gdm = {
      enable = true;
      debug = true;
    };
    desktopManager.gnome3 = {
      enable = true;
      debug = true;
      flashback.enableMetacity = true;
    };
  };
}

in a VM and found two issues:

  1. The Applications menu is empty. We probably need either

    environment.systemPackages = [ pkgs.gnome3.gnome-menus ];
    environment.pathsToLink = [ "/share/desktop-directories" ];
    

    or better yet

      preFixup = ''
        gappsWrapperArgs+=(
          --prefix XDG_DATA_DIRS : "${gnome-menus}/share"
        )
      '';
    

    in gnome-panel

  2. I cannot lock the screen:

    Jan 19 01:00:48 nixos .gnome-panel-wr[1180]: Could not ask screensaver to lock: GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name org.gnome.ScreenSaver was not provided by any .service files
    

    Packaging gnome-screensaver and adding it to dbus.packages will be needed to support this.

@jtojnar
Copy link
Member

jtojnar commented Jan 19, 2019

It also might be nice to be enable gnome-flashback without gnome-shell. Maybe we could move most of the gnome3.nix to gnome-common.nix and then have gnome3.nix and gnome-flashback only containing enable and their own specific flags in, along with imports = [ ./gnome-common.nix ];.

@jtojnar
Copy link
Member

jtojnar commented Jan 19, 2019

  preFixup = ''
    gappsWrapperArgs+=(
      --prefix XDG_DATA_DIRS : "${gnome-menus}/share:${gnome-flashback}/share"
      --prefix XDG_CONFIG_DIRS : "${gnome-menus}/etc/xdg:${gnome-flashback}/etc/xdg"
    )
  '';

seems to fix it.

@chpatrick
Copy link
Contributor Author

I'm working on packaging gnome-screensaver. While I do that, what do you think about opinionatedly replacing gnome-screensaver with xscreensaver? gnome-screensaver is deprecated and seems to need a lot of patches to get it to work properly (at least on Ubuntu). This shows how it's possible to do it.

@chpatrick
Copy link
Contributor Author

OK I got it working with gnome-screensaver and also fixed the panel. Please take a look.

@GrahamcOfBorg GrahamcOfBorg added the 8.has: package (new) This PR adds a new package label Jan 20, 2019
# Find out where the session service file goes
# The sad sed hack is recomended by section 27.10 of the automake manual.
-DBUS_SESSION_SERVICE_DIR=`pkg-config --variable session_bus_services_dir dbus-1 | sed -e 's,/usr/share,${datarootdir},g'`
+DBUS_SESSION_SERVICE_DIR=${datarootdir}/dbus-1/services
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use PKG_CONFIG_DBUS_1_SESSION_BUS_SERVICES_DIR = "${placeholder "out"}/share/dbus-1/services"; in the expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what that's for, I want to set the path directly instead of using the sed hack.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sed hack will not do anything on Nix, as we do not use /usr/share prefix. Instead, we can override the path returned by pkg-config --variable session_bus_services_dir dbus-1 using the aforementioned environment variable, avoiding the need to patch the configure script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pkgconfig + sed hack is introduced by the Ubuntu patch that adds the dbus service. As you said it doesn't work, so I just replace it with the correct path directly. I don't see why changing what pkg-config returns would help.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why pkg-config needs to be involved at all. This is the original patch:

https://git.launchpad.net/ubuntu/+source/gnome-screensaver/tree/debian/patches/05_dbus_service.patch?h=ubuntu/cosmic#n12

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the sed will not bother us with the environment variable set:

$ env PKG_CONFIG_DBUS_1_SESSION_BUS_SERVICES_DIR='${placeholder "out"}/share/dbus-1/services' pkg-config --variable session_bus_services_dir dbus-1 | sed -e 's,/usr/share,${datarootdir},g'
${placeholder "out"}/share/dbus-1/services

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, in that case isn't it best to patch in the correct solution you described? I've pushed that now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work too, though I would prefer to minimize the number of patches. It is messy enough as is, and there are much more patches than other distros have:

https://git.archlinux.org/svntogit/community.git/tree/trunk?h=packages/gnome-screensaver
https://salsa.debian.org/gnome-team/gnome-screensaver/tree/debian/master/debian/patches

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we might want to switch to Arch’s patchset, as it is much closer to our philosophy. With Ubuntu, it is hard to tell which patches are Ubuntu specific, only needed due to patches of some other component (24 looks like that).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

24 is also in the Arch patches: https://git.archlinux.org/svntogit/community.git/tree/trunk/use-screensaver-background.patch?h=packages/gnome-screensaver

The enabled Ubuntu patches all seem to be bugfixes not present in the other distros, I think it would be a good idea to have them. I think if we want to really get it right we should support xscreensaver since this package is old and deprecated, but let's do that in another PR.

@jtojnar
Copy link
Member

jtojnar commented Jan 21, 2019

Creating a DBus interface for xscreensaver could work, though maybe we would need to implement the other methods too.

@chpatrick
Copy link
Contributor Author

Let's just do what upstream does with gnome-screensaver for now and merge this if possible. We can make another PR to add xscreensaver support.

@jtojnar
Copy link
Member

jtojnar commented Jan 21, 2019

By the way, do we care about running Flashback without having GNOME Shell installed?

@chpatrick
Copy link
Contributor Author

By the way, do we care about running Flashback without having GNOME Shell installed?

I guess it could be useful, but I'd like to merge this first since it works now and separating the two would be quite a big change. If there's demand for it we can make another PR.

@jtojnar
Copy link
Member

jtojnar commented Jan 21, 2019

Works for me, thanks for your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants