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

feat: support cargo/runtime dependencies #396

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mrcjkb
Copy link
Contributor

@mrcjkb mrcjkb commented Feb 10, 2024

Fixes #126.

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

This adds settings.rust.cargoDeps and settings.runtimeDeps options, which can be used to specify the dependencies, e.g.

settings = {
  runtimeDeps = [ pkgs.openssl ];
  rust.cargoDeps = pkgs.rustPlatform.importCargoLock {
    lockFile = ./Cargo.lock;
  };
}

@domenkozar
Copy link
Member

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

@mrcjkb
Copy link
Contributor Author

mrcjkb commented Mar 20, 2024

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

Done 😄

@mrcjkb
Copy link
Contributor Author

mrcjkb commented Apr 11, 2024

@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 😃

@domenkozar
Copy link
Member

Yeah sorry :(

@domenkozar
Copy link
Member

Does #430 address this?

@mrcjkb
Copy link
Contributor Author

mrcjkb commented Apr 22, 2024

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.

@mrcjkb
Copy link
Contributor Author

mrcjkb commented Apr 24, 2024

@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?

@sandydoo
Copy link
Member

sandydoo commented Apr 25, 2024

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?

@mrcjkb
Copy link
Contributor Author

mrcjkb commented Apr 25, 2024

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.

@sandydoo
Copy link
Member

sandydoo commented May 7, 2024

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?

@mrcjkb
Copy link
Contributor Author

mrcjkb commented May 8, 2024

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?

  1. 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)

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

Yes, I think so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustfmt and clippy fail with "No such file or directory"
3 participants