nix-community / nixdoc

Tool to generate documentation for Nix library functions [maintainer=@infinisil]
GNU General Public License v3.0
122 stars 14 forks source link

ci: run rustfmt #63

Closed asymmetric closed 1 year ago

infinisil commented 1 year ago

Btw you can remove the checks.test attribute in flake.nix. It only builds the package, which the clippy test already does. Removing it speeds up nix flake check by about half since it doesn't need to build the same package twice.

asymmetric commented 1 year ago

@infinisil well they are slightly different no? checks.test runs cago test, checks.clippy doesn't as it overrides the checkPhase.

infinisil commented 1 year ago

There's apparently no check phase defined by default:

❯ nix repl
Welcome to Nix 2.15.1. Type :? for help.

nix-repl> :lf .                               
Added 16 variables.

nix-repl> checks.x86_64-linux.test ? checkPhase
false

But we can also just make sure we append to the checkPhase instead of overriding it completely (in case it exists in the future, or just good practice)

diff --git a/flake.nix b/flake.nix
index 15b94e8..c2e2ae5 100644
--- a/flake.nix
+++ b/flake.nix
@@ -30,10 +30,8 @@
         };

         checks = {
-          test = self.packages.${system}.default;
-
-          clippy = self.packages.${system}.default.overrideAttrs (_: {
-            checkPhase = ''
+          test = self.packages.${system}.default.overrideAttrs (attrs: {
+            checkPhase = attrs.checkPhase or "" + ''
               ${pkgs.clippy}/bin/cargo-clippy --no-deps -- -D warnings
             '';
           });
asymmetric commented 1 year ago

There's apparently no check phase defined by default:

@infinisil nix flake check definitely runs cargo testthough (I can see it in the logs when running nix flake check -L) :shrug:

See here which calls this hook.

But we can also just make sure we append to the checkPhase instead of overriding it completely (in case it exists in the future, or just good

Regading CI build times, my plan was to eventually switch to crane, which would create a separate derivation for compiling the build deps, which could then be reused by both test and ci outputs and would in most cases be cached by Cachix.

Re: best practices, it seems cleaner to me to replace the checkPhase rather than extending it :) Not sure I have super strong arguments for this, just preference.