Skip to content

Only apply unfree to nixos/darwin/homeManager#196

Merged
vic merged 5 commits intodenful:mainfrom
OscarMarshall:oscarmarshall/test-unfree-user-class
Feb 25, 2026
Merged

Only apply unfree to nixos/darwin/homeManager#196
vic merged 5 commits intodenful:mainfrom
OscarMarshall:oscarmarshall/test-unfree-user-class

Conversation

@OscarMarshall
Copy link
Copy Markdown
Contributor

Here's my example failing test demonstrating #195.

Copilot AI review requested due to automatic review settings February 24, 2026 23:11
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a test case to demonstrate issue #195, where using den._.unfree with a user aspect causes an evaluation error due to the user class (introduced in PR #170) attempting to set non-existent options under users.users.<userName>.

Changes:

  • Added test-user-class-works test case to verify unfree aspect works with user aspects

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread templates/ci/modules/features/batteries/unfree.nix Outdated
@vic
Copy link
Copy Markdown
Member

vic commented Feb 24, 2026

Oh, thanks for sending a proper test. :)

This is what is happening:

When the unfree battery was made it had support only for nixos (os-level nixos.nixpks.config.unfreePredicate) and for homeManager given that the Home Manager project by itself also has an nixpkgs.config.

The user forward class is trying to do set the users.users.<userName>.unfree option which does not exist. Because user class is just a short-cut for nixos.users.users I believe the correct way would be to include your unfree definition at host level.

It would be nice for user-class to also support this, right ? maybe sending a PR to the unfree battery ? or ask copilot to make a patch.

@vic
Copy link
Copy Markdown
Member

vic commented Feb 24, 2026

omg, test is passing now ?

@OscarMarshall
Copy link
Copy Markdown
Contributor Author

OscarMarshall commented Feb 24, 2026

It only breaks when you try to realize igloo.users.users.tux; that's why the non-sensical expr. Otherwise, it's identical to test-packages-set-on-home-manager just above it.

@vic
Copy link
Copy Markdown
Member

vic commented Feb 24, 2026

oh my bad, I accepted copilot change. hha,

@vic
Copy link
Copy Markdown
Member

vic commented Feb 24, 2026

your code using user class was correctly showing the bug.

@vic
Copy link
Copy Markdown
Member

vic commented Feb 24, 2026

hm now I understand more (now I'm talking like an AI) the problem seems to be more about how forward battery works. because your original code was including the un free at user aspect level not at 'user' class, right.?

@OscarMarshall
Copy link
Copy Markdown
Contributor Author

Correct. I only mention the user class because it introduced the bug.

@OscarMarshall OscarMarshall force-pushed the oscarmarshall/test-unfree-user-class branch from 0e27271 to 53b8afa Compare February 24, 2026 23:47
@OscarMarshall OscarMarshall force-pushed the oscarmarshall/test-unfree-user-class branch from 53b8afa to da51c48 Compare February 24, 2026 23:50
den.lib.take.unused aspect-chain {
${class}.unfree.packages = allowed-names;
};
den.lib.take.unused aspect-chain (
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unused is not needed anymore, I disabled the deadnix checks for now. I did not like that we were poluting with these unused calls. please remove and simplify the code.

den.default.homeManager.home.stateVersion = "25.11";
den.aspects.tux.includes = [ (den._.unfree [ "vscode" ]) ];

expr = igloo.users.users.tux.name;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add a comment here? That we are forcing evaluation of users.users.tux.name just to check that the unfree battery does not adds anything inside users.users.tux.unfree ?

The comment is needed otherwise people seeing this in the future will think the code is not testing unfree properly. As happened to me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perpaps even better, test that !(igloo.users.users.tux ? unfree)

@OscarMarshall OscarMarshall changed the title test(unfree): user class unhappy Only apply unfree to nixos/darwin/homeManager Feb 25, 2026
@OscarMarshall
Copy link
Copy Markdown
Contributor Author

Do you squash PRs? Should I manually squash my commits? Any sort of commit style I should follow?

@vic
Copy link
Copy Markdown
Member

vic commented Feb 25, 2026

All merges are via squash dont worry. Thanks for all your work :)

@vic vic merged commit 0aca761 into denful:main Feb 25, 2026
12 checks passed
@OscarMarshall OscarMarshall deleted the oscarmarshall/test-unfree-user-class branch February 25, 2026 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants