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

Jump to bottom

Gnome and Pantheon: install nixos wallpapers #86163

Merged
merged 4 commits into from Jun 7, 2020
Merged

Gnome and Pantheon: install nixos wallpapers #86163

merged 4 commits into from Jun 7, 2020

Conversation

worldofpeace
Copy link
Contributor

Motivation for this change

Fix #86146.
We were hardcoding them into settings but not installing them.

This also changes nixos-artwork slightly 581122f. Namely, install to share/backgrounds, and provide an absolute path to the file.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: nixos 6.topic: pantheon The Pantheon desktop environment 8.has: module (update) labels Apr 28, 2020
jtojnar
jtojnar reviewed Apr 28, 2020
View reviewed changes
pkgs/data/misc/nixos-artwork/wallpapers.nix
mkdir -p $out/share/backgrounds/nixos
ln -s $src $out/share/backgrounds/nixos/${src.name}

# TODO: is this path still needed?
mkdir -p $out/share/artwork/gnome
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not see artwork used within path when searching g-c-c. Googling "/usr/share/artwork/gnome" shows no results, without gnome only few mentions but none of them look official. We might want to keep this for backwards compatibility, though.

@worldofpeace
Copy link
Contributor Author

I don't really understand the failing check here.

@worldofpeace worldofpeace added this to In progress in Pantheon via automation Apr 28, 2020
@worldofpeace worldofpeace added this to In Progress in GNOME Apr 28, 2020
@worldofpeace
Copy link
Contributor Author

worldofpeace commented Apr 28, 2020 •
edited

@ofborg eval

(I have no idea)

@jtojnar
Copy link
Contributor

jtojnar commented Apr 29, 2020

Would using builtins.toString on the path remove the context and fix the error?

@worldofpeace
Copy link
Contributor Author

worldofpeace commented Apr 29, 2020 •
edited

Would using builtins.toString on the path remove the context and fix the error?

Nope, I tried that at first and still got

error: in 'toFile': the file 'options.xml' cannot refer to derivation outputs, at /home/worldofpeace/Code/nix/nixpkgs/nixos/lib/make-options-doc/default.nix:89:16

when building the manual.

@jtojnar
Copy link
Contributor

jtojnar commented Apr 29, 2020 •
edited

Using the following patch,

--- a/nixos/modules/services/x11/display-managers/lightdm.nix
+++ b/nixos/modules/services/x11/display-managers/lightdm.nix
@@ -133,7 +133,7 @@ in
 
       background = mkOption {
         type = types.path;
-        default = pkgs.nixos-artwork.wallpapers.simple-dark-gray-bottom.gnomeFilePath;
+        default = let pkg = pkgs.nixos-artwork.wallpapers.simple-dark-gray-bottom; pp = "${pkg}/share/backgrounds/nixos/${pkg.src.name}"; in builtins.trace (builtins.getContext pkg.gnomeFilePath) (builtins.trace (builtins.getContext pp) pp);
         description = ''
           The background image or color to use.
         '';

it looks like the issue is actually the opposite than I thought:

trace: { "/nix/store/qry13wjmx6wbj4rcb6qz2yis4m3bhin5-simple-dark-gray-2018-08-28.drv" = { outputs = [ "out" ]; }; }
trace: { }

Even though the both strings are the same, passthru strips the context, so toFile sees a path that was not mentioned in any context and complains.

Edit: Nope. Reading it wrong again.

@jtojnar
Copy link
Contributor

jtojnar commented Apr 29, 2020 •
edited

This patch seems to work:

--- a/nixos/modules/services/x11/display-managers/lightdm.nix
+++ b/nixos/modules/services/x11/display-managers/lightdm.nix
@@ -133,9 +133,7 @@ in
 
       background = mkOption {
         type = types.path;
-        # TODO: use gnomeFilePath when fixing string context issues in make-option-doc
-        # https://github.com/NixOS/nixpkgs/issues/83863
-        default = "${pkgs.nixos-artwork.wallpapers.simple-dark-gray-bottom}/share/artwork/gnome/nix-wallpaper-simple-dark-gray_bottom.png";
+        default = pkgs.nixos-artwork.wallpapers.simple-dark-gray-bottom.gnomeFilePath;
         description = ''
           The background image or color to use.
         '';
--- a/pkgs/data/misc/nixos-artwork/wallpapers.nix
+++ b/pkgs/data/misc/nixos-artwork/wallpapers.nix
@@ -29,7 +29,7 @@ _EOF
       '';
 
       passthru = {
-        gnomeFilePath = "${pkg}/share/backgrounds/nixos/${src.name}";
+        gnomeFilePath = builtins.unsafeDiscardStringContext "${pkg}/share/backgrounds/nixos/${src.name}";
         kdeFilePath = "/share/wallpapers/${name}/contents/images/${src.name}";
       };

Obviously, not a good idea. Not only it is unsafe but it would also break dependence tracking when used in derivations.

I am not sure why does not builtins.toFile support referencing the result of derivation, ¿perhaps it happens in evaluation time so it would cause IFD?

What is even weirder, when we construct the string in place as before, it shows no context:

trace: { }

but if we do the same in repl, we get a context:

nix-repl> let pkg = pkgs.nixos-artwork.wallpapers.simple-dark-gray-bottom; pp = "${pkg}/share/backgrounds/nixos/${pkg.src.name}"; in builtins.trace (builtins.getContext pp) pp
trace: { "/nix/store/qry13wjmx6wbj4rcb6qz2yis4m3bhin5-simple-dark-gray-2018-08-28.drv" = { outputs = [ "out" ]; }; }
"/nix/store/pzgmk5wx85drxbw1ikyg50gc1whi36ag-simple-dark-gray-2018-08-28/share/backgrounds/nixos/nix-wallpaper-simple-dark-gray_bottom.png"

@jtojnar
Copy link
Contributor

jtojnar commented Apr 29, 2020

It makes sense for the manual not to depend on other attributes but at the same time user’s configuration should. Not sure if this is solvable.

@jtojnar
Copy link
Contributor

jtojnar commented Apr 29, 2020

Hmm, mkOption actually supports defaultText so we can have something like a cake and eat it too. The downside is that it is rendered like a string, not a literal:

image

@worldofpeace
Copy link
Contributor Author

@jtojnar Yay ✨. That seems fine to me. Re the investigation #86163 (comment), perhaps @edolstra can shed some light.

worldofpeace and others added 4 commits April 29, 2020 13:23
@worldofpeace
nixos-artwork: add file path attributes
62587f4 
This makes things so much easier, and we install to
the path that both gnome-backgrounds and
elementary-wallpapers install to.
@jtojnar @worldofpeace
nixos/lightdm: change background type to path
3d1706c
@worldofpeace
nixos/gnome3: install nixos wallpapers
7f3bc5b
@worldofpeace
nixos/pantheon: install nixos wallpaper
5100e4f 
Fixes  NixOS#86146
@wamserma wamserma mentioned this pull request May 13, 2020
Merged
10 tasks
@worldofpeace worldofpeace merged commit d508591 into NixOS:master Jun 7, 2020
GNOME automation moved this from In Progress to Done Jun 7, 2020
Pantheon automation moved this from In progress to Done Jun 7, 2020
@worldofpeace worldofpeace deleted the wallpaper-refactor branch June 7, 2020 18:45
@worldofpeace worldofpeace mentioned this pull request Jun 7, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

Assignees
No one assigned
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: nixos 6.topic: pantheon The Pantheon desktop environment 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
GNOME
  
Done
Pantheon
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Default NixOS wallpaper missing after update
2 participants
@worldofpeace @jtojnar