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 developer shell and pre-commit hooks and simplify GitHub workflows #519

Conversation

trueNAHO
Copy link
Collaborator

@trueNAHO trueNAHO commented Aug 22, 2024

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.

Closes: #236

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
Copy link
Collaborator Author

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 trueNAHO marked this pull request as ready for review August 26, 2024 17:30
@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Aug 26, 2024

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
Copy link
Collaborator Author

trueNAHO commented Sep 4, 2024

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
Copy link
Collaborator Author

trueNAHO commented Sep 4, 2024

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?

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.

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.

.github/workflows/docs.yml Show resolved Hide resolved
docs/src/development_environment.md Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
@trueNAHO
Copy link
Collaborator Author

-      - 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 added a commit to trueNAHO/stylix that referenced this pull request Sep 10, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Sep 10, 2024
Leverage direnv [1] to automatically enter the developer shell upon
entering the project directory after running 'direnv allow'.

[1]: https://direnv.net

Link: danth#519
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Sep 10, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Sep 10, 2024
Add the deadnix and statix pre-commit hooks used in the Lint GitHub
workflow.

Link: danth#519
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Sep 10, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Sep 10, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Sep 10, 2024
@trueNAHO trueNAHO force-pushed the treewide-add-devshell-and-pre-commit-hooks-and-simplify-github-workflows branch from ad87e25 to cb8c988 Compare September 10, 2024 15:11
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Sep 10, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Sep 10, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Sep 10, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Sep 10, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Sep 10, 2024
@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Sep 10, 2024

Changelog

v1: cb8c988

v0: ad87e25

trueNAHO added a commit to trueNAHO/stylix-playground that referenced this pull request Jan 4, 2025
@trueNAHO trueNAHO force-pushed the treewide-add-devshell-and-pre-commit-hooks-and-simplify-github-workflows branch from c16493e to ad64260 Compare January 4, 2025 17:03
@trueNAHO trueNAHO merged commit 7dfcdb4 into danth:master Jan 4, 2025
@stylix-automation
Copy link

Backport failed for release-24.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.11
git worktree add -d .worktree/backport-519-to-release-24.11 origin/release-24.11
cd .worktree/backport-519-to-release-24.11
git switch --create backport-519-to-release-24.11
git cherry-pick -x 703f49aaca9030b066bb60310d19d7cadfb4b376 6ef37ca6aaf82e59394d0a93acdc932af1739d58 3c54cb3384116d430e2a83c5d3d3f0dd59eb9ff2 6085560893ae3620acf8d799dda138b207e6f5ed 0d0af5f4426104ed82b17d41f8763fcf84b2c0ea b3230ef39814b1c3809ffbad20f6361c8b737731 e56d332ca367bd021d3c584ac9130064916cd0c7 a0838923e45f63b2b46a132eaa02c45e03e8818a 1aa931f6f1eb85b4f8358e7ed3706ed82d3048ca fe72c2306f517a865c23e9fc92fc72cc408c1ab7 2b85a562355c80e1ef2f9070a3a44befdd7c9764 5ab7d0345a902f5f4c30f9c31ccf42c563ad6463 d3bdbf0c5b4656955be0019e821de6fa65a6bb78 4ceede75042e362d3270b52d4870702a99915bb0 439c6cf24e89730d3271baf10e5706df1cdecedf 708b2147c095a60994f56714968f00de531f8deb 807c81894e9af60b105476c3dab679e9dc86686f ad64260a75f0331d4d9b0db70f60634283e0aa5d

@stylix-automation
Copy link

Backport failed for release-24.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.11
git worktree add -d .worktree/backport-519-to-release-24.11 origin/release-24.11
cd .worktree/backport-519-to-release-24.11
git switch --create backport-519-to-release-24.11
git cherry-pick -x 703f49aaca9030b066bb60310d19d7cadfb4b376 6ef37ca6aaf82e59394d0a93acdc932af1739d58 3c54cb3384116d430e2a83c5d3d3f0dd59eb9ff2 6085560893ae3620acf8d799dda138b207e6f5ed 0d0af5f4426104ed82b17d41f8763fcf84b2c0ea b3230ef39814b1c3809ffbad20f6361c8b737731 e56d332ca367bd021d3c584ac9130064916cd0c7 a0838923e45f63b2b46a132eaa02c45e03e8818a 1aa931f6f1eb85b4f8358e7ed3706ed82d3048ca fe72c2306f517a865c23e9fc92fc72cc408c1ab7 2b85a562355c80e1ef2f9070a3a44befdd7c9764 5ab7d0345a902f5f4c30f9c31ccf42c563ad6463 d3bdbf0c5b4656955be0019e821de6fa65a6bb78 4ceede75042e362d3270b52d4870702a99915bb0 439c6cf24e89730d3271baf10e5706df1cdecedf 708b2147c095a60994f56714968f00de531f8deb 807c81894e9af60b105476c3dab679e9dc86686f ad64260a75f0331d4d9b0db70f60634283e0aa5d

trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
Link: danth#519
(cherry picked from commit 703f49a)
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
Leverage direnv [1] to automatically enter the developer shell upon
entering the project directory after running 'direnv allow'.

[1]: https://direnv.net

Link: danth#519
(cherry picked from commit 6ef37ca)
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
Add the deadnix and statix pre-commit hooks used in the Lint GitHub
workflow.

Link: danth#519
(cherry picked from commit 6085560)
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
Consolidate the separate Build and Lint workflows into a unified Check
workflow, as linting is now integrated into the build process.

Link: danth#519
(cherry picked from commit 0d0af5f)
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
Link: danth#519
(cherry picked from commit e56d332)
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
Add the nix-flake-check package, which is a parallelized alternative to
'nix flake check', as it is not yet natively parallel:

> In the near future, we will make more Nix subcommands multi-threaded,
> such as 'nix flake check'.
>
> -- Eelco Dolstra, https://determinate.systems/posts/parallel-nix-eval

On a 16-threaded machine, 'nix run .#nix-flake-check' runs three times
faster (74s vs. 243s) than 'nix flake check'.

Link: danth#519
(cherry picked from commit a083892)
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
Link: danth#519
(cherry picked from commit 2b85a56)
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
Prevent the Check workflow from running duplicated checks outputs.

The jq query should now be faster and arguably more readable.

Link: danth#519
(cherry picked from commit 4ceede7)
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
Link: danth#519
(cherry picked from commit 439c6cf)
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
Link: danth#519
(cherry picked from commit 708b214)
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
Link: danth#519
(cherry picked from commit 807c818)
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Jan 4, 2025
trueNAHO added a commit that referenced this pull request Jan 4, 2025
…ows (#746)

Add developer shells with direnv integration and the deadnix, hlint,
nixfmt-rfc-style, statix, stylish-haskell, typos, and yamllint
pre-commit hooks.

Ensure 'nix flake check' works as expected and add the nix-flake-check
package, which is a parallelized alternative to 'nix flake check'.

Improve and update the GitHub workflows.

Closes: #236
Link: #519

Approved-by: Daniel Thwaites <[email protected]>
(adapted from commit 7dfcdb4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Project maintenance task technical debt Things which should be cleaned up or improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

treewide: add linters and formatters
3 participants