freedesktop modules: init by michaelpj · Pull Request #45058 · NixOS/nixpkgs

michaelpj · 2018-08-15T10:29:52Z

Motivation for this change

Follow-up to/supersedes #44866.

This adds some modules that handle various parts of the XDG specs. This allows us to move some of the logic out of system-path.nix, and have a more sensible way to decide whether to update MIME caches.

I then added an option to system-path to allow extra environment setup commands to be defined by other modules, so the MIME module can also have the setup code in it.

Points I'm unsure about

Should the XDG modules be enabled by default? Lots of things probably assume they are, but I could see the argument for not doing so. Currently all the DEs turn them on, so off by default would mean that a machine without a DE or X would also not have all the MIME stuff linked in, and so on.
I wasn't brave enough to try and have a module controlling the use of the base directory spec - this is pretty deeply woven into everything, so I'm not sure that gains us much. However, it would give us a central location to define the constants /share and /etc/xdg, which might have some value.
Is the extraSetup mechanism for system-path OK? It's a bit ugly that clients use $out to be the output of another derivation.

@jtojnar @vcunat

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)
Fits CONTRIBUTING.md.

Mic92 · 2018-08-15T10:46:52Z

it could be also enabled if a display manager is enabled. Afaik it also make sense in the context of window managers like awesome or i3.

jtojnar · 2018-08-15T10:43:53Z

nixos/modules/config/xdg/menus.nix

typo dekstop

jtojnar · 2018-08-15T10:47:11Z

nixos/modules/programs/environment.nix

Where did this go?

Whoops, left over from when I did try having a base-directories module.

jtojnar · 2018-08-15T10:52:58Z

nixos/modules/config/system-path.nix

Why this? The rest of the share subdirectories in pathsToLink would be pointless then. I think we want to avoid "/share" (principle of least privilege or something).

Hm, I think I was confused by the need for /etc/xdg but not /share - I think you're right that it will do the right thing because we typically include subdirectories of /share explicitly.

michaelpj · 2018-08-15T11:12:37Z

Hm, good point about non-DE environments. xserver.nix also enables them, perhaps that's enough and I should remove the ones in the DE modules? OTOH, not sure how that will play with Wayland.

jtojnar · 2018-08-15T11:19:41Z

For Wayland, we still use xserver module (#43992, #44497). There might be some refactoring in the future, but for now, I think it is a good place as any.

michaelpj · 2018-08-15T11:28:23Z

Okay, I've removed the settings in the DEs, relying on the xserver module to set the option.

jtojnar · 2018-08-15T12:56:29Z

nixos/modules/config/xdg/mime.nix

+    environment.extraSetup = ''
+      XDG_DATA_DIRS=$out/share ${pkgs.shared-mime-info}/bin/update-mime-database -V $out/share/mime > /dev/null
+
+      ${pkgs.desktop-file-utils}/bin/update-desktop-database $out/share/applications


This should be probably separate from mime module. After all, the applications dir is also used by desktop-entry-spec and menu-spec.

Yes, but I believe that update-desktop-database only performs actions relevant to MIME lookup, so I think it belongs here.

jtojnar · 2018-08-15T13:12:29Z

nixos/modules/config/xdg/menus.nix

+  config = mkIf config.xdg.menus.enable {
+    environment.pathsToLink = [ 
+      "/share/applications"
+      "/share/applications-merged"


It looks like it should be share/menus/applications-merged: https://specifications.freedesktop.org/menu-spec/menu-spec-latest.html#paths

Ah yes, good spot.

jtojnar · 2018-08-15T18:23:45Z

The original code checked for existence of the directory used for generating the caches, can we make sure that the generators work even with the directory possibly missing?

jtojnar · 2018-08-15T18:25:00Z

nixos/modules/config/xdg/autostart.nix

@@ -0,0 +1,21 @@
+{ config, lib, pkgs, ... }:


Unused pkgs arg

jtojnar · 2018-08-15T18:27:31Z

nixos/modules/config/xdg/autostart.nix

+      type = types.bool;
+      default = true;
+      description = ''
+        Whether to install files to support the XDG Autostart specification.


It would be nice to have a link to the specification <link xlink:href="https://specifications.freedesktop.org/autostart-spec/autostart-spec-latest.html">XDG Autostart specification</link>

jtojnar · 2018-08-15T18:29:18Z

nixos/modules/config/system-path.nix

+      extraSetup = mkOption {
+        type = types.lines;
+        default = [ ];
+        description = "Shell fragments to be run after the system environment has been created. This should only be used for things that need to modify the internals of the environment, e.g. generating MIME caches.";


Maybe mention something about the $out thing.

jtojnar · 2018-08-15T18:31:02Z

nixos/modules/config/system-path.nix

-              XDG_DATA_DIRS=$out/share $out/bin/update-mime-database -V $out/share/mime > /dev/null
-          fi
-
          if [ -x $out/bin/gtk-update-icon-cache -a -f $out/share/icons/hicolor/index.theme ]; then


This should probably be moved to xdg.icons

Hm, I wasn't sure about that - I thought maybe there should be a GTK module that does this?

Right, that looks like GTK thing: https://developer.gnome.org/gtk3/stable/gtk-update-icon-cache.html

We should probably get a module later.

Similarly for the glib schemas.

michaelpj · 2018-08-15T23:02:57Z

Comments addressed. I put back the test that the target directory be writable in the fragments. I also fixed a mistake with the menus and menus/applications-merged directories - those should have been under /etc/xdg.

jtojnar · 2018-08-15T23:09:15Z

nixos/modules/config/xdg/menus.nix

Still some unused pkgs args.

michaelpj · 2018-08-16T10:28:41Z

Hold off this for a bit while I do some more testing.

michaelpj · 2018-08-16T10:42:40Z

Okay, it turns out that linking /share is very important, and many applications rely on it. That seems potentially kind of problematic, but I've left it in for now. Many DEs already provided this setting, and it's likely that anything that didn't was quite broken.

At this point, a diff of my system-path built with and without this reveals only the previously missing XDG menu dirs.

michaelpj · 2018-08-16T10:42:54Z

(so I'm fairly confident in this now)

jtojnar · 2018-08-16T11:24:38Z

nixos/modules/config/xdg/mime.nix

This module might also add shared-mime-info to environment.systemPackages to be linked into environment to fix bugs like #39493

Aha, good catch. I had thought I could get away with removing them since the use of the binary is now encoded here, but it seems like it also has data to install.

jtojnar · 2018-08-16T11:36:26Z

Okay, it turns out that linking /share is very important, and many applications rely on it. That seems potentially kind of problematic, but I've left it in for now. Many DEs already provided this setting, and it's likely that anything that didn't was quite broken.

Those applications are broken then. I intend to remove /share from GNOME and replace it with specific directories like /share/gnome-shell/extensions and /share/gnome-shell/search-providers. Other DEs should do so as well.

jtojnar · 2018-08-16T11:41:41Z

Also, similar to icons, we might want to have sounds, as per sound-theme-spec (for /share/sounds). And appstream AppStream for /share/appdata and /share/metainfo.

michaelpj · 2018-08-16T13:16:11Z

Those applications are broken then. I intend to remove /share from GNOME and replace it with specific directories like /share/gnome-shell/extensions and /share/gnome-shell/search-providers.

I think that's a good idea, but I think we can do it as future work.

Also, similar to icons, we might want to have sounds, as per sound-theme-spec (for /share/sounds). And appstream AppStream for /share/appdata and /share/metainfo.

Sure, but let's do follow-ups. As an aside, it looks like we install a bunch of stuff in /share/sounds that doesn't follow the spec.

jtojnar · 2018-08-16T13:22:27Z

I think that's a good idea, but I think we can do it as future work.

Yeah, but I would leave it localized in the DE modules that need it, not move it up to xserver.

Sure, but let's do follow-ups. As an aside, it looks like we install a bunch of stuff in /share/sounds that doesn't follow the spec.

Sounds good.

By the way, do we need to write docs for manual or release notes?

michaelpj · 2018-08-16T13:31:13Z

I don't think we should leave the linking of /share to individual DEs. There are a lot of applications that have data that doesn't get linked without it, and I suspect things will just be subtly broken if you don't have it.

I don't think we need a doc change, this should only make things work better. However, I forgot to ask: should we consider having the default for some of these modules be not enabled? That would mean that systems without X could get away with e.g. not having icons, which might shrink closures a bit. OTOH, then we would definitely need a release note, since some things might rely on it happening implicitly.

michaelpj · 2018-08-16T13:34:55Z

For example, I noticed this because /share/sddm wasn't linked, causing it to not work properly. That means that if we take it out, anyone using sddm without a DE will be broken. Of course, the correct fix is for the sddm module to link it, but I don't want to do all those fixes here.

michaelpj · 2018-08-16T13:36:50Z

This is the diff between the profiles - as you can see, a lot of stuff goes missing.

diff --brief -r /nix/store/9mj6n7ggca7r6d82sdjvjpd4rl8c01m3-system-path /nix/store/1rfgr9cwxh7n535fqv8ldcmahpbj7v0j-system-path | grep "Only in" > missing.txt

missing.txt

jtojnar · 2018-08-16T13:55:01Z

I don't think we should leave the linking of /share to individual DEs. There are a lot of applications that have data that doesn't get linked without it, and I suspect things will just be subtly broken if you don't have it.

Moving it up in the chain would make the future granularization harder as more bugs would be obscured until then. I think status quo is good enough for now.

should we consider having the default for some of these modules be not enabled? That would mean that systems without X could get away with e.g. not having icons, which might shrink closures a bit. OTOH, then we would definitely need a release note, since some things might rely on it happening implicitly.

Hmm,

$ nix path-info -f . -S shared-mime-info
/nix/store/g89z1wlss6dnhcvajxmn36nbayql80rh-shared-mime-info-1.10	   48303016
$ nix path-info -f . -S desktop-file-utils
/nix/store/i6iglvwcq294nyam49h0q04aqzjzqf2c-desktop-file-utils-0.23	   39642584

But looks like it is mostly just glibc, libxml2 and glib. Perhaps @vcunat can address this.

For example, I noticed this because /share/sddm wasn't linked, causing it to not work properly. That means that if we take it out, anyone using sddm without a DE will be broken. Of course, the correct fix is for the sddm module to link it, but I don't want to do all those fixes here.

I would leave this as is and address this in further PR.

This is the diff between the profiles - as you can see, a lot of stuff goes missing.
diff --brief -r /nix/store/9mj6n7ggca7r6d82sdjvjpd4rl8c01m3-system-path /nix/store/1rfgr9cwxh7n535fqv8ldcmahpbj7v0j-system-path | grep "Only in" > missing.txt
missing.txt

In my experience, most apps compile in the path. Only minority looks up their own data in $XDG_DATA_DIRS. I can recall quodlibet looking for translations in there because the Python package installation method they use does not support absolute installation paths.

michaelpj · 2018-08-16T19:43:00Z

Hm, I was thinking that we would save on closures by not linking /share/icons, but while this would shrink the size of the system environment tree it wouldn't save on the closure unless the icons were in a separate output.

michaelpj · 2018-08-16T20:24:50Z

I've returned the linking of /share to the DEs that had it before, with a note that they shouldn't do that.

michaelpj · 2018-08-16T20:25:21Z

In case it wasn't clear from my comment before: since there's unlikely to be a closure size improvement, I think we should leave these options enabled by default.

jtojnar

Great job. Looks good to me, though I would still like another set of eyes to check this.

michaelpj · 2018-08-18T17:14:35Z

@vcunat could you have a look at the final state of this?

joachifm · 2018-08-18T18:30:54Z

nixos/modules/config/system-path.nix


+      extraSetup = mkOption {
+        type = types.lines;
+        default = [ ];


Nit: I think using an empty string as the default value is strictly more correct.

Fixed, thanks.

michaelpj · 2018-08-21T20:45:03Z

ping

jtojnar · 2018-08-21T21:45:55Z

It is still in some reviewers’ queues. Let’s leave it one more week.

michaelpj · 2018-08-22T15:29:21Z

Wasn't intending to be pushy, just thought we'd got pretty close :)

samueldr · 2018-08-22T17:34:37Z

Don't hang the reviews and merging on this issue, but I'm wondering why the *.enable options have default = true, but are then enabled in xserver.nix, what would break defaulting them to false? (I'm not sure any of the previous conversations are relevant here.)

michaelpj · 2018-08-22T17:37:42Z

Enabling them in xserver.nix is mainly just to signal intent, and so we could turn them off if we wanted. As for what would break if we set them to false... I'm not sure, and I'm not brave enough to try it. Especially since the benefits seems slim. Maybe a follow-up PR can try that.

michaelpj · 2018-08-30T14:23:19Z

Ping - I think this would be a reasonable inclusion for 18.09.

Mic92 requested a review from jtojnar August 15, 2018 10:45

jtojnar reviewed Aug 15, 2018

View reviewed changes

michaelpj force-pushed the imp/freedesktop-modules branch from 2a69ba0 to d00f085 Compare August 15, 2018 11:11

michaelpj force-pushed the imp/freedesktop-modules branch from d00f085 to 1b3b81f Compare August 15, 2018 11:14

michaelpj force-pushed the imp/freedesktop-modules branch from 1b3b81f to 680bfb5 Compare August 15, 2018 11:27

jtojnar reviewed Aug 15, 2018

View reviewed changes

michaelpj force-pushed the imp/freedesktop-modules branch 2 times, most recently from bde1594 to e551e1b Compare August 15, 2018 23:02

jtojnar reviewed Aug 15, 2018

View reviewed changes

jtojnar reviewed Aug 16, 2018

View reviewed changes

michaelpj added 2 commits August 16, 2018 21:23

xdg: add modules for supporting various XDG specs

13e2e19

system-path: allow other modules to provide setup fragments

1b11fdd

michaelpj force-pushed the imp/freedesktop-modules branch from ca0476f to 1b11fdd Compare August 16, 2018 20:24

jtojnar approved these changes Aug 16, 2018

View reviewed changes

joachifm reviewed Aug 18, 2018

View reviewed changes

system-path: fix default option value

854ebed

jtojnar added this to the 18.09 milestone Aug 21, 2018

jtojnar added the 9.needs: community feedback This needs feedback from more community members. label Aug 21, 2018

samueldr approved these changes Aug 22, 2018

View reviewed changes

jtojnar merged commit 8a8056c into NixOS:master Aug 30, 2018

jtojnar mentioned this pull request Sep 26, 2018

DEs probably shouldn't link /share #47173

Open

jtojnar mentioned this pull request Oct 4, 2018

build GTK icon caches for some packages #47690

Merged

9 tasks

romildo mentioned this pull request Oct 9, 2018

gtk/icons: (re)build icon theme caches #48116

Merged

9 tasks

jtojnar mentioned this pull request Mar 21, 2019

Should X really be coupled so tightly with xdg? #57999

Closed

Uh oh!

Conversation

michaelpj commented Aug 15, 2018

Motivation for this change

Things done

Uh oh!

Mic92 commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelpj commented Aug 15, 2018

Uh oh!

jtojnar commented Aug 15, 2018

Uh oh!

michaelpj commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtojnar commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelpj commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelpj commented Aug 16, 2018

Uh oh!

michaelpj commented Aug 16, 2018

Uh oh!

michaelpj commented Aug 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtojnar commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtojnar commented Aug 16, 2018

Uh oh!

michaelpj commented Aug 16, 2018

Uh oh!

jtojnar commented Aug 16, 2018

Uh oh!

michaelpj commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Mic92 commented Aug 15, 2018 •

edited

Loading

jtojnar commented Aug 16, 2018 •

edited

Loading

michaelpj commented Aug 16, 2018 •

edited

Loading

michaelpj Aug 20, 2018 •

edited

Loading

samueldr commented Aug 22, 2018 •

edited

Loading