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.

Sign up for GitHub

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

nixos/gdm: add option to configure monitors.xml #107850

Closed
wants to merge 1 commit into from
Closed

nixos/gdm: add option to configure monitors.xml #107850

wants to merge 1 commit into from

Conversation

isobit
Copy link

@isobit isobit commented Dec 28, 2020 •
edited

Motivation for this change

This option allows users to configure GDM to respect custom monitor
setups, such as those generated by GNOME in ~/.config/monitors.xml.
See this section of the ArchWiki GDM page regarding monitor settings:
https://wiki.archlinux.org/index.php/GDM#Setup_default_monitor_settings

The GDM home directory in NixOS is a tmpfs at /run/gdm, so the config
is persisted in a similar way to the pulseaudio config, by adding a
systemd tmpfiles rule to symlink it from a generated file written to the
nix store.

Relevant discourse thread: https://discourse.nixos.org/t/gdm-monitor-configuration/6356

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.

@isobit isobit marked this pull request as ready for review December 28, 2020 22:27
@worldofpeace worldofpeace requested a review from a team December 30, 2020 23:53
@isobit
nixos/gdm: add option to configure monitors.xml
b15435e 
This option allows users to configure GDM to respect custom monitor
setups, such as those generated by GNOME in `~/.config/monitors.xml`.
See this section of the ArchWiki GDM page regarding monitor settings:
https://wiki.archlinux.org/index.php/GDM#Setup_default_monitor_settings

The GDM home directory in NixOS is a tmpfs at `/run/gdm`, so the config
is persisted in a similar way to the pulseaudio config, by adding a
systemd tmpfiles rule to symlink it from a generated file written to the
nix store.

Relevant discourse thread: https://discourse.nixos.org/t/gdm-monitor-configuration/6356
@isobit
Copy link
Author

isobit commented Jan 6, 2021

I changed the type from lines to nullOr str to use null as the zero value as well as avoid issues with concatenation merging since the option value is expected to be a full XML file. I'm a nixpkgs newbie so definitely let me know if there's a more appropriate type or pattern to use for something like this!

Tested and confirmed working using nixos-rebuild test -I nixpkgs=<path to my nixpkgs checkout>.

One other thing I wanted to note is that removing a definition for this option (after setting it and building at least once) won't have an effect until the system is restarted since the rebuild will only remove the tmpfile rule, which doesn't result in the config being unlinked until a fresh tmpfs is created on reboot. Open to suggestions if there's an approach that avoids this or if I should make a note of that behavior somehow.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/gdm-monitor-configuration/6356/3

@justinas
Copy link
Contributor

Prematurely: not stale. I'm interested in this.

@tomberek
Copy link
Contributor

@isobit it would be unexpected for a reboot is necessary unless it it documented. For this reason the tmpfile.rules system is not ideal. Often you'll see an ExecPreStart populating this directory with content. You'll also see the usage of serviceConfig.StateDirectory.

caevee
caevee approved these changes Sep 18, 2021
View reviewed changes
Copy link

@caevee caevee left a comment

Choose a reason for hiding this comment

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

Tested on nixos-21.05 and fixed my issue. Backport would be appreciated.

@fufexan
Copy link
Contributor

fufexan commented Sep 18, 2021

@NixOS/gnome can this be merged with tomberek's recommendations?

VergeDX
VergeDX reviewed Sep 27, 2021
View reviewed changes
Copy link
Member

@VergeDX VergeDX left a comment

Choose a reason for hiding this comment

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

LGTM

@rofrol
Copy link
Contributor

rofrol commented Nov 15, 2021

Any news on that?

@sandangel
Copy link

sandangel commented May 21, 2022 •
edited

Hi, may I ask if we could merge this PR?

jtojnar
jtojnar requested changes May 21, 2022
View reviewed changes
Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Sorry, I feel that this is too niche a feature to warrant a NixOS option. It does not really add anything over users customizing the contents of /run/gdm/.config/monitors.xml themselves, other that somewhat obscure documentation that already understaffed GNOME team would have to end up maintaining.

By the way, if you use home-manager through NixOS configuration, you can just do:

  home-manager.users.gdm = {
    home.file.".config/monitors.xml".text = ''
      <monitors version="2">
        …
      </monitors>
    '';
  };

Though note that gdm will have to be in nix.settings.allowed-users NixOS option in order for the home-manager activation systemd service to be able to run. This is true unless you override the NixOS option, since the option defaults to [ "*" ].

@isobit
Copy link
Author

isobit commented May 21, 2022 •
edited

My thought was that the /run/gdm home setup was enough of a quirk of the NixOS GDM setup that it would be helpful to have an option that handles the symlinking, if users wanted a system level config (as opposed to ~/.config/monitors.xml). If you feel strongly though I'm fine closing this and keeping it in my personal config.

To elaborate a bit, the issue that I and other users in the discourse thread ran into was needing the config to exist at the system level so that even the GDM login screen would display on a preferred monitor, so ~/.config/monitors.xml was insufficient. It wasn't apparent how that could be configured since it wasn't simply the config file itself.

@sandangel
Copy link

I have the file already in ~/.config/monitors.xml. But the display was not correct at GDM login screen. that's why I would prefer an option to config GDM resolution.

This is what I have ended up with:

  systemd.tmpfiles.rules = [
    ''f+ /run/gdm/.config/monitors.xml - gdm gdm - <monitors version="2"><configuration><logicalmonitor><x>0</x><y>0</y><scale>2</scale><primary>yes</primary><monitor><monitorspec><connector>Virtual-1</connector><vendor>unknown</vendor><product>unknown</product><serial>unknown</serial></monitorspec><mode><width>4112</width><height>2572</height><rate>60.005760192871094</rate></mode></monitor></logicalmonitor></configuration></monitors>''
  ];

Using the symlink approach, I still see the link under root user, that's why I use f+ to have that under gdm user.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 8, 2023
@isobit isobit closed this Jan 31, 2023
@jtojnar jtojnar mentioned this pull request Apr 24, 2023
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VergeDX VergeDX VergeDX left review comments

@jtojnar jtojnar jtojnar requested changes

@caevee caevee caevee approved these changes

Assignees
No one assigned
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 1 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet