git-hooks.nix

git-hooks.nix copied to clipboard

Fixes #126. Fixes #452. Fixes #455.

If a Rust project has dependencies, cargo-clippy and cargo check will fail in the pre-commit-check derivation.

This adds a settings.rust.check.cargoDeps ~and settings.runtimeDeps~ option, which can be used to specify the dependencies, e.g.

settings = {
  rust.check.cargoDeps = pkgs.rustPlatform.importCargoLock {
    lockFile = ./Cargo.lock;
  };
}

and reuses the hooks' extraPackages as nativeBuildInputs in the derivation, as they are likely to be used in the checks in addition to the devShell environment.

Sorry for the mess, we revamed module structure in https://github.com/cachix/pre-commit-hooks.nix/pull/397, could you port these changes over?

Sorry for the mess, we revamed module structure in #397, could you port these changes over?

Done :smile:

@domenkozar looks like this is conflicted again.

Please let me know if you would like to get this merged, and I will resolve conflicts one more time 😃

Yeah sorry :(

Does https://github.com/cachix/git-hooks.nix/pull/430 address this?

Does #430 address this?

It looks like it addresses this partially.

• settings.runtimeDeps seems to be addressed.
• settings.rust.cargoDeps (required e.g. to run clippy in sandboxed/offline mode) isn't, afaict.

I'll have to look into it at a later time.

@domenkozar Actually, I don't think #430 addresses the runtimeDeps either. It looks to me like it only adds dependencies that are required to run a hook in a devShell, but it won't include them when the hook is run in a sandboxed nix flake check.

Or am I missing something?

I'm just wondering if this is the right place to inject these dependencies. Can they go into the packages for clippy and cargo check?

Otherwise, aren't the individual hook entrys broken unless executed within this run derivation? Will a manual pre-commit run <hook_id> work inside the shell?

I'm just wondering if this is the right place to inject these dependencies. Can they go into the packages for clippy and cargo check?

You mean using something like symlinkJoin? I don't think so. They need to be in nativeBuildInputs and cargoSetupHook is needed so that clippy can find the crates.

Otherwise, aren't the individual hook entrys broken unless executed within this run derivation?

No, they're not. Clippy will fetch the crates from the crate registy (just as cargo build does).

Will a manual pre-commit run <hook_id> work inside the shell?

Yes.

You mean using something like symlinkJoin?

No, I meant using buildInputs and friends in a derivation that wraps clippy. But since this is a purely flake check issue, that would make setting up the shell more expensive than it needs to be.

My current concerns/thoughts/ideas:

1. The new options only affect nix flake check and that isn't clear from the way they're defined.
2. The new options are essentially mkDerivation options. Can and should we generalize this? Is there a design that's basically "give us an attrset and we'll merge it into mkDerivation"?
3. Should runtimeDeps be added to the shell as well? In that case, can we re-use extraPackages?

I see.

My current concerns/thoughts/ideas:

Here are my thoughts:

1. The new options only affect nix flake check and that isn't clear from the way they're defined.

Good point. I guess they could go in a settings.check set?

2. The new options are essentially mkDerivation options. Can and should we generalize this? Is there a design that's basically "give us an attrset and we'll merge it into mkDerivation"?

I would say YAGNI. The options could be deprecated and copied into such an attrset if that is needed one day. But maybe this can be worked around be overriding the attrs of the run derivation? In which case this could be closed if it's too much of an edge case 🤔 (I'll have to look into that later)

3. Should runtimeDeps be added to the shell as well? In that case, can we re-use extraPackages?

Yes, I think so.

@mrcjkb, let's get this merged in. Would you have time to work on this?

Let's re-use extraPackages and mark somehow mark that cargoDeps is specifically for nix flake check. Either settings.check or some sort of flakeCheck option in `pre-commit.nix.

@mrcjkb, let's get this merged in. Would you have time to work on this?

Sure. I'm sick today, but hopefully I'll be able to pick it up again soon 😄

Seems like this PR conflicts with the main branch

Totally forgot about this. I will have to rework this from scratch anyway.