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

treewide: add overlays option #866

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

brckd
Copy link
Contributor

@brckd brckd commented Feb 16, 2025

Adds the stylix.overlays.enable option which can be disabled to remove all overlays. This is to handle scenarios where overlays cannot be set, because the configuration doesn't handle its own nixpkgs instance. Fixes #865.

Copy link
Contributor

@awwpotato awwpotato left a comment

Choose a reason for hiding this comment

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

lgtm

@brckd
Copy link
Contributor Author

brckd commented Feb 16, 2025

Haven't really tested it so far FYI.

@awwpotato
Copy link
Contributor

tested it with my config and it's passing nix flake check

@brckd
Copy link
Contributor Author

brckd commented Feb 17, 2025

I used lib.mkIf to conditionally add overlays. Wouldn't #865 (comment) be relevant?

It seems that it's not.

@trueNAHO
Copy link
Collaborator

This PR does not resolve the root problem, and instead works around it by simply disabling affected code. With commit nix-community/home-manager@662fa98 merged, we can take our time to properly resolve this.

@danth, any ideas?

@brckd
Copy link
Contributor Author

brckd commented Feb 17, 2025

AFAIK this is the proper solution, because overlays cannot be set from within home manager when useGlobalPkgs = true. I read that it was never really possible, but there was no warning that recognized it before. So by design there is no way around it. We could add a warning when implicitly setting overlays.enable = false in the HM integration though.

Comment on lines +41 to +42
default = true;
example = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK this is the proper solution, because overlays cannot be set from within home manager when useGlobalPkgs = true. I read that it was never really possible, but there was no warning that recognized it before. So by design there is no way around it.

Any way we can use useGlobalPkgs as default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's used by the HM integration to override the default value.

(lib.mkIf config.home-manager.useGlobalPkgs {
home-manager.sharedModules = [ disableOverlaysModule ];
})

disableOverlaysModule = {
config.stylix.overlays.enable = false;
};

We could think about using lib.mkDefault though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could think about using lib.mkDefault though.

We generally avoid lib.mkDefault: #388.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it's used by the HM integration to override the default value.

(lib.mkIf config.home-manager.useGlobalPkgs {
home-manager.sharedModules = [ disableOverlaysModule ];
})

disableOverlaysModule = {
config.stylix.overlays.enable = false;
};

Should we do that or leave it as-is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you asking me? I added it because it covers the case where one would be required to set this anyways.

Copy link
Contributor

@donovanglover donovanglover left a comment

Choose a reason for hiding this comment

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

Tested and works. No more home-manager warnings with useGlobalPkgs

@danth
Copy link
Owner

danth commented Feb 21, 2025

This PR does not resolve the root problem, and instead works around it by simply disabling affected code. With commit nix-community/home-manager@662fa98 merged, we can take our time to properly resolve this.

@danth, any ideas?

My thoughts would be to treat nixpkgs overlays as a separate platform from NixOS / Home Manager / Darwin, and import it automatically where possible, similar to how we integrate Home Manager within NixOS.

The overall nixpkgs platform would be a function stylix config → nixpkgs overlay and each module would have the option to include a nixpkgs.nix which is also a function with this signature.

For example, the GNOME module's nixpkgs.nix could look like:

stylixConfig: self: super:

let theme = theme = self.callPackage ./theme.nix {
  inherit (stylixConfig) colors templates;
};

in {
  gnome-shell = super.gnome-shell.overrideAttrs (oldAttrs: {
    # Themes are usually applied via an extension, but extensions are
    # not available on the login screen. The only way to change the
    # theme there is by replacing the default.
    postFixup =
      (oldAttrs.postFixup or "")
      + ''
        cp ${theme}/share/gnome-shell/gnome-shell-theme.gresource \
          $out/share/gnome-shell/gnome-shell-theme.gresource
      '';
    patches = (oldAttrs.patches or [ ]) ++ [
      ./shell_remove_dark_mode.patch
    ];
  });
}

To validate that the format of the stylixConfig parameter is correct, we could use lib.types.submodule and manually call its check function: I was already planning to create such a submodule type anyway to facilitate nesting the global options under stylix.theme and having a different stylix.targets.*.theme per target.

@brckd
Copy link
Contributor Author

brckd commented Feb 21, 2025

My thoughts would be to treat nixpkgs overlays as a separate platform from NixOS / Home Manager / Darwin, and import it automatically where possible, similar to how we integrate Home Manager within NixOS.

Good idea, I have also seen other flake systems separating overrides into separate modules. I imagine they could also be a separate output like stylix.nixosModules.overrides.

The overall nixpkgs platform would be a function stylix config → nixpkgs overlay and each module would have the option to include a nixpkgs.nix which is also a function with this signature.

Why not use the usual module args like { config, lib, ... } instead of stylixConfig?

Adds the `stylix.overlays.enable` option which can be disabled to
remove all overlays. This is to handle scenarios where overlays cannot
be set, because the configuration doesn't handle its own nixpkgs
instance. Fixes danth#865.
@brckd brckd force-pushed the treewide/add-overlays-option branch from e3fe2c6 to 8364532 Compare February 24, 2025 23:35
@danth
Copy link
Owner

danth commented Feb 25, 2025

I imagine they could also be a separate output like stylix.nixosModules.overrides.

I believe «flake».overlays.default would be the correct output for this. However, that doesn't provide a way to pass in the stylixConfig without breaking the expected format. So, we could use «flake».lib.«some function name» instead and allow the user to pass their config as a function argument, and then the function returns a valid overlay.

Why not use the usual module args like { config, lib, ... } instead of stylixConfig?

Unlike the existing platforms, nixpkgs doesn't use a module system, so this attribute set doesn't exist naturally.

We could construct it, but config would only contain config.stylix and nothing else, so it can be simplified to just stylixConfig. This could be renamed to just config, but that might cause confusion. lib can already be accessed as self.lib and/or super.lib.

I suppose we could still format the arguments as { stylixConfig, self, super } so that it's possible to use ... for any unused ones.

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.

bug: rebuild fails after home manager update
5 participants