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/slim: reintroduce #75389

Closed
wants to merge 5 commits into from
Closed

nixos/slim: reintroduce #75389

wants to merge 5 commits into from

Conversation

oxij
Copy link
Member

@oxij oxij commented Dec 9, 2019

git log

nix-instantiate environment

  • Host OS: Linux 4.9, SLNOS 19.09
  • Nix: nix-env (Nix) 2.2.2
  • Multi-user: yes
  • Sandbox: yes
  • NIXPKGS_CONFIG:
{
  checkMeta = true;
  doCheckByDefault = true;
}

Things done

  • Normal login works.
  • Autologin works.
  • Special commands work.

(Tested by applying this patchset on top of NixOS 19.09, and then testing the resulting buiild on a real hardware.)

@oxij oxij requested a review from nbp as a code owner December 9, 2019 18:38
@oxij oxij mentioned this pull request Dec 9, 2019
10 tasks
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: qt/kde 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Dec 9, 2019
@@ -148,13 +148,6 @@
Now, all bash code is held to the same high standard, and the rather complex stateful manipulation of the options can be discarded.
</para>
</listitem>
<listitem>
Copy link
Contributor

@rnhmjoj rnhmjoj Dec 9, 2019

Choose a reason for hiding this comment

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

Maybe mention instead that slim is now your slim-ng

Copy link
Member

Choose a reason for hiding this comment

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

Release notes should not be undone

@jtojnar
Copy link
Member

jtojnar commented Dec 9, 2019

The main issue with SLIM is that it does not integrate well with modern desktops like GNOME, Plasma or Sway. In particular:

  • It is missing the ability to load session files from $XDG_DATA_DIRS, or some other way of loading desktop files from more than a single sessiondir. While we are currently linking upstream session files into a single tree, $out/share/xsessions and $out/share/wayland-sessions will always be different directories (session type is determined from the directory name).
  • Wayland sessions are not supported.
  • Desktop environments are moving towards using systemd for starting sessions, and without upstream, adding support would fall on us.

It is misleading when a software with limited and frozen feature set is advertised as an alternative to modern maintained DMs. In order for SLIM to get back to tree, it should be de-emphasized in the documentation, and users should be warned about its state of development and supported features.

Though, to be honest, LightDM module is not on par with SDDM and GDM at the moment either (see e.g. #56342).

@@ -39,7 +39,7 @@
can select an alternative one by picking one of the following lines:
<programlisting>
<xref linkend="opt-services.xserver.displayManager.sddm.enable"/> = true;
<xref linkend="opt-services.xserver.displayManager.gdm.enable"/> = true;
<xref linkend="opt-services.xserver.displayManager.slim.enable"/> = true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<xref linkend="opt-services.xserver.displayManager.slim.enable"/> = true;
<xref linkend="opt-services.xserver.displayManager.gdm.enable"/> = true;
<xref linkend="opt-services.xserver.displayManager.slim.enable"/> = true; # limited integration

@@ -146,7 +146,7 @@ xlink:href="https://nixos.org/nixpkgs/manual/#sec-package-naming">
/>), and to extend
it in each backend module
(<xref
linkend='ex-option-declaration-eot-backend-gdm' />,
linkend='ex-option-declaration-eot-backend-slim' />,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
linkend='ex-option-declaration-eot-backend-slim' />,
linkend='ex-option-declaration-eot-backend-gdm' />,
<xref
linkend='ex-option-declaration-eot-backend-slim' />,

@@ -167,11 +167,11 @@ services.xserver.displayManager.enable = mkOption {
};</screen>
</example>

<example xml:id='ex-option-declaration-eot-backend-gdm'>
<title>Extending <literal>services.xserver.displayManager.enable</literal> in the <literal>gdm</literal> module</title>
<example xml:id='ex-option-declaration-eot-backend-slim'>
Copy link
Member

Choose a reason for hiding this comment

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

Better use fully-featured DM here:

Suggested change
<example xml:id='ex-option-declaration-eot-backend-slim'>
<example xml:id='ex-option-declaration-eot-backend-gdm'>

<example xml:id='ex-option-declaration-eot-backend-gdm'>
<title>Extending <literal>services.xserver.displayManager.enable</literal> in the <literal>gdm</literal> module</title>
<example xml:id='ex-option-declaration-eot-backend-slim'>
<title>Extending <literal>services.xserver.displayManager.enable</literal> in the <literal>slim</literal> module</title>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<title>Extending <literal>services.xserver.displayManager.enable</literal> in the <literal>slim</literal> module</title>
<title>Extending <literal>services.xserver.displayManager.enable</literal> in the <literal>gdm</literal> module</title>

<screen>
services.xserver.displayManager.enable = mkOption {
type = with types; nullOr (enum [ "gdm" ]);
type = with types; nullOr (enum [ "slim" ]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type = with types; nullOr (enum [ "slim" ]);
type = with types; nullOr (enum [ "gdm" ]);

src = fetchFromGitHub {
owner = "oxij";
repo = pname;
rev = "b6d77d41eeb796d3788ecab2d02e616c858661e1";
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a tag here?


postPatch = ''
substituteInPlace CMakeLists.txt \
--replace /lib $out/lib
Copy link
Member

Choose a reason for hiding this comment

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

Aargh, why does not the build script use GNUInstallDirs CMake module?

Copy link
Contributor

Choose a reason for hiding this comment

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

The patch is ugly too. The build system should work in a really native way if it's literally a "For NixOS" package. GNUInstallDirs would be the way to go for that.
(though, not very fond of CMake)


cmakeFlags = [ "-DUSE_PAM=1" ];

enableParallelBuilding = true;
Copy link
Member

Choose a reason for hiding this comment

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

CMake builds in parallel by default.

enableParallelBuilding = true;

buildInputs =
[ cmake pkgconfig libjpeg libpng fontconfig freetype
Copy link
Member

Choose a reason for hiding this comment

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

Build platform dependencies like cmake and pkg-config should go to nativeBuildInputs

@oxij
Copy link
Member Author

oxij commented Dec 10, 2019 via email

@avnik
Copy link
Contributor

avnik commented Dec 10, 2019

@jtojnar I am sure, that most of slim (or slim-ng) users don't use major DEs, and use it with simple WMs (icewm/xmonad/qtile/etc). It can be mentioned in docs as recomendation to choose other one for GNOME, etc.

@worldofpeace
Copy link
Contributor

worldofpeace commented Dec 11, 2019

@oxij This package shouldn't even be called slim. It isn't slim anymore, it's a package you have taken ownership of and continued. It's likely to proliferate in use. You should even rename the slim module to slim-ng.

Also, what about:

I'd also want to emphasize the importance of xdg spec compliance.

And the biggest issue is wayland, it can't start wayland sessions.
You don't actually have to rewrite it to run on wayland, at minimum could it start a wayland-session?
We did mention that lightdm doesn't have this capability, but it's only on NixOS.
I've recently contributed canonical/lightdm@03f2189, and it should soon work properly on nixos.

So I'm seeing this being blocked on some code changes.
I'm not equipped to review C++ code, since you are making changes to slim's code I feel the need for someone with the know-how to review your work and diff the changes from the prior project.


I mentioned how if a community member forked and continued the project we'd be in good hands.
I'm glad this has happened.

Comment on lines 53 to 64
startAll;
$machine->waitForText(qr/Username:/);
$machine->sendChars("${user.name}\n");
$machine->waitForText(qr/Password:/);
$machine->sendChars("${user.password}\n");

$machine->waitForFile('${user.home}/.Xauthority');
$machine->succeed('xauth merge ${user.home}/.Xauthority');
$machine->waitForWindow('^IceWM ');

# Make sure SLiM doesn't create a log file
$machine->fail('test -e /var/log/slim.log');
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we port this to the python driver? #72828

Copy link
Member

@jtojnar jtojnar Dec 21, 2019

Choose a reason for hiding this comment

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

This should just be replacing make-test.nix with make-test-python.nix above and using:

    start_all()
    machine.wait_for_text("Username:")
    machine.send_chars("${user.name}\n")
    machine.wait_for_text("Password:")
    machine.send_chars("${user.password}\n")

    machine.wait_for_file("${user.home}/.Xauthority")
    machine.succeed("xauth merge ${user.home}/.Xauthority")
    machine.wait_for_window(r"^IceWM ")

    # Make sure SLiM doesn't create a log file
    machine.fail("test -e /var/log/slim.log")

buildInputs =
[ cmake pkgconfig libjpeg libpng fontconfig freetype
pam dbus
xorg.libX11 xorg.libXext xorg.libXrandr xorg.libXrender xorg.libXmu xorg.libXft makeWrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

what has makeWrapper been used for?

Copy link
Member

Choose a reason for hiding this comment

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

if makeWrapper is used it should be in nativeBuildInputs as well.

@worldofpeace worldofpeace added the 2.status: work-in-progress This PR isn't done label Dec 11, 2019
@worldofpeace
Copy link
Contributor

worldofpeace commented Dec 11, 2019

Can slim-ng support default or soon to be defaultSession?

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Dec 11, 2019

What for? It is a drop-in replacement.

Technically It's a different software. One should know it's getting your fork and not the original slim.

This package shouldn't even be called slim. It isn't slim anymore, it's a package you have taken ownership of and continued. It's likely to proliferate in use. You should even rename the slim module to slim-ng.

Is there a policy on this topic? For example in 34cc444 compton has been (more or less) silently replaced by a fork. Imho even if by the same author this should be mentioned in the release notes and maybe the attribute name should change as well.

@worldofpeace
Copy link
Contributor

Is there a policy on this topic? For example in 34cc444 compton has been (more or less) silently replaced by a fork. Imho even if by the same author this should be mentioned in the release notes and maybe the attribute name should change as well.

In that particular example, I don't think it should even have been aliased. An alias could be used when two identities were created for the same thing, old and new. A good example was gnome-mpv was renamed to celluloid, but it's the same code. In that example you want to help out with migraton, so you create a throw indicating that compton has been removed, but picom is essentially a drop in replacement. This is what should be done in slim-ng as well.

@worldofpeace
Copy link
Contributor

worldofpeace commented Dec 11, 2019

However, sometimes in the cases like with forks, the new maintainer doesn't re-identify this software. Things become historically confusing, and the "new" source will proliferate in distributions but it's fundamentally different. It's not as bad because it's usually 1:1 compatible, but as a packager, I'd request the maintainer to re-identify the software to avoid this.

So to make on topic with slim, we shouldn't call this slim because we're not talking about the project without a developer who also proclaimed it abandoned, and with various compatibility issues.

@oxij
Copy link
Member Author

oxij commented Dec 13, 2019 via email

@oxij
Copy link
Member Author

oxij commented Dec 13, 2019 via email

@worldofpeace
Copy link
Contributor

Out of scope of this PR.

Porting to the python testing driver is not actually of of scope.
Because this is reintroducing a test, all tests need to be written in python as the perl driver is due for removal and tests are actively being moved over #72828.
I have no reason to treat this test like any other test being added.

@worldofpeace
Copy link
Contributor

I'm not sure what you want here. Renaming the nixos module seems rather strange. Even if slim-ng would diverge wildly from slim I would rather keep the same module and add an option there to select between packages, since they would do essentially the same thing.

Moreover, since, at this moment, slim-ng is just slim with less bugs I would vote against renaming the package attribute too. Why add an alias when you can skip it?

Looking at some commits it's slowly diverting from slim, and already behaves in slightly different ways.
In my personal experience, I think a rename could be a good move here.

core review
Sure, if somebody wants to do it, feel free. But, in general, the code there is rather messy so don't blame me for something I didn't do, please. With slim-ng v1.3.8 I did my best without, hopefully, breaking anything.

I will relay it to the person that any design decisions or original code is not to come back and reflect on you directly 😄

@worldofpeace
Copy link
Contributor

The main issue with SLIM is that it does not integrate well with modern desktops like GNOME, Plasma or Sway.

It is amazing with XMonad.

The desktops mentioned there provide wayland sessions. As X has no future this should be a great concern.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jan 16, 2020

Updates? Also, is the problem in #4300 still present?

@misuzu
Copy link
Contributor

misuzu commented Jan 30, 2020

slim seems to work fine, but slimlock theme is taken neither from configuration nor from SLIM_THEMESDIR variable.

@misuzu
Copy link
Contributor

misuzu commented Feb 2, 2020

I think slimlock has always been un-configurable because lack of slimlock.conf.

slimlock from regular slim do take theme from SLIM_THEMESDIR env variable so this is kind of regression.

This reverts commit 4583e29,
reversing changes made to d6fa540.

The fact that SLiM is not officially actively maintained is not a good
reason to remove it. It works, and, in some instances, like automatic
logins, it is the only thing that actually works. See
NixOS#46396 and
NixOS#30890 (comment).
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Given there is apparently quite the interest, I suppose with the users taking up active maintainership it should be fine to get this back in. Regarding compatibility, is it possible to add a check preventing it from being used with incompatible DE's?

@@ -148,13 +148,6 @@
Now, all bash code is held to the same high standard, and the rather complex stateful manipulation of the options can be discarded.
</para>
</listitem>
<listitem>
Copy link
Member

Choose a reason for hiding this comment

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

Release notes should not be undone

name = "slim";

meta = with pkgs.stdenv.lib.maintainers; {
maintainers = [ aszlig ];
Copy link
Member

Choose a reason for hiding this comment

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

security.pam.services.slimlock = {};

environment.systemPackages = [ pkgs.slim ];

Copy link
Member

Choose a reason for hiding this comment

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

add as maintainer

@oxij
Copy link
Member Author

oxij commented Feb 10, 2020 via email

@misuzu
Copy link
Contributor

misuzu commented Feb 14, 2020

slimlock themes is working now, but there are still some issues. Try doing this:

  1. boot your machine with slim enabled
  2. enter your username and wrong password
  3. enter your username and right password
  4. observe crash without proper restart

Comment on lines 411 to 413
smbclient = samba; # added 2018-04-25
slim = throw "slim has been removed. Please use a different display-manager"; # added 2019-11-11
slimThemes = throw "slimThemes has been removed because slim has been also"; # added 2019-11-11
slim = slim-ng; # added 2019-11-11
net_snmp = net-snmp; # added 2019-12-21
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't correct. An alias is supposed to be a bind to something that's equivalent, and we've already addressed that it isn't anymore.

slim = throw "slim has been removed. Please use the replacement slim-ng or a different display-manager"

Copy link
Contributor

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

I agree that there is a place for slim-ng in NixOS, but IMHO it should be called slim-ng and clearly mention the differences with gdm/sddm in the description of the module

@@ -39,7 +39,7 @@
can select an alternative one by picking one of the following lines:
<programlisting>
<xref linkend="opt-services.xserver.displayManager.sddm.enable"/> = true;
<xref linkend="opt-services.xserver.displayManager.gdm.enable"/> = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove gdm? That's what most users probably want

@@ -99,7 +99,7 @@ xlink:href="https://nixos.org/nixpkgs/manual/#sec-package-naming">
<para>
As an example, we will take the case of display managers. There is a central
display manager module for generic display manager options and a module file
per display manager backend (sddm, gdm ...).
per display manager backend (slim, sddm, gdm ...).
</para>
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting slim first makes it seem like the first one users should try, whereas slim is a very leftfield alternative

@ryantm ryantm added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 3, 2020
@stale
Copy link

stale bot commented Jun 4, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 4, 2021
@Artturin Artturin closed this May 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: work-in-progress This PR isn't done 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: qt/kde 8.has: changelog 8.has: documentation 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 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.