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

Quoting "$vars" in xsession #25199

Merged
merged 1 commit into from Apr 27, 2017
Merged

Quoting "$vars" in xsession #25199

merged 1 commit into from Apr 27, 2017

Conversation

nyarly
Copy link
Contributor

@nyarly nyarly commented Apr 25, 2017 •
edited

Motivation for this change

The clause that fixes the call when SDDM execs xsession was broken (not just for me?) because the argument variables weren't quoted, so session names like 'xmonad + xterm' were split by bash, and the single quotes were treated as syntax errors.

I think this will correct the issue, but I'm unclear how to test the change locally to be sure, since it's a module change, and the actual script is produced by a function call.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@nyarly, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vcunat, @edolstra and @dezgeg to be potential reviewers.

Mic92
Mic92 reviewed Apr 25, 2017
View reviewed changes
nixos/modules/services/x11/display-managers/default.nix
@@ -33,7 +33,7 @@ let
#! ${pkgs.bash}/bin/bash

# Handle being called by SDDM.
if test "''${1:0:1}" = / ; then eval exec $1 $2 ; fi
if test "''${1:0:1}" = / ; then eval exec "$1" "$2" ; fi
Copy link
Member

@Mic92 Mic92 Apr 25, 2017 •
edited

Choose a reason for hiding this comment

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

this one also handles multiple arguments (more then three) correctly:

if test "''${1:0:1}" = / ; then
  shift;
  eval exec "$@";
fi

it did not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without being able to reasonably test the change, I was trying to limit it to things I was 100% sure would work.

@grahamc
Copy link
Member

grahamc commented Apr 26, 2017 •
edited

LGTM

  • ./nixos/tests/sddm.nix: passed
  • ./nixos/tests/gnome3-gdm.nix: failed...
  • ./nixos/tests/gnome3.nix: passed
  • ./nixos/tests/plasma5.nix: passed
  • ./nixos/tests/slim.nix: passed
  • ./nixos/tests/i3wm.nix: passed
  • ./nixos/tests/lightdm.nix: passed
  • ./nixos/tests/xfce.nix: passed

the gdm test loops over:

machine: must succeed: xwininfo -root -tree | sed 's/.*0x[0-9a-f]* \"\([^\"]*\)\".*/\1/; t; d'
machine# No protocol specified
machine# xwininfo: error: unable to open display ":0.0"
machine: exit status 0

forever. Also does this on master, and appears to do the same on 17.03

@grahamc
Copy link
Member

grahamc commented Apr 26, 2017

Can you provide a configuration which reproduces the problem? I'm not sure how to test in your use case.

@nyarly
Copy link
Contributor Author

nyarly commented Apr 26, 2017

Something like:

{ config, pkgs, ... }:
{
  services = {
    xserver = {
      enable = true;
      exportConfiguration = true;
      
      displayManager.sddm.enable = true;
      windowManager = {
        i3.enable = true;
        xmonad.enable = true;
        default = "i3";
      };
    };
}

should be sufficient.

Selecting from the session options on login doesn't work - IIRC you get i3 + xterm every time. (Or else xsession crashes entirely...) I'm not sure how to test it automatically.

@fpletz
Copy link
Member

fpletz commented Apr 27, 2017

A friend is also experiencing the very same problem with i3 and slim.

Does anybody know which commit introduced this regression?

@nyarly
Copy link
Contributor Author

nyarly commented Apr 27, 2017

@fpletz I don't - since I switched to NixOS on my desktop, this has been an issue for me. It seems to be a problem with the way that SDDM calls xsession

@fpletz fpletz merged commit 0d72629 into NixOS:master Apr 27, 2017
@fpletz
Copy link
Member

fpletz commented Apr 27, 2017

Okay, let's merge this. Quoting shell variables, especially in this case, is good practice anyways. ;)

Thanks a lot!

@grahamc
Copy link
Member

grahamc commented Apr 27, 2017

I'm just worried we depended upon the bad behavior somehow. Should we backport?

@nyarly
Copy link
Contributor Author

nyarly commented Apr 27, 2017

On the one hand, I can't see how Nix would depend on erroneous shell splitting; worse: I have no idea to determine that.

What I was thinking though: I would have preferred to start this process by overriding my local definition of xession, but the file is defined in the module. Would it make sense to extract it out to a package that could be overridden more easily, especially since its one of those places that a lot of system configuration happens?

Alternatively, if there's a way to override (or overlay?) modules, that would be great, and I'd've done that in a heartbeat if I understood how.

@nyarly
Copy link
Contributor Author

nyarly commented Apr 28, 2017

@grahamc If the "backport" question relates to 17.03, I'd be pro that.

@primeos
Copy link
Member

primeos commented Apr 29, 2017

Together with #25138 this should fix all problems with all display/login managers. I would wait ~1 week (to see if this will cause any unexpected problems on nixos-unstable) and backport both commits to 17.03 (after testing it on that branch).

primeos pushed a commit to primeos/nixpkgs that referenced this pull request May 6, 2017
@nyarly @primeos
nixos/display-managers: Quote "$vars" (NixOS#25199)
c893d7a 
(cherry picked from commit 0d72629)
@primeos
Copy link
Member

primeos commented May 6, 2017

Backported to 17.03 (c893d7a and 9283310) and tested with SDDM, LightDM and SLiM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@fpletz fpletz fpletz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@nyarly @mention-bot @grahamc @fpletz @primeos @Mic92