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

Support conditional module loading #501

Merged
merged 1 commit into from Dec 22, 2018
Merged

Support conditional module loading #501

merged 1 commit into from Dec 22, 2018

Conversation

rycee
Copy link
Member

@rycee rycee commented Dec 16, 2018

In particular, mark Linux specific modules as such.

@rycee rycee self-assigned this Dec 16, 2018
@rycee rycee force-pushed the module/platforms branch 2 times, most recently from 34de345 to fed722b Compare December 16, 2018 13:10
@rycee
Copy link
Member Author

rycee commented Dec 16, 2018

@jwiegley Hi, I know you have a fairly substantial HM config under macOS. Would you mind trying this branch out? I've added code to not load the modules that are Linux specific on non-Linux platforms, which hopefully will give a more useful home-configuration.nix(5) man page and perhaps also slightly faster load time.

@rycee rycee mentioned this pull request Dec 16, 2018
@jwiegley
Copy link
Contributor

@rycee Sure. It builds fine here, I'll let you know if there are any problems.

@league
Copy link
Collaborator

league commented Dec 17, 2018

@rycee Seems to evaluate fine for me too, and home-manager news doesn't list Linux-specific stuff like xsession.

@league
Copy link
Collaborator

league commented Dec 17, 2018

@rycee: Ah, I should have run home-manager switch again to ensure it was bootstrapping correctly. I encountered a reference to systemd.user = ... that wasn't behind a mkIf or similar. So that's something I got away with previously but flagged an error now.

Similarly with xresources.*. That one is behind a mkIf cfg.enable where cfg.enable is false, but it still flags an error because that setting no longer exists. Would I have to update similarly using a mkIf (cfg.enable && isLinux) like you do in HM itself?

@rycee
Copy link
Member Author

rycee commented Dec 17, 2018

@league Ah, yeah. That is a bit unfortunate, I wonder how many mac users who are reusing their Linux config?

In any case, I think it is not enough to do something like

{
  some.option = "foo";

  xresources = mkIf (pkgs.hostPlatform.isLinux && cfg.enable) {
    # …
  };
}

it would have to be

mkMerge [
  (mkIf (pkgs.hostPlatform.isLinux && cfg.enable) {
    xresources = {
      # …
    };
  })

  {
    some.option = "foo";
  }
]

Which is a bit unfortunate. It is probably nicest to put the Linux specific stuff in a separate file that is conditionally imported into your configuration. Maybe something like

imports = [ ./foo.nix ./bar.nix ]
  ++ optional pkgs.hostPlatform.isLinux ./linux-conf.nix;

?

@league
Copy link
Collaborator

league commented Dec 18, 2018

@rycee Yeah, that would require some refactoring of what I have now. I recently built up a more extensive configuration for NixOS and other Linux machines, that uses imports and enable flags more thoroughly, and also the home-manager.users.NAME module, so all user profiles are updated with a single nixos-rebuild. My Mac config links into that, but just doesn't enable very much of it.

It's not a problem for me personally to partition those imports based on the platform, although I'm not in a rush to do it either. If you merged this I'd probably pin a version of H-M before the change until I'm ready to do that refactoring. But it's definitely not so onerous that I'd maintain a separate branch to avoid it.

But I guess this experience answers the question, that it can be a breaking change for some of us.

@rycee
Copy link
Member Author

rycee commented Dec 22, 2018

@league Thanks for the good explanation!

I think I'll sit on this for a while and try to figure out a more smooth way to introduce this change. I guess the ideal would be to have something like stateVersion <= "18.09" || hostPlatform.isLinux as condition but the state version is defined within a module itself so it is a bit tricky to access here. I shall retire to my cave to ponder 🙂

@rycee
Copy link
Member Author

rycee commented Dec 22, 2018

I realized that the condition still might be useful for future modules so I've cleaned up the PR to allow all modules even on mac but still let us use conditional inclusion for future modules.

I'll still have to figure out a way to have the old modules that are Linux only, though. But I'll merge and close this for now and make a new PR when I've come up with something.

Many thanks @jwiegley and @league for trying this out!

Also make use of this functionality for the `programs.chromium`
module.

See #501
@rycee rycee merged commit 218a8c4 into master Dec 22, 2018
@rycee rycee deleted the module/platforms branch December 22, 2018 23:05
rycee added a commit that referenced this pull request Jan 14, 2019
Also make use of this functionality for the `programs.chromium`
module.

See #501

(cherry picked from commit 218a8c4)
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

3 participants