stylix/mk-target: polish, simplify, and extend interface#1721
Conversation
stylix-automation
bot
added
topic: documentation
trueNAHO
force-pushed
the
stylix-mk-target-polish-simplify-and-extend-interface
branch
2 times, most recently
from
b311b35 to
dd5389a
Compare
trueNAHO
force-pushed
the
stylix-mk-target-polish-simplify-and-extend-interface
branch
from
dd5389a to
eeac205
Compare
Flameopathic
left a comment
Flameopathic
left a comment
There was a problem hiding this comment.
Implementation looks good. I disagree with renaming configElements and extraOptions because a more transparent interface with the module system hides the subtle and important differences in our interface.
trueNAHO
force-pushed
the
stylix-mk-target-polish-simplify-and-extend-interface
branch
2 times, most recently
from
90ca0a9 to
c0e8cde
Compare
The differences are major and crucial. Unknowingly treating |
|
personally I like
personally I would prefer moving the documentation and renaming changes into separate pr(s) and have this pr be focused on functionality to reduce the number of diffs to review. |
trueNAHO
force-pushed
the
stylix-mk-target-polish-simplify-and-extend-interface
branch
from
c0e8cde to
c9a0103
Compare
Agreed
I'm cautious of using overloaded names like
I agree, all PRs should strive to be as small and reviewable as possible; if sections of a PR can be extracted out into separate PRs to merge earlier or later, then that is often a good idea. |
If a more transparent interface with the underlying Nixpkgs module system does not cause confusion, the transparent interface would be an improvement. I suggest optimistically applying
Initially, I submitted
I superseded #1709 with this patchset because most patches depend on previous patches. Submitting this patchset as a sequence of PRs would synchronize discussions instead of asynchronously discussing the whole patchset. Unless further insisted, I would prefer this patchset to be reviewed as a whole, especially since I believe it is ready for merge since the added core functionality seems to be working (see the changelog notes from v3). To review the patchset, consider taking inspiration from #1212 (review), which itself was inspired by the Linux Kernel review process. Feel free to clone my branch and use all your local tooling to review. Alternatively, the following GitHub endpoints should cover most use cases:
To get a high-level overview of the generated module options from nix run github:trueNAHO/stylix/stylix-mk-target-polish-simplify-and-extend-interface#doc |
stylix/mk-target.nix
Outdated
| Settings merged with ${config}. Attribute sets are | ||
| recursively merged with ${config}, while all other | ||
| non-`null` types override ${config}. |
There was a problem hiding this comment.
"Setting merged with config" this only applies to attrsets, I would just remove this part. also lib.stylix.colors is an internal impl detail that might be confusing to users here.
There was a problem hiding this comment.
"Setting merged with config" this only applies to attrsets, I would just remove this part.
Good point. This is resolved in v7.
lib.stylix.colorsis an internal impl detail that might be confusing to users here.
AFAIK, people frequently use lib.stylix.colors, and exposing it here would partially resolve #208. It is only partially resolved because the mkTarget migration is incomplete.
trueNAHO
force-pushed
the
stylix-mk-target-polish-simplify-and-extend-interface
branch
from
c9a0103 to
4628b14
Compare
trueNAHO
force-pushed
the
stylix-mk-target-polish-simplify-and-extend-interface
branch
from
aa02968 to
0bfa746
Compare
0xda157
left a comment
0xda157
left a comment
There was a problem hiding this comment.
code LGTM, my config builds when overriding stylix with this branch
Tested-by: 0xda157 <da157@voidq.com>
FYI and as a reminder, the changelog of v3 includes an in-tree patch for testing the target level overriding:
The generated options can also conveniently be inspected with:
As mentioned in the PR description, this PR should be up-to-date with the master branch: Important This patchset should be Upon merging this PR, new PRs should be rebased on top of this PR to avoid unexpected evaluation errors. |
trueNAHO
force-pushed
the
stylix-mk-target-polish-simplify-and-extend-interface
branch
from
0bfa746 to
1bc50a2
Compare
Fixes: 093087e ("stylix: add imports to mkTarget (nix-community#1363)")
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)")
Polish the mkTarget implementation to improve error reporting and simplify future enhancements. Configuration elements can now recursively resolve to paths.
Rename the generalConfig argument to unconditionalConfig to better imply the intentional lack of safeguarding.
Rename mkTarget's 'configElements' argument to 'config' and 'extraOptions' to 'options' to provide a more transparent interface with the underlying Nixpkgs module system.
Optionalize mkTarget's 'humanName' and 'name' arguments by inferring 'humanName' from the 'name' attribute in the /modules/<MODULE>/meta.nix file, and 'name' from the /modules/<NAME>/ directory name. Inferring the 'humanName' and 'name' arguments ensures consistency and reduces boilerplate. The 'humanName' and 'name' arguments are optionalized instead of removed because complex modules generating target derivations need to distinguish between them. Closes: nix-community#1661
Generate targets.${target}.${argument}.enable and
targets.${target}.${argument}.override options for disabling and
configuring safeguarded arguments on a target level.
Normalize the options argument identically to config to provide a coherent and extensible options interface.
trueNAHO
force-pushed
the
stylix-mk-target-polish-simplify-and-extend-interface
branch
from
1bc50a2 to
6153df3
Compare
trueNAHO
added
backport: release-25.11
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-25.11
git worktree add -d .worktree/backport-1721-to-release-25.11 origin/release-25.11
cd .worktree/backport-1721-to-release-25.11
git switch --create backport-1721-to-release-25.11
git cherry-pick -x a4ffbc20eabd1dd14ae43298ec58dd1c4bf1e874 25354cc88b67bede74dbf314949821889ae15e48 9afd8230cd8c92c0450f9f9ece65c05062160fc1 1272e6858e29205b8b03e696f121e6eedcb8b3b5 f7b554dea904393e483df9c13f0b6d6286d53ba7 76d05fd9c068d63f0dd747d40bcb8a4a57720daa 16df6b8448a856532b66ef8f7e012d29390d070b dfc859f54d937a31b954901ed01f3d6961cd2ead 953c3fb01e2a2cbfef092309852ba19d5ae6ee34 6153df31ce4f68f5709755994cc49c48d737e98c |
4 participants
Changelog
v11: 6153df3
v10: 1bc50a2
v9: 0bfa746
stylix/mk-target: rename mkConfig function to callModuletreewide: rename mkTarget's configElements and extraOptions optionsmodules: flatten single-attribute set declarations.stylix/mk-target: generate options for configuring safeguarded argumentsv8: aa02968
stylix/mk-target: generate options for configuring safeguarded argumentsconfig.stylix.targets.${name}.${argument}.overridetype checking, as suggested in stylix/mk-target: polish, simplify, and extend interface #1721 (comment).v7: 4628b14
stylix/mk-target: generate options for configuring safeguarded argumentstargets.${target}.${argument}.settingstotargets.${target}.${argument}.override, as suggested in stylix/mk-target: polish, simplify, and extend interface #1721 (comment).Settings merged with ${config}.documentation sentence, as suggested in stylix/mk-target: polish, simplify, and extend interface #1721 (comment).v6: c9a0103
v5: c0e8cde
stylix/mk-target: generate options for configuring safeguarded argumentsimportsandoptions.v4: 90ca0a9
stylix/mk-target: generate options for configuring safeguarded argumentstargets.${target}.${argument}.settingsoptions formkTargetoptions, resolving the regression introduced in v3.areArgumentsEnabled, following the existinglib.mkIfconvention.targets.${target}.enableandtargets.${target}.settingstotargets.${target}.${argument}.enabletargets.${target}.${argument}.settingsin the commit message.stylix/mk-target: normalize options argument identically to configv3: eeac205
stylix/mk-target: generate options for disabling safeguarded argumentsExtend functionality to generate the
targets.${target}.enableandtargets.${target}.settingsoptions instead of only thetargets.${target}option.Rename commit to
stylix/mk-target: generate options for configuring safeguarded arguments.Tested with
nix run .#testbed:kitty:darkafter applying the following debug patch:v2: dd5389a
stylix/mk-target: generate options for disabling safeguarded argumentsstylix.targets.${target}.cfgexclusion regression introduced in v1.lib.mkEnableOptiondescription.v1: b311b35
stylix/mk-target: sort argumentsstylix/mk-target: sort optional arguments.stylix/mk-target: polish implementation and improve error reportingstylix/mk-target: generate options for disabling safeguarded argumentsstylix.targets.${target}.cfgoptions.lib.mkEnableOptionTODO marker by generating a different message forcolorsarguments.v0: 176d421
Note
It is strongly encouraged to review and discuss commits individually instead of the total diff.
Important
This patchset should be
git rebase masterbefore being merged to avoid uncaught internal breaking changes.CC: @Flameopathic @MattSturgeon @awwpotato @danth
Submission Checklist
Notify Maintainers