Clean up the Nix flake, recommended practices, provide overlay by spikespaz · Pull Request #6 · daylinmorgan/monolisa-nerdfont-patch

spikespaz · 2024-02-02T07:18:26Z

This is how I organize my Nix flakes and allow for the utmost compatibility for any desired use-case. When I say "recommended practices", they are my own, but also very similar to the "best practices" purported by the community.

I recommend reviewing each commit individually.

daylinmorgan · 2024-02-02T20:39:26Z

I recommend reviewing each commit individually.

Some of these changes aren't really independent, but I'll try.

Here are some high-level thoughts:

These changes broke nix flake check and nix flake show

❯ nix flake check
error:
       … while checking flake output 'overlays'

         at /nix/store/pps7hwqysf2l449krqg70yx3z2gslhbj-source/flake.nix:20:7:

           19|     in {
           20|       overlays = {
             |       ^
           21|         default = pkgs: pkgs0: {

       … while checking the overlay 'overlays.default'

         at /nix/store/pps7hwqysf2l449krqg70yx3z2gslhbj-source/flake.nix:21:9:

           20|       overlays = {
           21|         default = pkgs: pkgs0: {
             |         ^
           22|           monolisa-nerdfont-patch = pkgs.stdenv.mkDerivation {

       error: overlay does not take an argument named 'final'
❯ nix flake show
git+file:///home/daylin/stuff/fonts/monolisa-nerdfont-patch?ref=refs/heads/spikespaz/main&rev=fa54af9cba60c7ff2e2928a5dbe95800f4bc57dd
├───devShells
│   ├───aarch64-linux
│   │   └───default omitted (use '--all-systems' to show)
│   └───x86_64-linux
│       └───default: development environment 'nix-shell'
├───formatter
│   ├───aarch64-linux omitted (use '--all-systems' to show)
error:
       … while evaluating the attribute 'packages.x86_64-linux.default'

         at /nix/store/96dn6crb2c2l9mwv64a0yq7rnwbf9v6h-source/flake.nix:73:11:

           72|         packages = rec {
           73|           default = nixfmt;
             |           ^
           74|           nixfmt = pkgs.haskellPackages.nixfmt;

       … while evaluating the attribute 'haskellPackages.nixfmt'

         at /nix/store/96dn6crb2c2l9mwv64a0yq7rnwbf9v6h-source/flake.nix:31:15:

           30|             packageOverrides = self: super: {
           31|               nixfmt = self.callCabal2nix "nixfmt" src { };
             |               ^
           32|             };

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

       error: cannot build '/nix/store/qqkh6wvkk8y3ansvbcxxq2ysgdnh07dy-cabal2nix-nixfmt.drv^out' during evaluation because the option 'allow-import-from-derivation' is disabled

With regards to adding nixfmt. Seems unnecessary to me to add a formatter for a single (< 100LOC) .nix file that probably won't even change much after this (until nix breaks something). But, also as a flake input it adds OVER 60 additional inputs for little benefit! (FWIW I usually use alejandra).

Using nix-systems is fine with me, but the community consensus seems quite unsettled on that being the appropriate solution to a perceived problem with defining outputs.

daylinmorgan

I'd prefer to keep a forAllSystems function that that takes pkgs something like:

forAllSystems = f:
 genAttrs (import systems)
  (system:
     f (import nixpkgs {
       localSystem.system = system;
       overlays = [ self.overlays.default ];
     }));

Removing the formatter output removes the standalone use for the eachSystem function and then negates the redundant let pkgs = pkgsFor.${system} everywhere else.

flake.nix

spikespaz · 2024-02-10T19:10:26Z

The problem with Alejandra is that it causes very noisy diffs, which is a pain I contend with every time I rebase a PR. Bear with me.

spikespaz · 2024-02-10T19:23:30Z

Take this one for example. It messes with indentation so much. I really don't like Alejandra.

spikespaz · 2024-02-10T19:25:05Z

And this one, the diff was much nicer before.

spikespaz · 2024-02-10T19:27:09Z

Some of these changes aren't really independent, but I'll try.

The commits are as they are because I have experience with people telling me no to Nixfmt, and this makes the rebase easier when I inevitably replace it with Alejandra. :)

spikespaz · 2024-02-10T19:35:36Z

If you don't already know, Alejandra makes manual merges painful.

spikespaz · 2024-02-10T19:44:28Z

That should be all the changes you asked for done.

daylinmorgan · 2024-02-10T20:36:12Z

Thanks!

spikespaz · 2024-03-27T13:31:11Z

flake.nix

          src = ./.;
-          nativeBuildInputs = with final; [makeWrapper];
-          buildInputs = with final; [fontforge python3];
+          nativeBuildInputs = with pkgs; [makeWrapper];


I just noticed, this change is bad, don't ever use an external pkgs from inside an overlay. You should treat final as if it were pkgs, otherwise you evaluate Nixpkgs stages twice. See here for some relevant discussion.

Never mind the above comment. I now see line 26. If this is your preference (and you should be consistent with your change on lines 16-22) consider using the naming scheme that I have come up with in the thread I have already linked. It does make sense to rename final to pkgs, but as final/self and prev/super can also be used for overrideAttrs (and others) inside overlays, to avoid shadowing, consider pkgs: pkgs0.

consider using the naming scheme that I have come up with in the thread I have already linked. It does make sense to rename final to pkgs, but as final/self and prev/super can also be used for overrideAttrs (and others) inside overlays, to avoid shadowing, consider pkgs: pkgs0.

For one they aren't used within this overlay so avoiding name confusion is a moot point. And secondly, your scheme is incompatible with nix flake check as I previously demonstrated when you first submitted a PR with this change.

No, nixfmt caused the check failure. Not this.

spikespaz · 2024-03-27T13:41:02Z

flake.nix

          postFixup = ''
            wrapProgram $out/bin/monolisa-nerdfont-patch \
-              --set PATH ${lib.makeBinPath (with final; [fontforge])}
+              --set PATH ${makeBinPath (with final; [fontforge])}


This is subjective, and I understand that it is preference, but parts of Nixpkgs are moving away from this, which is why I changed it. The idea is to keep it clear what is in lib and what is in the prelude. You may safely ignore this comment, especially since I can't find the relevant discussion that I read and I can provide no evidence other than anecdotal.

Perhaps the issue I am thinking of in Nixpkgs is the over-usage of with which causes name-clashing. I don't remember well.

spikespaz · 2024-03-27T14:15:16Z

flake.nix

    devShells = eachSystem (_: pkgs: {
      default = pkgs.mkShell {
-        packages = with pkgs; [fontforge python3 pre-commit];
+        buildInputs = with pkgs; [fontforge python3 pre-commit];


Question, not a suggestion or correction: why did you choose to use buildInputs over packages?

Now a little pushy: mkShell is a smart function, is documented, and it does not suggest using buildInputs (though this is common practice). See the manual.

It make no difference they are all just merged.

spikespaz · 2024-03-27T14:22:50Z

flake.nix

    });
-
-    formatter = eachSystem (system: _: alejandra.packages.${system}.default);
  };


I understand not wanting to take the formatter's flake as an input, however consider re-introducing the formatter output because it is special. You can use the package from Nixpkgs instead. The output allows nix fmt to work. This is useful: I manually run nix fmt to keep myself organized while I am working on dirty code, and additionally, configure my editor to format the code using the nix fmt command when the file is saved. This allows me to write long lines, and then semi-automatically format to keep my head clear and the code readable.

I am well aware of how nix fmt works.

spikespaz · 2024-03-27T14:31:13Z

I have added comments, as I am sure your inbox is well-aware. I probably spammed you while struggling with the GitHub UI. It seemed to post them in the wrong locations, and when I went to correct them, refreshing the page showed that I had double-posted, edited, or even had it correct in the first place. I have absolutely no idea what was happening; I do sincerely apologize.

flake: update description

c85d8b9

daylinmorgan approved these changes Feb 2, 2024

View reviewed changes

daylinmorgan requested changes Feb 2, 2024

View reviewed changes

daylinmorgan approved these changes Feb 2, 2024

View reviewed changes

daylinmorgan reviewed Feb 9, 2024

View reviewed changes

flake.nix Outdated Show resolved Hide resolved

flake.nix Outdated Show resolved Hide resolved

spikespaz added 3 commits February 10, 2024 12:17

flake: add alejandra as formatter

4e9e3ab

flake: format nix code

3dbfeee

flake: make systems overrideable

f5aef0b

spikespaz force-pushed the main branch from fa54af9 to b1893b2 Compare February 10, 2024 19:20

spikespaz added 3 commits February 10, 2024 12:31

flake: replace forAllSystems with eachSystem

35b4ac8

flake: inherit lib at top level

fabe645

flake: move usages of with to smallest scopes

9fb310f

spikespaz added 3 commits February 10, 2024 12:37

flake: introduce overlay

8841ba2

flake: package: remove needless unpackPase override

29aef1d

flake: devShell: replace buildInputs with packages

5b9e5fa

spikespaz force-pushed the main branch from b1893b2 to 5b9e5fa Compare February 10, 2024 19:38

spikespaz requested a review from daylinmorgan February 10, 2024 19:51

daylinmorgan added 4 commits February 10, 2024 14:20

chore: move alejandra to pre-commit-hook

9c8d7f6

chore: inherit from nixpkgs.lib not nixpkgs

589b98c

chore: remove some lets add others

4ae5988

chore: change mkShell back to buildInputs

8a95f2c

daylinmorgan merged commit 80c347e into daylinmorgan:main Feb 10, 2024

spikespaz commented Mar 27, 2024

View reviewed changes

Conversation

spikespaz commented Feb 2, 2024

Uh oh!

daylinmorgan commented Feb 2, 2024

Uh oh!

daylinmorgan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

spikespaz commented Feb 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spikespaz commented Feb 10, 2024

Uh oh!

spikespaz commented Feb 10, 2024

Uh oh!

spikespaz commented Feb 10, 2024

Uh oh!

spikespaz commented Feb 10, 2024

Uh oh!

spikespaz commented Feb 10, 2024

Uh oh!

daylinmorgan commented Feb 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spikespaz Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spikespaz commented Mar 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

daylinmorgan left a comment •

edited

Loading

spikespaz commented Feb 10, 2024 •

edited

Loading

spikespaz Mar 27, 2024 •

edited

Loading