Skip to content

treewide: use mkTarget (batch 3)#1371

Merged
trueNAHO merged 1 commit intonix-community:masterfrom
0xda157:treewide-mkTarget-3
Jun 3, 2025
Merged

treewide: use mkTarget (batch 3)#1371
trueNAHO merged 1 commit intonix-community:masterfrom
0xda157:treewide-mkTarget-3

Conversation

@0xda157
Copy link
Copy Markdown
Contributor

@0xda157 0xda157 commented May 23, 2025

Things done

Notify maintainers

@0xda157
Copy link
Copy Markdown
Contributor Author

0xda157 commented May 23, 2025

@MattSturgeon could you take a look?

error: infinite recursion encountered

@panchoh panchoh mentioned this pull request May 24, 2025
5 tasks
@Flameopathic
Copy link
Copy Markdown
Contributor

Flameopathic commented May 25, 2025

The infinite recursion here seems to be coming from setting config.lib.stylix, and it may be because setting it is based on an option in config.lib.stylix (colors). I'm not sure what could be done here.

@MattSturgeon
Copy link
Copy Markdown
Member

@MattSturgeon could you take a look?

error: infinite recursion encountered

I'll try to have a look a bit later

@0xda157
Copy link
Copy Markdown
Contributor Author

0xda157 commented May 25, 2025

The infinite recursion here seems to be coming from setting config.lib.stylix, and it may be because setting it is based on an option in config.lib.stylix (colors). I'm not sure what could be done here.

It's a fundamental issue of having the config.lib.stylix.i3.bar, I'll drop i3 from this pr and look into fixing it in a separate pr.

@0xda157 0xda157 force-pushed the treewide-mkTarget-3 branch from b0d8d3f to 55edd69 Compare May 25, 2025 18:29
@MattSturgeon
Copy link
Copy Markdown
Member

MattSturgeon commented May 26, 2025

The infinite recursion here seems to be coming from setting config.lib.stylix, and it may be because setting it is based on an option in config.lib.stylix (colors). I'm not sure what could be done here.

It's a fundamental issue of having the config.lib.stylix.i3.bar, I'll drop i3 from this pr and look into fixing it in a separate pr.

 configElements = [
  (
    { fonts }:
    {
      xsession.windowManager.i3.config.fonts = {
        names = [ fonts.sansSerif.name ];
        size = fonts.size.desktop * 1.0;
      };
      lib.stylix.i3.bar.fonts = {
        names = [ fonts.sansSerif.name ];
        size = fonts.size.desktop * 1.0;
      };
    }
  )
]

I believe what makes this inf-rec (and why it wasn't before) is the fact that whether or not lib.stylix.i3.bar.fonts.{name,size} are defined is now predicated on whether lib.stylix.fonts is non-null.

Nix should be able to support that kinda recursion, because lib.stylix.i3 should be able to depend on lib.stylix.fonts lazily, however sometimes module-system types introduce additional strictness (i.e., the opposite of laziness).

In this case config.lib is type lib.types.attrs, which like lib.types.attrsOf may be strict. IIRC this is the reason lib.types.lazyAttrsOf exists, however that has its own pitfalls.


Caveat: this is just based on reading the diff and your comments, so I may be slightly off.

Also: sorry I wasn't around to help identify the issue! Well done figuring it out.

@0xda157 0xda157 force-pushed the treewide-mkTarget-3 branch from 55edd69 to 67a3599 Compare May 27, 2025 23:11
Copy link
Copy Markdown
Member

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

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

The infinite recursion here seems to be coming from setting config.lib.stylix, and it may be because setting it is based on an option in config.lib.stylix (colors). I'm not sure what could be done here.

It's a fundamental issue of having the config.lib.stylix.i3.bar, I'll drop i3 from this pr and look into fixing it in a separate pr.

 configElements = [
  (
    { fonts }:
    {
      xsession.windowManager.i3.config.fonts = {
        names = [ fonts.sansSerif.name ];
        size = fonts.size.desktop * 1.0;
      };
      lib.stylix.i3.bar.fonts = {
        names = [ fonts.sansSerif.name ];
        size = fonts.size.desktop * 1.0;
      };
    }
  )
]

I believe what makes this inf-rec (and why it wasn't before) is the fact that whether or not lib.stylix.i3.bar.fonts.{name,size} are defined is now predicated on whether lib.stylix.fonts is non-null.

Nix should be able to support that kinda recursion, because lib.stylix.i3 should be able to depend on lib.stylix.fonts lazily, however sometimes module-system types introduce additional strictness (i.e., the opposite of laziness).

In this case config.lib is type lib.types.attrs, which like lib.types.attrsOf may be strict. IIRC this is the reason lib.types.lazyAttrsOf exists, however that has its own pitfalls.

Thanks for the great explanations. As always.

There have been brief discussions in the past of migrating these interfaces into readOnly options to prevent them from being undocumented outside of the brief source code comment and because it makes the code nicer.

For reference, here are the relevant rg '# Merge this with' matches:

It is possible that this pattern is used elsewhere without this comment.

@0xda157 0xda157 force-pushed the treewide-mkTarget-3 branch 3 times, most recently from ba19cdd to e536830 Compare May 31, 2025 18:09
Copy link
Copy Markdown
Member

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

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

LGTM, except for one detail.

@0xda157 0xda157 force-pushed the treewide-mkTarget-3 branch from e536830 to a492a7f Compare June 2, 2025 17:15
Co-authored-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
@0xda157 0xda157 force-pushed the treewide-mkTarget-3 branch from ed4ed79 to 1117e86 Compare June 2, 2025 19:11
Copy link
Copy Markdown
Member

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

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

LGTM.

@trueNAHO trueNAHO enabled auto-merge (squash) June 3, 2025 13:57
@trueNAHO trueNAHO merged commit aa5e3c0 into nix-community:master Jun 3, 2025
5 checks passed
stylix-automation bot pushed a commit that referenced this pull request Jun 3, 2025
Link: #1371

Reviewed-by: pancho horrillo <pancho@pancho.name>
Reviewed-by: Matt Sturgeon <matt@sturgeon.me.uk>
Co-authored-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
(cherry picked from commit aa5e3c0)
@stylix-automation
Copy link
Copy Markdown
Contributor

Successfully created backport PR for release-25.05:

trueNAHO added a commit that referenced this pull request Jun 3, 2025
Link: #1371

Reviewed-by: pancho horrillo <pancho@pancho.name>
Reviewed-by: Matt Sturgeon <matt@sturgeon.me.uk>
Co-authored-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
(cherry picked from commit aa5e3c0)
@panchoh panchoh mentioned this pull request Jun 3, 2025
5 tasks
@0xda157 0xda157 deleted the treewide-mkTarget-3 branch June 3, 2025 16:41
0xda157 pushed a commit that referenced this pull request Jul 6, 2025
Link: #1609
Fixes: aa5e3c0 ("treewide: use mkTarget (batch 3) (#1371)")

Reviewed-by: awwpotato <awwpotato@voidq.com>
stylix-automation bot pushed a commit that referenced this pull request Jul 6, 2025
Link: #1609
Fixes: aa5e3c0 ("treewide: use mkTarget (batch 3) (#1371)")

Reviewed-by: awwpotato <awwpotato@voidq.com>
(cherry picked from commit 5cd1e4f)
0xda157 pushed a commit that referenced this pull request Jul 6, 2025
Link: #1609
Fixes: aa5e3c0 ("treewide: use mkTarget (batch 3) (#1371)")

Reviewed-by: awwpotato <awwpotato@voidq.com>
(cherry picked from commit 5cd1e4f)
trueNAHO pushed a commit that referenced this pull request Jul 11, 2025
Fixes: aa5e3c0 ("treewide: use mkTarget (batch 3) (#1371)")
Link: #1670

Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
stylix-automation bot pushed a commit that referenced this pull request Jul 11, 2025
Fixes: aa5e3c0 ("treewide: use mkTarget (batch 3) (#1371)")
Link: #1670

Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
(cherry picked from commit 0150050)
trueNAHO pushed a commit that referenced this pull request Jul 11, 2025
Fixes: aa5e3c0 ("treewide: use mkTarget (batch 3) (#1371)")
Link: #1670

Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
(cherry picked from commit 0150050)
LunaCOLON3 pushed a commit to LunaCOLON3/stylix that referenced this pull request Jul 14, 2025
Link: nix-community#1609
Fixes: aa5e3c0 ("treewide: use mkTarget (batch 3) (nix-community#1371)")

Reviewed-by: awwpotato <awwpotato@voidq.com>
LunaCOLON3 pushed a commit to LunaCOLON3/stylix that referenced this pull request Jul 14, 2025
)

Fixes: aa5e3c0 ("treewide: use mkTarget (batch 3) (nix-community#1371)")
Link: nix-community#1670

Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: home-manager Home Manager target topic: nixos NixOS target topic: overlay Overlay changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants