treewide: add developer shell and pre-commit hooks and simplify GitHub workflows

[trueNAHO]

This patchset closes issue #236 ("treewide: add linters and formatters") after integrating pre-commit hooks into the developer shell and simplifying GitHub workflows using nix flake check.

This patchset is stacked on top of the pending patchset #515 ("treewide: re-add flake-utils dependency and interface flake systems") and should only be merged after it.

Note

These commits should be merged individually, not squashed, to ensure a clear and easily reversible changelog. In case of change requests, review the commits separately.

[trueNAHO]

Subject: [PATCH 04/17] stylix: add deadnix and statix pre-commit hooks

[...]

diff --git a/flake.nix b/flake.nix
index a407bb6..e4596c8 100644
--- a/flake.nix
+++ b/flake.nix
@@ -59,6 +59,16 @@
 
     nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable";
 
+    pre-commit-hooks = {
+      inputs = {
+        flake-compat.follows = "flake-compat";
+        nixpkgs-stable.follows = "pre-commit-hooks/nixpkgs";
+        nixpkgs.follows = "nixpkgs";
+      };
+
+      url = "github:cachix/pre-commit-hooks.nix";
+    };
+
     # Interface flake systems.
     systems.url = "github:nix-systems/default";
   };
@@ -70,6 +80,15 @@
         inherit (nixpkgs) lib;
         pkgs = nixpkgs.legacyPackages.${system};
       in {
+        checks.pre-commit-hooks = inputs.pre-commit-hooks.lib.${system}.run {
+          hooks = {
+            deadnix.enable = true;
+            statix.enable = true;
+          };
+
+          src = ./.;
+        };
+
         devShells.default = pkgs.mkShell {
           packages = [ inputs.home-manager.packages.${system}.default ];
         };

All pre-commit-hooks instances should be renamed to git-hooks, following cachix/git-hooks.nix#488.

[trueNAHO]

I will address #519 (comment) together with any potential change requests.

For reference:

I prefer squashing as the default since it keeps the history cleaner on the master branch

[...] squashing #519 into a single commit would result in a practically incomprehensible and revertible history.

-- #514 (comment)

[trueNAHO]

Subject: [PATCH 07/17] ci: simplify Docs GitHub workflow

Link: https://github.com/danth/stylix/pull/519
Link: https://github.com/trueNAHO/dotfiles/blob/390d6261e7cf6d19a119643da216258c772db881/.github/workflows/docs.yml
---
 .github/workflows/docs.yml | 61 +++++++++++++-------------------------
 1 file changed, 20 insertions(+), 41 deletions(-)

diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml
index 3c758152..4d1515e9 100644
--- a/.github/workflows/docs.yml
+++ b/.github/workflows/docs.yml
@@ -1,3 +1,4 @@
+---
 name: Docs

 on:
@@ -5,53 +6,31 @@ on:
     branches:
       - master

-jobs:
-  build:
-    name: Build
+permissions:
+  contents: read
+  id-token: write
+  pages: write

-    permissions:
-      contents: read
+concurrency:
+  cancel-in-progress: true
+  group: pages

+jobs:
+  docs:
     runs-on: ubuntu-22.04

Restrict the workflow permissions to the docs job:

jobs:
  docs:
    runs-on: ubuntu-22.04

    permissions:
      contents: read
      id-token: write
      pages: write

[trueNAHO]

I will address #519 (comment) together with any potential change requests.

For reference:

I prefer squashing as the default since it keeps the history cleaner on the master branch

[...] squashing #519 into a single commit would result in a practically incomprehensible and revertible history.

-- #514 (comment)

@danth, I don't think it would significantly impact your review, but should I rebase this PR on top of master and resolve the #519 (comment) and #519 (comment) nitpicks before you review this patchset?

[danth]

Feel free to go ahead and rebase the PR and make any other changes as well as addressing the comments in this review :)

Personally I would prefer nixfmt over Alejandra, since it's being developed into the official standard formatter. At a glance it seems pre-commit-hooks supports it.

[trueNAHO]

-      - name: Prepare docs for upload
-        run: cp -r --dereference --no-preserve=mode,ownership result/ public/

Unless the situation has changed since the original workflow was written, I think this copy was necessary due to an error with the files being part of the Nix store. (It was either because they were owned by root, or because there were symlinks involved, I can't remember exactly)

Good catch! Admittedly, I adapted this GitHub Action from my dotfiles setup without testing it. The only difference with my dotfiles setup is:

-path: result/usr/share/doc
+path: result

I assume the GitHub Action does not consider path pointing to a read-only symlink directory. In my dotfiles setup, I do not run into such problems because path points to a regular (not a read-only symlink directory) directory inside result. I use /usr/share/doc because it is a more conventional FHS location for documentation.

Should we apply your cp workaround or move the documentation output to result/usr/share/doc? NOTE: This is unaddressed in v1.

+To automatically enter the developer shell upon entering the project directory
+with [`direnv`](https://direnv.net), run:

For clarity we should mention that direnv needs to be installed beforehand

Alternatively, if you have [`direnv`](https://direnv.net) installed, you may
automatically enter the developer shell upon entering the project directory.
To enable this, run:

I assumed the direnv link made this obvious, but I would not mind adding the note. NOTE: I have not included this in v1.

+          packages = [
+            inputs.home-manager.packages.${system}.default
+            inputs.self.checks.${system}.pre-commit-hooks.enabledPackages
+            pkgs.ghc

I don't think GHC is necessary since the palette generator should be built using nix build .#palette-generator

The developer shell should include all necessary development packages. Since GHC is almost never used, I created a separate developer shell for it.

Personally I would prefer nixfmt over Alejandra, since it's being developed into the official standard formatter. At a glance it seems pre-commit-hooks supports it.

Despite its very early development, using the official formatter sound sreasonable due to Stylix's popularity.

Currently, harmonizing nixfmt (with the required treefmt-nix input) across nix fmt and pre-commit hooks is a nightmare and not worth implementing. The following patch demonstrates a "working" proof of concept without guaranteeing harmonization with nix fmt:

From 8c9597ea94795ee3061d6efb2d4d808ef9471ca5 Mon Sep 17 00:00:00 2001
From: NAHO <[email protected]>
Date: Tue, 10 Sep 2024 00:33:18 +0200
Subject: [PATCH] stylix: add nixfmt formatter

---
 flake.lock | 23 ++++++++++++++++++++++-
 flake.nix  | 13 +++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/flake.lock b/flake.lock
index b05b9fd..ddfaa91 100644
--- a/flake.lock
+++ b/flake.lock
@@ -283,7 +283,8 @@
         "gnome-shell": "gnome-shell",
         "home-manager": "home-manager",
         "nixpkgs": "nixpkgs",
-        "systems": "systems"
+        "systems": "systems",
+        "treefmt-nix": "treefmt-nix"
       }
     },
     "systems": {
@@ -300,6 +301,26 @@
         "repo": "default",
         "type": "github"
       }
+    },
+    "treefmt-nix": {
+      "inputs": {
+        "nixpkgs": [
+          "nixpkgs"
+        ]
+      },
+      "locked": {
+        "lastModified": 1725271838,
+        "narHash": "sha256-VcqxWT0O/gMaeWTTjf1r4MOyG49NaNxW4GHTO3xuThE=",
+        "owner": "numtide",
+        "repo": "treefmt-nix",
+        "rev": "9fb342d14b69aefdf46187f6bb80a4a0d97007cd",
+        "type": "github"
+      },
+      "original": {
+        "owner": "numtide",
+        "repo": "treefmt-nix",
+        "type": "github"
+      }
     }
   },
   "root": "root",
diff --git a/flake.nix b/flake.nix
index d6c7192..e1dc976 100644
--- a/flake.nix
+++ b/flake.nix
@@ -71,6 +71,11 @@

     # Interface flake systems.
     systems.url = "github:nix-systems/default";
+
+    treefmt-nix = {
+      inputs.nixpkgs.follows = "nixpkgs";
+      url = "github:numtide/treefmt-nix";
+    };
   };

   outputs =
@@ -109,6 +114,14 @@
           ];
         };

+        formatter = (
+          inputs.treefmt-nix.lib.evalModule pkgs {
+            programs.nixfmt.enable = true;
+            projectRootFile = "flake.nix";
+          }
+        )
+        .config.build.wrapper;
+
         packages = let
             universalPackages = {
               docs = import ./docs { inherit pkgs inputs lib; };
--

For reference, Alejandra adds 69% less new lines than nixfmt:

• Alejandra

  • 87 files changed, 1355 insertions(+), 1148 deletions(-)
    

• nixfmt

  • 85 files changed, 2532 insertions(+), 1874 deletions(-)
    

[trueNAHO]

Changelog

v1: cb8c988

• Replace Alejandra with nixfmt-rfc-style
• Add ghc developer shell
• Restrict .github/workflows/docs.yml workflow permission to the docs job
• Rename pre-commit-hooks instances to git-hooks
• Rebase on top of commit ef81ad9 ("gnome: move gnome-shell overlay out of gnome scope (gnome: moved gnome-shell overlay outside its gnome packages umbrella #541)")

v0: ad87e25

[trueNAHO]

Changelog

v2: e7d6995

• Bump magic-nix-cache-action GitHub Action: v7 -> v8
• Bump nix-installer-action GitHub Action: v13 -> v14

v1: cb8c988

• Replace Alejandra with nixfmt-rfc-style
• Add ghc developer shell
• Restrict .github/workflows/docs.yml workflow permission to the docs job
• Rename pre-commit-hooks instances to git-hooks
• Rebase on top of commit ef81ad9 ("gnome: move gnome-shell overlay out of gnome scope (gnome: moved gnome-shell overlay outside its gnome packages umbrella #541)")

v0: ad87e25

[trueNAHO]

@danth, could you approve this PR again by looking at its changelog and addressing the following unanswered question:

-      - name: Prepare docs for upload
-        run: cp -r --dereference --no-preserve=mode,ownership result/ public/

Unless the situation has changed since the original workflow was written, I think this copy was necessary due to an error with the files being part of the Nix store. (It was either because they were owned by root, or because there were symlinks involved, I can't remember exactly)

Good catch! Admittedly, I adapted this GitHub Action from my dotfiles setup without testing it. The only difference with my dotfiles setup is:

-path: result/usr/share/doc
+path: result

I assume the GitHub Action does not consider path pointing to a read-only symlink directory. In my dotfiles setup, I do not run into such problems because path points to a regular (not a read-only symlink directory) directory inside result. I use /usr/share/doc because it is a more conventional FHS location for documentation.

Should we apply your cp workaround or move the documentation output to result/usr/share/doc? NOTE: This is unaddressed in v1.

-- #519 (comment)

I will rebase this PR on top of master and resolve merge conflicts when I resolve the GitHub Action issue based on your feedback.

Note

Heavily consider reviewing the commits individually (https://patch-diff.githubusercontent.com/raw/danth/stylix/pull/519.patch), as over 90% of the diff stems from the final "treewide: add and apply nixfmt pre-commit hook" commit.

[trueNAHO]

Changelog

v3: ffd7436

• Rebase on top of commit 04afcfc ("gnome: fix GDM theme not applying when Gnome is disabled (gnome: fix GDM theme not applying when Gnome is disabled #598)")
• Bump nix-installer-action GitHub action: v14 -> v15
• Remove Link: https://github.com/trueNAHO/dotfiles* tags from commit messages

v2: e7d6995

• Bump magic-nix-cache-action GitHub Action: v7 -> v8
• Bump nix-installer-action GitHub Action: v13 -> v14

v1: cb8c988

• Replace Alejandra with nixfmt-rfc-style
• Add ghc developer shell
• Restrict .github/workflows/docs.yml workflow permission to the docs job
• Rename pre-commit-hooks instances to git-hooks
• Rebase on top of commit ef81ad9 ("gnome: move gnome-shell overlay out of gnome scope (gnome: moved gnome-shell overlay outside its gnome packages umbrella #541)")

v0: ad87e25

[trueNAHO]

Cc: @danth

path: result

-      - name: Prepare docs for upload
-        run: cp -r --dereference --no-preserve=mode,ownership result/ public/

Unless the situation has changed since the original workflow was written, I think this copy was necessary due to an error with the files being part of the Nix store. (It was either because they were owned by root, or because there were symlinks involved, I can't remember exactly)

Good catch! Admittedly, I adapted this GitHub Action from my dotfiles setup without testing it. The only difference with my dotfiles setup is:

-path: result/usr/share/doc
+path: result

I assume the GitHub Action does not consider path pointing to a read-only symlink directory. In my dotfiles setup, I do not run into such problems because path points to a regular (not a read-only symlink directory) directory inside result. I use /usr/share/doc because it is a more conventional FHS location for documentation.

Should we apply your cp workaround or move the documentation output to result/usr/share/doc? NOTE: This is unaddressed in v1.

-- #519 (comment)

To continue making progress on the Roadmap by getting this PR merged, I have verified that the current path: result solution works.

Setup

To ensure that the GitHub Actions work properly, ensure that the GitHub repository has the following permissions before merging this PR.

https://github.com/danth/stylix/settings/actions

https://github.com/danth/stylix/settings/pages

Demo

The temporary https://github.com/trueNAHO/stylix-519 GitHub repository demonstrates the working GitHub Actions. Additionally, this repository displays the GitHub Pages website in its description:

PR Status

AFAIK, this PR is merge-ready with #608 being merged.

[trueNAHO]

Changelog

v4: 8880c87

• Rebase on top of commit 5ab1207 ("hyprland: adapt breaking changes (hyprland: adapt breaking changes #610)")
• Update git-hooks input
• Bump nix-installer-action GitHub action: v15 -> v16

v3: ffd7436

• Rebase on top of commit 04afcfc ("gnome: fix GDM theme not applying when Gnome is disabled (gnome: fix GDM theme not applying when Gnome is disabled #598)")
• Bump nix-installer-action GitHub action: v14 -> v15
• Remove Link: https://github.com/trueNAHO/dotfiles* tags from commit messages

v2: e7d6995

• Bump magic-nix-cache-action GitHub Action: v7 -> v8
• Bump nix-installer-action GitHub Action: v13 -> v14

v1: cb8c988

• Replace Alejandra with nixfmt-rfc-style
• Add ghc developer shell
• Restrict .github/workflows/docs.yml workflow permission to the docs job
• Rename pre-commit-hooks instances to git-hooks
• Rebase on top of commit ef81ad9 ("gnome: move gnome-shell overlay out of gnome scope (gnome: moved gnome-shell overlay outside its gnome packages umbrella #541)")

v0: ad87e25

[jaredmontoya]

@trueNAHO @danth isn't it better to just configure formatters and linters using treefmt-nix so that nix fmt executes all formatters and linters and nix flake check checks if formatting is applied. Then just execute nix flake check on github actions as a PR check?

pre-commit hooks are useless in my opinion if the only thing they are used for is formatting and linting, they are easily bypassed.
If someone is serious about enforcing code styling they should use treefmt-nix with a nix flake check github action that runs on pull requests.

treefmt-nix also has no dependencies other than nixpkgs so consumers of the nix flake don't have to download git hooks flake and all it's dependencies.

And treefmt-nix is such a simple enhancement with no downsides for nix fmt and nix flake check that everyone should be using it in their system configuration flake anyway. It's much better than default way of using nix fmt where user just assigns

formatter = pkgs.nixfmt-rfc-style;

and can't use anything other than that one formatter + doesn't get style checks on nix flake check

[trueNAHO]

isn't it better to just configure formatters and linters using treefmt-nix so that nix fmt executes all formatters and linters and nix flake check checks if formatting is applied. Then just execute nix flake check on github actions as a PR check?

pre-commit hooks are useless in my opinion if the only thing they are used for is formatting and linting, they are easily bypassed. If someone is serious about enforcing code styling they should use treefmt-nix with a nix flake check github action that runs on pull requests.

Yes, pre-commit hooks are an optional Git feature. Their compliance is, as you proposed, already checked with nix flake check in a GitHub action. Considering that Stylix naturally attracts casual one-off contributors, it seems good to ensure that their commits meet minimum quality standards:

 treewide: integrate linters and formatters into nix develop

What is the reasoning for this to be integrated into nix develop instead of just using nix fmt?

nix develop and nix flake simplify CI integration:

• I acknowledge pre-commit hooks being controversial. Nonetheless, they integrate nicely with nix flake check and nix develop, simplifying GitHub Actions and arguably improving the developer feedback loop.
  -- treewide: add linters and apply pending suggestions #426 (comment)

• Interesting, hooks might make it more convenient for regular contributors. Having it run on CI too ensures nothing is missed.
  -- treewide: add linters and apply pending suggestions #426 (comment)

-- #236 (comment)

treefmt-nix also has no dependencies other than nixpkgs so consumers of the nix flake don't have to download git hooks flake and all it's dependencies.

And treefmt-nix is such a simple enhancement with no downsides for nix fmt and nix flake check that everyone should be using it in their system configuration flake anyway. It's much better than default way of using nix fmt where user just assigns

formatter = pkgs.nixfmt-rfc-style;

and can't use anything other than that one formatter + doesn't get style checks on nix flake check

The current git-hooks.nix input introduces only the repository itself and its gitignore dependency as overhead. Note that end-users can disable the developer git-hooks input with:

inputs.stylix.inputs.git-hooks.follows = "";

Initially, nix fmt was supposed to be supported, but it didn't seem possible when I last tried. At least without weird workarounds:

Currently, harmonizing nixfmt (with the required treefmt-nix input) across nix fmt and pre-commit hooks is a nightmare and not worth implementing. The following patch demonstrates a "working" proof of concept without guaranteeing harmonization with nix fmt:

From 8c9597ea94795ee3061d6efb2d4d808ef9471ca5 Mon Sep 17 00:00:00 2001
From: NAHO <[email protected]>
Date: Tue, 10 Sep 2024 00:33:18 +0200
Subject: [PATCH] stylix: add nixfmt formatter

---
 flake.lock | 23 ++++++++++++++++++++++-
 flake.nix  | 13 +++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/flake.lock b/flake.lock
index b05b9fd..ddfaa91 100644
--- a/flake.lock
+++ b/flake.lock
@@ -283,7 +283,8 @@
         "gnome-shell": "gnome-shell",
         "home-manager": "home-manager",
         "nixpkgs": "nixpkgs",
-        "systems": "systems"
+        "systems": "systems",
+        "treefmt-nix": "treefmt-nix"
       }
     },
     "systems": {
@@ -300,6 +301,26 @@
         "repo": "default",
         "type": "github"
       }
+    },
+    "treefmt-nix": {
+      "inputs": {
+        "nixpkgs": [
+          "nixpkgs"
+        ]
+      },
+      "locked": {
+        "lastModified": 1725271838,
+        "narHash": "sha256-VcqxWT0O/gMaeWTTjf1r4MOyG49NaNxW4GHTO3xuThE=",
+        "owner": "numtide",
+        "repo": "treefmt-nix",
+        "rev": "9fb342d14b69aefdf46187f6bb80a4a0d97007cd",
+        "type": "github"
+      },
+      "original": {
+        "owner": "numtide",
+        "repo": "treefmt-nix",
+        "type": "github"
+      }
     }
   },
   "root": "root",
diff --git a/flake.nix b/flake.nix
index d6c7192..e1dc976 100644
--- a/flake.nix
+++ b/flake.nix
@@ -71,6 +71,11 @@

     # Interface flake systems.
     systems.url = "github:nix-systems/default";
+
+    treefmt-nix = {
+      inputs.nixpkgs.follows = "nixpkgs";
+      url = "github:numtide/treefmt-nix";
+    };
   };

   outputs =
@@ -109,6 +114,14 @@
           ];
         };

+        formatter = (
+          inputs.treefmt-nix.lib.evalModule pkgs {
+            programs.nixfmt.enable = true;
+            projectRootFile = "flake.nix";
+          }
+        )
+        .config.build.wrapper;
+
         packages = let
             universalPackages = {
               docs = import ./docs { inherit pkgs inputs lib; };
--

-- #519 (comment)