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

Fix potential recursion between users.users and home-manager.users #6622

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Bert-Proesmans
Copy link

@Bert-Proesmans Bert-Proesmans commented Mar 13, 2025

Description

Changes the strategy how home-manager integrates with users.users.<name>.packages to pull instead of push. This side-steps the potential circular dependency when the attribute sets home-manager.users or users.users are calculated from each other.

Checklist

  • Change is backwards compatible. (AFAIK, please doublecheck edge cases)

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all
    or nix build --reference-lock-file flake.lock ./tests#test-all using Flakes.

  • 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.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

@Bert-Proesmans Bert-Proesmans marked this pull request as ready for review March 13, 2025 23:33
@khaneliman
Copy link
Collaborator

error: attribute 'enable' missing
       at /home/runner/work/home-manager/home-manager/nixos/common.nix:56:10:
           55|       config = mkIf
           56|         (config.enable && cfg.useUserPackages && (lib.hasAttr name cfg.users)) {
             |          ^
           57|           packages = [ cfg.users.${name}.home.path ];

@Bert-Proesmans
Copy link
Author

I based that if-test on the users options on unstable. looks like 24.11 does not have the "enable" option backported.
supposing we want backwards compatibility accross nixpkgs versions, is the preference to remove that check or do i also validate if 'enable' is even an option?

…s and nixos users configuration

Pushing users.users.<name>.packages from matching home-manager users leads to a circular dependency when
one attribute set is calculated from the other.
A configuration pull approach replaces the previous one to break up the circular dependency. This new approach
allows more flexibility in configuring both option sets, including calculating from each other.

EXAMPLE;

```nix
{lib, /* custom arg */ flake, config, ...}: {
    home-manager.useUserPackages = true;
    home-manager.users = builtins.intersectAttrs (lib.filterAttrs (_: v: v.isNormalUser) config.users.users) (flake.outputs.homeModules.users);
}
```

EXAMPLE;
```nix
{lib, /* custom arg */ flake, config, ...}: {
    home-manager.useUserPackages = true;
    home-manager.users = { inherit (flake.outputs.homeModules.users) demo-user; };
    users.users = lib.mapAttrs (_: _: { isNormalUser = true; }) (lib.filterAttrs (_: v: v.programs.git.enable) config.home-manager.users);
}
```

fixes nix-community#594
@Bert-Proesmans
Copy link
Author

Bert-Proesmans commented Mar 14, 2025

I checked the impact of setting on users.<name>.packages and it doesn't look like anything could go wrong, so removing the check for backwards compatibility should be ok.
Now, this change manipulates a non-owned option set so it might be a good idea to look into future proofing. Home-manager shouldn't generally do anything to the user if that user is disabled AFAIK, and "enable" is a standardised/intuitive option (so won't be changed in the future). This is an argument to keep the check to prevent future (semi-)related breaking and prevent future unwanted side-effects.

I rebased the commits per checklist instructions. The tests do not all succeed, can't directly say if this error is related to this change (doesn't look so at first sight);

error: attribute 'applicationName' missing
       at /nix/store/g4ppspdl4fy7hnp4jgjl4ll03v7i08w3-source/pkgs/applications/networking/browsers/firefox/wrapper.nix:167:23:
          166|         inherit icon;
          167|         desktopName = browser.applicationName;
             |                       ^
          168|         startupNotify = true;

EDIT; Regarding tests, I get failures different from CI on my local machine. Is there impurity in the testset? I'll need some guidance on how to tackle the errors.

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.

2 participants