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

xmonad: put the correct xmonad binary in PATH #100141

Merged
merged 4 commits into from Oct 13, 2020

Conversation

@xaverdh
Copy link
Contributor

@xaverdh xaverdh commented Oct 10, 2020

Motivation for this change

If services.xserver.windowManager.xmonad.config is used, until this pr the module would put the official xmonad package in systemPackages which is almost certainly not what you want. After this pr, if you e.g. rebuild your config and then restart xmonad with xmonad --restart, it will take the binary from PATH which will be the one you specified in your config.
I also took the liberty to rename some things to hopefully make their purpose more obvious.

Personally I would simply consider the old behavior a bug, but not sure if somebody relies on it for some strange reason?

By the way don't get confused by the diff; the relevant line here is unchanged, but xmonad is bound to a different value.

Things done
  • Tested this with my own xmonad config (which uses services.xserver.windowManager.xmonad.config). Should be no-op if not using the config option.
  • 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.
@xaverdh
Copy link
Contributor Author

@xaverdh xaverdh commented Oct 10, 2020

cc @Profpatsch (might be of interest to you?)

@Lassulus
Copy link
Contributor

@Lassulus Lassulus commented Oct 10, 2020

seems like a good idea. This sadly has broken the xmonad test in nixos/tests because it depended on this "broken" behavior. Would be nice of you if you could fix it also :)

@xaverdh
Copy link
Contributor Author

@xaverdh xaverdh commented Oct 10, 2020

Hmm the test is actually tricky. It uses launch which does not support the xmonad command line options..
There probably was a reason why launch was used instead of the standard xmonad entry point, but lets see.

(I can't run the tests locally atm because qemu is broken on my hardware – gives me a kernel panic)

@xaverdh
Copy link
Contributor Author

@xaverdh xaverdh commented Oct 10, 2020

Ok, I finally found machine with working qemu. The test won't work as is, essentially because it tests precisely the old behavior (which makes me think that somebody wanted it?). Using the standard xmonad entry point fails because that makes wrong assumptions about the binary name, using launch would require reimplementing some logic from the xmonad module, which I would rather not do for a simple test.

Actually my favorite solution for this problem would be to just drop that part of the test. Maybe have a second test that actually tests the code path without custom config.

@xaverdh xaverdh force-pushed the xaverdh:xmonad-correct-path branch from 01fab24 to e24374e Oct 10, 2020
@xaverdh
Copy link
Contributor Author

@xaverdh xaverdh commented Oct 10, 2020

I finally found a way to make the test work while staying close to the original version. It does test restarting as the original test did (at least I think that was the intention). Just have to be careful to avoid shelling out to xmonad --restart which will not work with launch..

@xaverdh xaverdh force-pushed the xaverdh:xmonad-correct-path branch from e24374e to 156aa24 Oct 10, 2020
@xaverdh
Copy link
Contributor Author

@xaverdh xaverdh commented Oct 10, 2020

(fixed a type in the commit message)

@xaverdh
Copy link
Contributor Author

@xaverdh xaverdh commented Oct 11, 2020

Ok, so I now understand why people might want the vanilla xmonad in their PATH:

The standard keybinding for restarting xmonad actually goes through a somewhat convoluted process (detailed in this discussion), which involves executing the xmonad from PATH and makes assumptions about it, namely that it uses the xmonad entry point.

But if people use launch as entry point (the example actually advertises this), putting the configured binary in path will break that key binding.
I think its still more consistent to have the configured xmonad in PATH, as this

  • avoids accidentally dropping you into an vanilla xmonad (without any config) by not overwriting and pressing the standard mod+q key binding
  • allows you to restart into the new xmonad after rebuilding your system by adding a key binding that calls restart "xmonad" True

But the module description / docs could be improved somewhat in this regard ;-) will look into that..

The one thing I am still worried about a bit is that some people may rely on the following workflow atm:
configure xmonad with nixos + config but have a copy of that xmonad.hs in .xmonad
-> change that local copy, use mod+q to recompile and exec into it
-> when you are satisfied with the result, copy+paste that into your nixos config
That workflow can be saved by making your own binary do the recompilation / exec on startup, but requires significantly more insight than previously.

Apart from what this pr does there are two other other sensible approaches I can think of in the case of xmonad configured through the config module option:

  • put nothing into PATH, but put both xmonad versions in a read only variable of the module: This will avoid accidentally dropping ppl into unconfigured xmonad, if they want either the old behavior or the one from this pr, they can explicitly put either one or the other in PATH
  • put both in PATH but under different names (say xmonad and xmonad-configured). Can still accidentally dropping ppl into unconfigured xmonad, but also allows users to restart into xmonad-configured if they want.
@xaverdh
Copy link
Contributor Author

@xaverdh xaverdh commented Oct 11, 2020

@Lassulus ah I looked through the history of this module, and you actually submitted the original pr for the config option (thanks by the way!); after reading that, I think it wasn't intentional to put the plain xmonad in PATH, right?

The one thing I am still worried about a bit is that some people may rely on the following workflow atm

^ It occurs to me now, that you can also always just temporarily disable the config option, use plain xmonad with recompilation semantics, and re-enable it afterwards...

@xaverdh
Copy link
Contributor Author

@xaverdh xaverdh commented Oct 12, 2020

Pushed some docs. Is there a known way to show inline code, in such a way that there is a visible difference to ordinary text in man configuration.nix? I tried <code></code> and it failed me :-/

Apart from that I think I am now happy with the state of this pr.

xaverdh added 2 commits Oct 10, 2020
The old (slightly broken) behavior of the xmonad module was to put the vanilla xmonad binary into PATH. This was changed to put the users xmonad into PATH instead.

But since the config for the xmonad test uses `launch` (to avoid xmonads self-recompilation logic), it now can't handle the `--restart` flag anymore. So instead use a key binding for restarting, and let xmonad spawn a new xterm on restart.

The key binding has to be explicitly added because the default binding
will shell out to `xmonad --restart` and therefore not work with the `launch` entrypoint.
@xaverdh xaverdh force-pushed the xaverdh:xmonad-correct-path branch from 6cee503 to 206c668 Oct 12, 2020
@xaverdh
Copy link
Contributor Author

@xaverdh xaverdh commented Oct 12, 2020

(fixed up a remnant from some early experimentation in the nixos test)

@Lassulus
Copy link
Contributor

@Lassulus Lassulus commented Oct 12, 2020

no, putting the original xmonad was indeed not intentional, but never bothered me enough to actually change it. While you are on it. Maybe you can add some maintainers to the service while you are on it? (like you and me for example)
And thanks for your contribution :)

@xaverdh
Copy link
Contributor Author

@xaverdh xaverdh commented Oct 12, 2020

before merging this @NeQuissimus might want to have a look, because he is listed as maintainer of the xmonad test (just noticed)

@NeQuissimus
Copy link
Member

@NeQuissimus NeQuissimus commented Oct 13, 2020

I think I adopted the test at one point when it was broken. I don't really remember though :D I do use XMonad, so I appreciate this!

@Lassulus Lassulus merged commit 53f810c into NixOS:master Oct 13, 2020
17 checks passed
17 checks passed
tests tests
Details
action
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7389407"; rev="7389407490e3691bcdbd97d680b4e843ceda5609"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7389407"; rev="7389407490e3691bcdbd97d680b4e843ceda5609"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7389407"; rev="7389407490e3691bcdbd97d680b4e843ceda5609"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7389407"; rev="7389407490e3691bcdbd97d680b4e843ceda5609"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7389407"; rev="7389407490e3691bcdbd97d680b4e843ceda5609"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7389407"; rev="7389407490e3691bcdbd97d680b4e843ceda5609"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7389407"; rev="7389407490e3691bcdbd97d680b4e843ceda5609"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@copyread
Copy link

@copyread copyread commented Oct 13, 2020

This fixes configuration not updating after recompiling & restarting? Appreciate it.

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Oct 13, 2020

I don’t change my config often enough to particularly care, but great to see it works now!

@xaverdh xaverdh deleted the xaverdh:xmonad-correct-path branch Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.