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

treewide: add stylix.enable option #244

Merged
merged 12 commits into from
Jun 10, 2024
Merged

Conversation

jalil-salame
Copy link
Contributor

@jalil-salame jalil-salame commented Feb 2, 2024

About

Add a stylix.enable option to enable or disable all Stylix modules in order to resolve issues similar to #216.

To align with the default lib.mkEnableOption behavior, stylix.enable defaults to false.

Breaking Change

Stylix is disabled by default. To enable it, use:

stylix.enable = true;

Closes

Steps

Unresolved Questions

@jalil-salame jalil-salame marked this pull request as draft February 2, 2024 16:59
@trueNAHO trueNAHO changed the title Add enable option (fixes #216) stylix: add stylix.enable option Feb 10, 2024
stylix/target.nix Outdated Show resolved Hide resolved
@jalil-salame jalil-salame marked this pull request as ready for review May 5, 2024 11:19
@jalil-salame
Copy link
Contributor Author

I don't have the bandwidth to test this PR, seeing that it has been stale for ~3 months I'll mark it as ready, and if someone tests it and finds some problems I'll happily fix them.

@trueNAHO trueNAHO requested a review from danth May 5, 2024 12:50
@jalil-salame
Copy link
Contributor Author

Ci is failing, I think we need to set sylix.enable = true in CI.

@trueNAHO
Copy link
Collaborator

trueNAHO commented May 5, 2024

Ci is failing, I think we need to set sylix.enable = true in CI.

CI is successful now: https://github.com/danth/stylix/actions/runs/8958380642.

I tested stylix.enable = false; and stylix.enable = true; in my Home Manger configuration, and so far everything works as expected.

@jalil-salame, could you maybe double check this PR on your setup?

@jalil-salame
Copy link
Contributor Author

My setup relies on #201 so it would be a mess to use this PR sadly.

@jalil-salame
Copy link
Contributor Author

I was nerd sniped into trying it out anyways, both true and false work (with some minor modifications). By work I mean compile, my styling relies on #201 so it is broken in this PR.

stylix/nixos/default.nix Outdated Show resolved Hide resolved
@trueNAHO
Copy link
Collaborator

I tested stylix.enable = false; and stylix.enable = true; in my Home Manger configuration, and so far everything works as expected.

@trueNAHO trueNAHO requested a review from danth May 13, 2024 09:05
@jalil-salame
Copy link
Contributor Author

I'm adding this PR to my config (here if you are curious), so I'll be able to test the Linux modules a bit more thoroughly soon c:

Copy link
Collaborator

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

Considering that stylix.enable defaults to false, why does f957c28 cause the following error:

$ nix build .#docs
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'stylix-book'
         whose name attribute is located at /nix/store/3lllcglr2ip90mkxs5q4asl98r305mia-source/pkgs/stdenv/generic/make-derivation.nix:331:7

       … while evaluating attribute 'patchPhase' of derivation 'stylix-book'

         at /nix/store/blw4ppvzwz08gw7dxbc41pxv83qgr6pi-source/docs/default.nix:44:3:

           43|
           44|   patchPhase = ''
             |   ^
           45|     cp ${../README.md} src/README.md

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: A definition for option `stylix.image' is not of type `path or package convertible to it'. Definition values:
       - In `/nix/store/3lllcglr2ip90mkxs5q4asl98r305mia-source/flake.nix': null

Ideally, it should be possible to revert e8e3304.

@trueNAHO
Copy link
Collaborator

trueNAHO commented May 22, 2024

For reference, I would use the following commit message when merging this PR:

stylix: add 'stylix.enable' option

Add a 'stylix.enable' option to enable or disable all Stylix modules in
order to resolve issues similar to [2].

To align with the default 'lib.mkEnableOption' [1] behavior,
'stylix.enable' defaults to 'false'.

BREAKING CHANGE: Stylix is disabled by default. To enable it, use:

    stylix.enable = true;

[1]: https://github.com/NixOS/nixpkgs/blob/23.11/lib/options.nix#L91-L105
[2]: https://github.com/danth/stylix/issues/216

@jalil-salame
Copy link
Contributor Author

Considering that stylix.enable defaults to false, why does f957c28 cause the following error:

$ nix build .#docs
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'stylix-book'
         whose name attribute is located at /nix/store/3lllcglr2ip90mkxs5q4asl98r305mia-source/pkgs/stdenv/generic/make-derivation.nix:331:7

       … while evaluating attribute 'patchPhase' of derivation 'stylix-book'

         at /nix/store/blw4ppvzwz08gw7dxbc41pxv83qgr6pi-source/docs/default.nix:44:3:

           43|
           44|   patchPhase = ''
             |   ^
           45|     cp ${../README.md} src/README.md

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: A definition for option `stylix.image' is not of type `path or package convertible to it'. Definition values:
       - In `/nix/store/3lllcglr2ip90mkxs5q4asl98r305mia-source/flake.nix': null

Ideally, it should be possible to revert e8e3304.

I'd need to look at the code carefully, but if a lib.mkIf removes imports or options then you'd expect that behavior.

@trueNAHO
Copy link
Collaborator

trueNAHO commented May 24, 2024

Considering that stylix.enable defaults to false, why does f957c28 cause the following error:

$ nix build .#docs
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'stylix-book'
         whose name attribute is located at /nix/store/3lllcglr2ip90mkxs5q4asl98r305mia-source/pkgs/stdenv/generic/make-derivation.nix:331:7

       … while evaluating attribute 'patchPhase' of derivation 'stylix-book'

         at /nix/store/blw4ppvzwz08gw7dxbc41pxv83qgr6pi-source/docs/default.nix:44:3:

           43|
           44|   patchPhase = ''
             |   ^
           45|     cp ${../README.md} src/README.md

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: A definition for option `stylix.image' is not of type `path or package convertible to it'. Definition values:
       - In `/nix/store/3lllcglr2ip90mkxs5q4asl98r305mia-source/flake.nix': null

Ideally, it should be possible to revert e8e3304.

I'd need to look at the code carefully, but if a lib.mkIf removes imports or options then you'd expect that behavior.

Considering this merely cleans up the code, this should probably be addressed in a followup-PR.

NOTE: I added a list of things that should be verified right before merging, to the first PR message.

@danth, could you review this PR?

Copy link
Collaborator

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

I rebased this PR on top of master and added an additional breaking change note to the top-level README.

I tested stylix.enable = false; and stylix.enable = true; in my Home Manger configuration once again, and so far everything works as expected.

Could someone test this in a NixOS setup?

@jalil-salame
Copy link
Contributor Author

jalil-salame commented May 26, 2024

I am testing in in my configuration, but setting stylix.enable = false; on my nixos configuration does nothing, the styling is still applied.

Edit: I had autoEnable = true set. The behavior of autoEnable is similar to enable, we might want to consider changing this.

@jalil-salame
Copy link
Contributor Author

Works on my configuration now that I fixed the autoEnable thing c:

@danth
Copy link
Owner

danth commented Jun 9, 2024

I managed to do the first part with some sed magic, just need to go through and fix the second issue by hand now.

Copy link
Owner

@danth danth left a comment

Choose a reason for hiding this comment

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

I think this is ready to merge.

@danth danth changed the title stylix: add stylix.enable option treewide: add stylix.enable option Jun 10, 2024
@danth danth enabled auto-merge (squash) June 10, 2024 00:57
@danth danth requested a review from trueNAHO June 10, 2024 00:57
@danth danth mentioned this pull request Jun 10, 2024
@danth
Copy link
Owner

danth commented Jun 10, 2024

Can docs/settings.nix be removed due to the new stylix.enable option?

Not yet: although the option is not required any more, the default value for stylix.image is still of an incorrect type, so it needs to be overridden. This will be fixed in #407, after which it should be possible to remove the file.

@trueNAHO trueNAHO disabled auto-merge June 10, 2024 08:20
@trueNAHO
Copy link
Collaborator

Is the following commit message good:

stylix: add 'stylix.enable' option

Add a 'stylix.enable' option to enable or disable all Stylix modules in
order to resolve issues similar to [2].

To align with the default 'lib.mkEnableOption' [1] behavior,
'stylix.enable' defaults to 'false'.

BREAKING CHANGE: Stylix is disabled by default. To enable it, use:

    stylix.enable = true;

[1]: https://github.com/NixOS/nixpkgs/blob/23.11/lib/options.nix#L91-L105
[2]: https://github.com/danth/stylix/issues/216

Co-authored-by: Daniel Thwaites <[email protected]>
Co-authored-by: Jalil David Salamé Messina <[email protected]>
Co-authored-by: NAHO <[email protected]>

@danth
Copy link
Owner

danth commented Jun 10, 2024

Yep, I had already set that for auto-merge, just using ` rather than '

@trueNAHO trueNAHO merged commit 7682713 into danth:master Jun 10, 2024
9 checks passed
@jalil-salame jalil-salame deleted the enable-option branch June 10, 2024 09:55
trueNAHO added a commit to trueNAHO/dotfiles that referenced this pull request Jun 11, 2024
Update the 'sytlix' input, use its new 'stylix.enable' option, and
conditionally set the 'stylix.image' and 'programs.mpv.scriptOpts.uosc'
options based on 'stylix.enable'.

Addresses:

- 25e5229
- 30231a3
- danth/stylix#244
jalil-salame added a commit to jalil-salame/configuration.nix that referenced this pull request Jun 14, 2024
[stylix/#244][1] has been merged so I can finally stop using my fork
(also I deleted that branch so it broke :p).

[1]: <danth/stylix#244>
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.

3 participants