Skip to content

stylix: restrict access to config while using mkTarget#1368

Merged
trueNAHO merged 2 commits intonix-community:masterfrom
0xda157:mkTarget-safe-config
Jul 4, 2025
Merged

stylix: restrict access to config while using mkTarget#1368
trueNAHO merged 2 commits intonix-community:masterfrom
0xda157:mkTarget-safe-config

Conversation

@0xda157
Copy link
Copy Markdown
Contributor

@0xda157 0xda157 commented May 23, 2025

allows for accessing config once config has been removed from module args (if that ends up happening). Also allows for removal of the review burden of ensuring config.stylix or config.stylix.lib.colors isn't accessed when using config.

cc @trueNAHO @MattSturgeon @Flameopathic

Things done

Notify maintainers

@0xda157 0xda157 force-pushed the mkTarget-safe-config branch 3 times, most recently from 587d0ad to a1e5024 Compare May 23, 2025 16:03
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.

allows for accessing config once config has been removed from module args (if that ends up happening). Also allows for removal of the review burden of ensuring config.stylix or config.stylix.lib.colors isn't accessed when using config.

IIUC, this is for the following additional safety:

-{ config, mkTarget, ... }:
+{ mkTarget, ... }:
 mkTarget {
   name = /* ... */;
   humanName = /* ... */;
- configElements = ({ colors }: { /* ... */ });
+ configElements = ({ config, colors }: { /* ... */ });
 }

Copy link
Copy Markdown
Member

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

IMO this feels like something that should be done in the file's top-level args (which are currently the module args), rather than configElement args.

Only being able to access config from within a configElement feels cumbersome; you may wish to also access it in option defaults, in shared/common let binding, etc.

But I don't see any issue with the implementation, if you want to stick with this approach for now.

@Flameopathic
Copy link
Copy Markdown
Contributor

IMO this feels like something that should be done in the file's top-level args (which are currently the module args), rather than configElement args.

Only being able to access config from within a configElement feels cumbersome; you may wish to also access it in option defaults, in shared/common let binding, etc.

Agreed. Though I can somewhat see the draw of wanting to eliminate the top-level args, I think that the visual improvement is very little in comparison to the increased difficulty for new contributors. Its abstractions are great for the future and people who understand it well, but the more mkTarget separates targets from the module system, the more difficult things will be to learn, understand, and debug.

@0xda157
Copy link
Copy Markdown
Contributor Author

0xda157 commented May 25, 2025

IMO this feels like something that should be done in the file's top-level args (which are currently the module args), rather than configElement args.

Only being able to access config from within a configElement feels cumbersome; you may wish to also access it in option defaults, in shared/common let binding, etc.

there are 3 options here

  1. access config through top-level args. I don't like this because we can get everything except colors, fonts, etc through top-level args, so below still applies.

Only being able to access config from within a configElement feels cumbersome; you may wish to also access it in option defaults, in shared/common let binding, etc.

  1. access config, colors, fonts, etc through top-level args. This breaks the mkIf != null thing.
  2. don't access config through top-level args

@Flameopathic
Copy link
Copy Markdown
Contributor

we can't get everything except colors, fonts, etc through top-level args, so below still applies.

What do you mean by this?

@MattSturgeon
Copy link
Copy Markdown
Member

MattSturgeon commented May 26, 2025

I don't like this because we can't get everything except colors, fonts, etc through top-level args, so below still applies

I don't think that's necessarily true. With some refactoring, mkTarget could apply top-level args to its input argument using the fnOrAttrs pattern.

This would work the same way as (e.g.) nixpkgs override functions that accept either attrs or a function. This is also what the module system checks internally when importing modules, since they are also fnOrAttrs values.


With the current design, usage would be a bit ugly, but this would improve once modules are no longer modules and are instead implicitly "args for mkTarget".

{ mkTarget, ... }:
mkTarget (
  { config, options, pkgs ... }:
  {
    name = "foo";
    humanName = "bar";
  }
)

@stylix-automation stylix-automation bot added the status: merge conflict Merge conflict label Jun 12, 2025
@0xda157 0xda157 force-pushed the mkTarget-safe-config branch from a1e5024 to afb9b35 Compare June 16, 2025 00:44
@0xda157 0xda157 removed the status: merge conflict Merge conflict label Jun 16, 2025
@0xda157 0xda157 force-pushed the mkTarget-safe-config branch from afb9b35 to df59edf Compare June 16, 2025 00:47
Copy link
Copy Markdown
Member

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

LGTM, but the commit/title should change to reflect that this isn't adding the ability to access config. Rather it is adding restrictions to the config that was already accessible.

Something like stylix: restrict access to config in mkTarget?

@0xda157 0xda157 force-pushed the mkTarget-safe-config branch from df59edf to c1569fa Compare June 16, 2025 00:55
@stylix-automation stylix-automation bot added the topic: home-manager Home Manager target label Jun 16, 2025
@0xda157 0xda157 force-pushed the mkTarget-safe-config branch 4 times, most recently from 70084f8 to 6d976e3 Compare June 16, 2025 01:00
@0xda157 0xda157 changed the title stylix: allow for safe config access in mkTarget stylix: restrict access to config while using mkTarget Jun 16, 2025
@0xda157 0xda157 mentioned this pull request Jun 16, 2025
5 tasks
@0xda157 0xda157 force-pushed the mkTarget-safe-config branch 2 times, most recently from 91fca8f to 3af67f7 Compare June 16, 2025 23:35
@0xda157 0xda157 mentioned this pull request Jun 16, 2025
5 tasks
@0xda157 0xda157 force-pushed the mkTarget-safe-config branch from 3af67f7 to 50170c7 Compare June 16, 2025 23:59
@stylix-automation stylix-automation bot added the status: merge conflict Merge conflict label Jun 18, 2025
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.

Code LGTM. Could you rebase to resolve the merge conflicts and double check that no config module accesses have been missed elsewhere in the tree?

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 25, 2025 19:38
@0xda157
Copy link
Copy Markdown
Contributor Author

0xda157 commented Jun 26, 2025

either need to manual merge or have @danth approve for the auto merge to work

@trueNAHO trueNAHO disabled auto-merge July 4, 2025 17:04
@trueNAHO trueNAHO merged commit dea0337 into nix-community:master Jul 4, 2025
5 checks passed
stylix-automation bot pushed a commit that referenced this pull request Jul 4, 2025
Link: #1368

Reviewed-by: Flameopathic <64027365+Flameopathic@users.noreply.github.com>
Reviewed-by: Matt Sturgeon <matt@sturgeon.me.uk>
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
(cherry picked from commit dea0337)
@stylix-automation
Copy link
Copy Markdown
Contributor

Successfully created backport PR for release-25.05:

@0xda157 0xda157 deleted the mkTarget-safe-config branch July 4, 2025 17:27
@0xda157 0xda157 mentioned this pull request Jul 4, 2025
5 tasks
trueNAHO pushed a commit that referenced this pull request Jul 4, 2025
Link: #1368

Reviewed-by: Flameopathic <64027365+Flameopathic@users.noreply.github.com>
Reviewed-by: Matt Sturgeon <matt@sturgeon.me.uk>
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
(cherry picked from commit dea0337)
0xda157 added a commit that referenced this pull request Jul 4, 2025
Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (#1368)")
Fixes: 713f8da ("rofi: use mkTarget (#1550)")
stylix-automation bot pushed a commit that referenced this pull request Jul 4, 2025
Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (#1368)")
Fixes: 713f8da ("rofi: use mkTarget (#1550)")
(cherry picked from commit 606944b)
0xda157 added a commit that referenced this pull request Jul 4, 2025
Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (#1368)")
Fixes: 713f8da ("rofi: use mkTarget (#1550)")
(cherry picked from commit 606944b)
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jul 6, 2025
Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (nix-community#1368)")
trueNAHO added a commit that referenced this pull request Jul 6, 2025
Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (#1368)")
Link: #1613

Reviewed-by: awwpotato <awwpotato@voidq.com>
stylix-automation bot pushed a commit that referenced this pull request Jul 6, 2025
Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (#1368)")
Link: #1613

Reviewed-by: awwpotato <awwpotato@voidq.com>
(cherry picked from commit a294d8b)
trueNAHO added a commit that referenced this pull request Jul 6, 2025
Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (#1368)")
Link: #1613

Reviewed-by: awwpotato <awwpotato@voidq.com>
(cherry picked from commit a294d8b)
LunaCOLON3 pushed a commit to LunaCOLON3/stylix that referenced this pull request Jul 14, 2025
…#1368)

Link: nix-community#1368

Reviewed-by: Flameopathic <64027365+Flameopathic@users.noreply.github.com>
Reviewed-by: Matt Sturgeon <matt@sturgeon.me.uk>
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
LunaCOLON3 pushed a commit to LunaCOLON3/stylix that referenced this pull request Jul 14, 2025
Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (nix-community#1368)")
Fixes: 713f8da ("rofi: use mkTarget (nix-community#1550)")
LunaCOLON3 pushed a commit to LunaCOLON3/stylix that referenced this pull request Jul 14, 2025
Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (nix-community#1368)")
Link: nix-community#1613

Reviewed-by: awwpotato <awwpotato@voidq.com>
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Nov 21, 2025
Rename the mkConfig function to callModule to generalize the name beyond
the specific config and options module arguments.

Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (nix-community#1368)")
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Nov 21, 2025
Rename the mkConfig function to callModule to generalize the name beyond
the specific config and options module arguments.

Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (nix-community#1368)")
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Dec 10, 2025
Rename the mkConfig function to callModule to generalize the name beyond
the specific config and options module arguments.

Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (nix-community#1368)")
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Dec 10, 2025
Rename the mkConfig function to callModule to generalize the name beyond
the specific config and options module arguments.

Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (nix-community#1368)")
(cherry picked from commit 1272e68)
0xda157 pushed a commit that referenced this pull request Dec 10, 2025
Rename the mkConfig function to callModule to generalize the name beyond
the specific config and options module arguments.

Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (#1368)")
(cherry picked from commit 1272e68)
rwxae pushed a commit to rwxae/stylix that referenced this pull request Jan 5, 2026
Rename the mkConfig function to callModule to generalize the name beyond
the specific config and options module arguments.

Fixes: dea0337 ("stylix: restrict access to config while using mkTarget (nix-community#1368)")
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants