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: add libFiles option #1765

Merged
merged 1 commit into from Mar 28, 2021
Merged

Conversation

markusscherer
Copy link
Contributor

The libFiles option allows home-manager to manage additional files for
xmonad in .xmonad/lib. The advantage of doing it like this compared to
just using home.file.".xmonad/lib/File.hs".source is, that we can
detect file changes and recompile xmonad accordingly.

Description

This commit adds an option libFiles that allows to add additional files to .xmonad/libs/.
The xmonad recompilation process can access files stored in this directory as modules, therefore they can influence the configuration. When we would add these files via home.file.".xmonad/lib/File.hs".source, changes to them would not cause xmonad to be recompiled, thereby making it necessary to call xmonad --recompile && xmonad -- restart after home-manager switch.

If such files are added via the new libFiles option, changes to them cause a recompilation.

Right now, unfortunately, if both xmonad.hs and the files in libFiles change, xmonad is recompiled once for every changed file, as I don't know yet how one could implement this in an elegant way.

Note, that right now there are no tests for xmonad but since only an option is added to an xmonad specific file, other components should not be affected (also, when running nix-shell --pure tests -A run.all against the branch I'm tracking, release-20.09, all tests passed).

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

Copy link
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

Sorry for the late review and thanks for the contribution.

Have you figured out a way to only restart xmonad once?

Comment on lines 107 to 110
if [[ -v DISPLAY ]] ; then
echo "Restarting xmonad"
$DRY_RUN_CMD ${config.xsession.windowManager.command} --restart
fi
Copy link
Member

Choose a reason for hiding this comment

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

I would gate the auto restarting behind an option that defaults to false if it can have any unwanted behavior (closing applications, crashing, etc.).

modules/services/window-managers/xmonad.nix Outdated Show resolved Hide resolved
@@ -75,27 +75,57 @@ in {
by Home Manager.
'';
};

libFiles = mkOption {
type = types.attrs;
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 = types.attrs;
type = types.attrsOf (types.oneOf [ types.package types.path ]);

More precise tying.

Would it make sense to allow the user to use a multiline string directly here?
In which case the oneOf should be changed to oneOf [ ..... types.lines ].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The values in libFiles are (right now at least, see rycee's comment below) assigned to home.file.".xmonad/lib/Something.hs".source which means, if I understand the documentation correctly, that only path-values are allowed. So maybe the type should be types.attrsOf types.path; On the other hand: it would of course be nice to be able to use lines also (as the name of the files is anyway given in the key).

@rycee
Copy link
Member

rycee commented Feb 21, 2021 via email

@markusscherer
Copy link
Contributor Author

I think the better option would be to change the module to build an ready xmonad package and reference that binary in .xsession.

How would one go about this? Are there any other modules which do something similar?

@berbiche
Copy link
Member

I think the better option would be to change the module to build an ready xmonad package and reference that binary in .xsession.

How would one go about this? Are there any other modules which do something similar?

I think you could achieve this using something like the following as a basis (not tested):

pkgs.runCommandLocal "xmonad-compile" {
  inherit (cfg) libFiles;
  passAsFile = [ "libFiles" ];
  nativeBuildInputs = [ xmonad ];
} ''
  mkdir -p $out/bin
  export XMONAD_CONFIG_DIR="$(pwd)/xmonad-config"
  export XMONAD_DATA_DIR="$(pwd)/data"
  export XMONAD_CACHE_DIR="$(pwd)/cache"

  mkdir -p "$XMONAD_CONFIG_DIR/lib" "$XMONAD_CACHE_DIR" "$XMONAD_DATA_DIR"

  cp ${cfg.config} xmonad-config/xmonad.hs
  # Copied from symlinkJoin, copies libFiles files to Xmonad's expected lib folder
  for i in $(cat $libFilesPath); do
    ${pkgs.xlibs.lndir}/bin/lndir -silent $i xmonad-config/lib
  done

  xmonad --recompile

  # The resulting binary name depends on the arch and os
  # https://github.com/xmonad/xmonad/blob/56b0f850bc35200ec23f05c079eca8b0a1f90305/src/XMonad/Core.hs#L565-L572
  mv "$XMONAD_DATA_DIR/xmonad-${pkgs.hostPlatform.system}" $out/bin/
''

The result of this expression would need to be assigned to xsession.windowManager.command I think.

see:
https://github.com/xmonad/xmonad/blob/a90558c07e3108ec2304cac40e5d66f74f52b803/src/XMonad/Main.hs#L99-L128
https://github.com/xmonad/xmonad/blob/56b0f850bc35200ec23f05c079eca8b0a1f90305/src/XMonad/Core.hs#L481-L525

@rycee
Copy link
Member

rycee commented Feb 22, 2021 via email

@markusscherer
Copy link
Contributor Author

@berbiche I just tried your code and the and unfortunately it doesn't compile. The offending line seems to be inherit (cfg) libFiles;. Including it causes the following error (--show-trace enabled):

error: while evaluating the attribute 'buildCommand' of the derivation 'home-manager-generation' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/pkgs/build-support/trivial-builders.nix:7:7:
while evaluating the attribute 'text' of the derivation 'activation-script' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/pkgs/build-support/trivial-builders.nix:7:7:
while evaluating 'mkCmd' at /home/markus/code/home-manager/modules/home-environment.nix:502:17, called from undefined position:
while evaluating the attribute 'data' at /home/markus/code/home-manager/modules/lib/dag.nix:91:37:
while evaluating the attribute 'data' at /home/markus/code/home-manager/modules/lib/dag.nix:85:9:
while evaluating the attribute 'data' at undefined position:
while evaluating 'g' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/attrsets.nix:276:19, called from undefined position:
while evaluating anonymous function at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:98:72, called from /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/attrsets.nix:279:20:
while evaluating the attribute 'value' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:465:9:
while evaluating the option `home.activation.checkFilesChanged.data':
while evaluating the attribute 'mergedValue' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:497:5:
while evaluating the attribute 'values' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:491:9:
while evaluating the attribute 'values' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:590:7:
while evaluating anonymous function at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:477:28, called from /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:477:17:
while evaluating definitions from `/home/markus/code/home-manager/modules/files.nix':
while evaluating 'dischargeProperties' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:549:25, called from /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:478:137:
while evaluating the attribute 'value' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:338:44:
while evaluating 'concatMapStrings' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/strings.nix:31:25, called from /home/markus/code/home-manager/modules/files.nix:240:12:
while evaluating anonymous function at /home/markus/code/home-manager/modules/files.nix:244:20, called from undefined position:
while evaluating the attribute 'onChange' at undefined position:
while evaluating 'g' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/attrsets.nix:276:19, called from undefined position:
while evaluating anonymous function at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:98:72, called from /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/attrsets.nix:279:20:
while evaluating the attribute 'value' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:465:9:
while evaluating the option `home.file..xmonad/lib/Config.hs.onChange':
while evaluating the attribute 'mergedValue' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:497:5:
while evaluating the attribute 'values' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:491:9:
while evaluating the attribute 'values' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:590:7:
while evaluating anonymous function at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:477:28, called from /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:477:17:
while evaluating definitions from `/home/markus/code/home-manager/modules/services/window-managers/xmonad.nix':
while evaluating 'dischargeProperties' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:549:25, called from /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:478:137:
while evaluating the attribute 'value' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:338:44:
while evaluating the attribute 'xsession.windowManager.command' at undefined position:
while evaluating 'g' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/attrsets.nix:276:19, called from undefined position:
while evaluating anonymous function at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:98:72, called from /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/attrsets.nix:279:20:
while evaluating the attribute 'value' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:465:9:
while evaluating the option `xsession.windowManager.command':
while evaluating the attribute 'mergedValue' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:497:5:
while evaluating the attribute 'values' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:491:9:
while evaluating the attribute 'values' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:590:7:
while evaluating anonymous function at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:477:28, called from /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:477:17:
while evaluating definitions from `/home/markus/code/home-manager/modules/services/window-managers/xmonad.nix':
while evaluating 'dischargeProperties' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:549:25, called from /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:478:137:
while evaluating 'dischargeProperties' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:549:25, called from /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:555:11:
while evaluating the attribute 'content' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/lib/modules.nix:633:14:
while evaluating the attribute 'libFiles' of the derivation 'xmonad-compile' at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/pkgs/build-support/trivial-builders.nix:7:7:
cannot coerce a set to a string, at /nix/store/lpm3a2b6ki857pw71kh67q0m1r8ji0hb-nixos-20.09.2624.1eff582e7ea/nixos/pkgs/build-support/trivial-builders.nix:7:7

Is this a trivial error? I wasn't able to find a to find a solution quickly, but anyway it should be possible to access cfg.libFiles in the build script without inheriting it. I hope I find time tomorrow to try this out.

@markusscherer
Copy link
Contributor Author

I have now a solution that works but may need some adjustments to be more idiomatic:

  config = mkIf cfg.enable (mkMerge [
    {
      home.packages = [ (lowPrio xmonad) ];
      xsession.windowManager.command = "xmonad";
    }

    (mkIf (cfg.config != null) {
      home.file.".xmonad/xmonad-${pkgs.hostPlatform.system}" = {
        source = "${
            pkgs.runCommandLocal "xmonad-compile" {
              nativeBuildInputs = [ xmonad ];
              unpackPhase = "true";
              installPhase = "true";
            } ''
              mkdir -p $out/bin

              export XMONAD_CONFIG_DIR="$(pwd)/xmonad-config"
              export XMONAD_DATA_DIR="$(pwd)/data"
              export XMONAD_CACHE_DIR="$(pwd)/cache"

              mkdir -p "$XMONAD_CONFIG_DIR/lib" "$XMONAD_CACHE_DIR" "$XMONAD_DATA_DIR"

              cp ${cfg.config} xmonad-config/xmonad.hs

              declare -A libFiles
              libFiles=(${
                lib.concatStringsSep " "
                (lib.mapAttrsToList (name: value: "['${name}']='${value}'")
                  cfg.libFiles)
              })
              for key in "''${!libFiles[@]}"; do
                cp "''${libFiles[$key]}" "xmonad-config/lib/$key";
              done

              xmonad --recompile

              # The resulting binary name depends on the arch and os
              # https://github.com/xmonad/xmonad/blob/56b0f850bc35200ec23f05c079eca8b0a1f90305/src/XMonad/Core.hs#L565-L572
              mv "$XMONAD_DATA_DIR/xmonad-${pkgs.hostPlatform.system}" $out/bin/
            ''
          }/bin/xmonad-${pkgs.hostPlatform.system}";

        onChange = ''
          # Attempt to restart xmonad if X is running.
          if [[ -v DISPLAY ]] ; then
            echo "Restarting xmonad"
            $DRY_RUN_CMD ${config.xsession.windowManager.command} --restart
          fi
        '';
      };
    })
  ]);

The solution is mostly @berbiche's suggestion but with a different way of passing the libFiles to the build script (the original suggestion didn't compile when I tried it, see my previous comment).

I also tried to set xsession.windowManager.command to the file produced by the build script, but this seems to cause problems with xmonad --restart (as it at least prefers, maybe expects (did not try yet), the executable in ~/.xmonad/, so if there is a executable not managed by home-manager, this is the one that will be executed by xmonad --restart).

@berbiche
Copy link
Member

One last thing: please set the resulting binary obtained from the compilation step to xsession.windowManager.command

@markusscherer
Copy link
Contributor Author

One last thing: please set the resulting binary obtained from the compilation step to xsession.windowManager.command

Done!

One thing I just recognized is that with this new approach xmonad --recompile fails, when executed by a user. On the other hand: although it did not fail before it should not have had an observable effect, so the change is is probably not that bad.

@berbiche
Copy link
Member

berbiche commented Mar 1, 2021

One last thing: please set the resulting binary obtained from the compilation step to xsession.windowManager.command

Done!

One thing I just recognized is that with this new approach xmonad --recompile fails, when executed by a user. On the other hand: although it did not fail before it should not have had an observable effect, so the change is is probably not that bad.

We could always install the files in $HOME/.xmonad.
When rebuilding, I assume xmonad will replace our symlinked binary.

@markusscherer
Copy link
Contributor Author

We could always install the files in $HOME/.xmonad.
When rebuilding, I assume xmonad will replace our symlinked binary.

Yes, that works (although after xmonad --recompile home-manager switch of course wants the binary to be deleted again).

Shall I link the xmonad-config files to $HOME?

The code would look like this:

    (mkIf (cfg.config != null) {
      home.file.".xmonad/xmonad.hs".source = cfg.config;
      home.file.".xmonad/xmonad-${pkgs.hostPlatform.system}" = {
        source = xmonadBin;
        onChange = ''
          # Attempt to restart xmonad if X is running.
          if [[ -v DISPLAY ]] ; then
            echo "Restarting xmonad"
            $DRY_RUN_CMD ${config.xsession.windowManager.command} --restart
          fi
        '';
      };
    })

    {
      home.file = lib.mapAttrs' (name: value:
        lib.attrsets.nameValuePair (".xmonad/lib/" + name) {
          source = value;
        }) cfg.libFiles;
    }

@berbiche
Copy link
Member

berbiche commented Mar 1, 2021

@markusscherer For the sample code you don't need the lib since a with lib; expression is present at the top of the file.

Copy link
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

I think this is ready to be merged but I just want to know @rycee's opinion on the xmonad-compile solution before merging

modules/services/window-managers/xmonad.nix Show resolved Hide resolved
@berbiche
Copy link
Member

@rycee I'd like to hear your thoughts before merging this. The PR looks good to me.

The only thing that could potentially become an issue is https://github.com/nix-community/home-manager/pull/1765/files#diff-47cbc76991d0d9b568f0f62b0b31aee6f69652b7302a7e1836e7adbe65041cc5R131-R133 but even then it will be caught during the build phase.

The `libFiles` option allows Home Manager to manage additional files
for xmonad.

Also compile xmonad during configuration build time. This avoids the
need to compile the configuration during activation.
@rycee rycee merged commit c176136 into nix-community:master Mar 28, 2021
@rycee
Copy link
Member

rycee commented Mar 28, 2021

Had a chance to try it out and it worked just fine. Much nicer to do the compilation during configuration build time instead of activation time. Many thanks @markusscherer and @berbiche!

rraval added a commit to rraval/nix that referenced this pull request May 10, 2021
This is expected breakage because of the shenanigans involved in reusing
the home-manager xmonad configuration but trying to shoehorn it into
something managed by xfce-session.
@NickHu
Copy link
Collaborator

NickHu commented Jun 21, 2021

For some reason, if I use an overlay to update to the git version of xmonad and xmonad-contrib, I get the following error on rebuild:

XMonad will use ghc to recompile, because "/build/xmonad-config/build" does not exist.
xmonad: ghc: runProcess: runInteractiveProcess: exec: does not exist (No such file or directory)

The overlay is:

self: super:
{
  haskellPackages = with self.haskell.lib; super.haskellPackages.extend (packageSourceOverrides {
    X11 = builtins.fetchTarball {
      url = "https://hackage.haskell.org/package/X11-1.10/X11-1.10.tar.gz";
    };
    xmonad = builtins.fetchTarball {
      url = "https://github.com/xmonad/xmonad/archive/refs/heads/master.tar.gz";
    };
    xmonad-contrib = builtins.fetchTarball {
      url = "https://github.com/xmonad/xmonad-contrib/archive/refs/heads/master.tar.gz";
    };
  });
}

(yeah I know it's super lazy, but I'm hoping the next XMonad release is around the corner...)

It seems like it xmonad --recompile is trying to call ghc, which is not in the nativeBuildInputs, and failing there (possibly new behaviour?). If you call nix-shell --pure -p xmonad-with-packages you'll also see that ghc is not in the shell. I'm able to fix the issue with the following patch:

diff --git a/modules/services/window-managers/xmonad.nix b/modules/services/window-managers/xmonad.nix
index 07ed284..de4945a 100644
--- a/modules/services/window-managers/xmonad.nix
+++ b/modules/services/window-managers/xmonad.nix
@@ -101,7 +101,7 @@ in {
 
     xmonadBin = "${
         pkgs.runCommandLocal "xmonad-compile" {
-          nativeBuildInputs = [ xmonad ];
+          nativeBuildInputs = [ (pkgs.haskellPackages.ghcWithPackages (p: [p.X11 p.xmonad p.xmonad-contrib])) xmonad ];
         } ''
           mkdir -p $out/bin

but this is probably a bit too heavy handed and I didn't have time to investigate further. My hypothesis is that between the released version of xmonad and the git version, the mechanism for xmonad --recompile has changed, so this PR may need an update in future.

@berbiche
Copy link
Member

Sooner the better! 😄

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/use-latest-version-of-xmonad-0-17-0/16191/11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants