Add missing-patch-comment check using rust and rnix #16

rmcgibbo · 2021-01-24T18:06:07Z

Example:

$ nix run -c nixpkgs-hammer -f ~/projects/nixpkgs python38Packages.credstash
When evaluating attribute ‘python38Packages.credstash’:
warning: missing-patch-comment
Please add a comment on the line above explaining the purpose of this patch.
Near /home/mcgibbon/projects/nixpkgs/pkgs/development/python-modules/credstash/default.nix:13:4:
   |
13 |     (fetchpatch {
   |    ^
See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/missing-patch-comment.md

Here's the relevant line of code: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/python-modules/credstash/default.nix#L13

Notes:
(a) This is a big change because it requires rust and rnix. There might be better ways to do this with other tools.
(b) The check, as implemented, is probably too strict. It looks for each element in the patches = [...] list, and demands to find a comment immediately prior to the element. So, for example, a comment like this one in imageio-ffmpeg is not currently detected. But this could be changed.

tools/nixpkgs-hammer

flake.nix

rmcgibbo · 2021-01-24T22:19:53Z

I've pushed a bunch of improvements + fixes in response to your review in 9313e78

tools/nixpkgs-hammer

flake.nix

ast-checks/Cargo.toml

ast-checks/src/comment_finders.rs

ast-checks/src/missing-patch-comment.rs

ast-checks/src/tree_utils.rs

ast-checks/src/missing-patch-comment.rs

ast-checks/src/tree_utils.rs

ast-checks/Cargo.toml

ast-checks/src/comment_finders.rs

ast-checks/src/tree_utils.rs

flake.nix

rmcgibbo · 2021-01-25T18:44:06Z

In dea035c, I've switched to naersk for building the rust package, which required updating the flake lock file. I don't have much of an opinion one way or the other, so I'm happy to leave that in or revert it.

ast-checks/src/comment_finders.rs

ast-checks/src/tree_utils.rs

lovesegfault

The code looks good, I left one last idea for a small improvement :)

ast-checks/src/missing-patch-comment.rs

jtojnar · 2021-01-28T15:21:57Z

explanations/missing-patch-comment.md

+
+In each patch comment, please explain the purpose of the patch and link to the relevant upstream issue if possible. If the patch has been merged upstream but is not yet part of the released version, please note the version number or date in the comment such that a future maintainer updating the nix expression will know whether the patch has been incorporated upstream and can thus be removed from nixpkgs.
+
+Furthermore, please use a _stable_ URL for the patch. Rather than, for example, linking to a github pull request of the form `https://github.com/owner/repo/pull/pr_number.patch` which would change every time a commit is addded or the PR is force-pushed, link to a specific commit-sha.patch of the form `https://github.com/owner/repo/commit/sha.patch`.


Agreed. But maybe this should be a separate check. But looks good enough and we can extract it later.

👍 Separating it out into another check at a later time sounds good to me.

flake.nix

rmcgibbo · 2021-01-31T21:29:59Z

(I've rebased on top of master and squashed this into 1 commit.)

jtojnar

Looks very nice, just few last comments.

flake.nix

ast-checks/src/tree_utils.rs

ast-checks/src/missing-patch-comment.rs

rmcgibbo · 2021-01-31T23:05:08Z

Pushed a new commit based on your comments.

jtojnar

Tweaked style a bit, added a forgotten args.show_trace and now it is good to go.

Thanks for the excellent work.

jtojnar · 2021-02-01T00:24:41Z

.github/workflows/main.yaml

@@ -15,4 +15,4 @@ jobs:
      uses: cachix/install-nix-action@v12

    - name: Run tests
-      run: nix-shell --run ./run-tests.py
+      run: nix run -c ./run-tests.py


Just to save myself confusion in the future, Nix 2.4 will likely use nix shell command for this feature (this is how it is named at current nixUnstable).

jtojnar · 2021-02-01T00:29:34Z

flake.nix

@@ -37,7 +61,8 @@
    devShell = pkgs.mkShell {
      buildInputs = with pkgs; [
        python3
-        nix


Removed Nix here since nixpkgs-hammer will already have it in PATH when ran from the package (as tests do), and for development one will have Nix anyway. And stable Nix here would make updating flake annoying.

rmcgibbo · 2021-02-01T00:32:04Z

Awesome!

rmcgibbo force-pushed the missing-patch-comment branch from e936d37 to 8df319b Compare January 24, 2021 18:07