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.

Sign up for GitHub

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

Jump to bottom

feat: support cargo/runtime dependencies #396

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

feat: support cargo/runtime dependencies #396

wants to merge 1 commit into from

Conversation

mrcjkb
Copy link
Contributor

@mrcjkb mrcjkb commented Feb 10, 2024 •
edited
Loading

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.

@mrcjkb mrcjkb mentioned this pull request Feb 10, 2024
Merged
@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 •
edited
Loading

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 sandydoo mentioned this pull request May 7, 2024
Closed
@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 •
edited
Loading

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.

@sandydoo sandydoo mentioned this pull request Jun 2, 2024
Open
@sandydoo sandydoo mentioned this pull request Jun 12, 2024
Open
@sandydoo
Copy link
Member

@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
Copy link
Contributor Author

mrcjkb commented Jun 12, 2024

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

@sandydoo sandydoo added enhancement New feature or request flake Related to running in pure environments labels Jun 19, 2024
@Scrumplex
Copy link

Seems like this PR conflicts with the main branch

@mrcjkb
Copy link
Contributor Author

mrcjkb commented Aug 14, 2024

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

@mrcjkb mrcjkb force-pushed the clippy branch 3 times, most recently from 499c28e to 43da434 Compare September 4, 2024 08:01
@mrcjkb mrcjkb marked this pull request as draft September 4, 2024 08:13
@mrcjkb mrcjkb force-pushed the clippy branch 2 times, most recently from 612be66 to ec0f4d9 Compare September 4, 2024 08:23
@mrcjkb mrcjkb marked this pull request as ready for review September 4, 2024 08:51
@mrcjkb
Copy link
Contributor Author

mrcjkb commented Sep 4, 2024

@sandydoo I finally got around to this again.

I've updated the PR to reuse extraPackages (concatenating them from each hook)
and renamed rust.cargoDeps to rust.check.cargoDeps to clarify that it's only needed for checks.

It would be nice to have cargoDeps per hook, but I don't think it's possible to merge them from multiple hooks.

sandydoo
sandydoo requested changes Sep 19, 2024
View reviewed changes
modules/pre-commit.nix Outdated Show resolved Hide resolved
modules/pre-commit.nix Outdated Show resolved Hide resolved
modules/hooks.nix
description = "Path to Cargo.toml";
default = null;
rust = {
check.cargoDeps = mkOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domenkozar, need to make a call here.
This is a setting that'll only be used to fix nix flake check. Other hooks might need to make their own modifications to the derivation used in flake check.
Should we prefix them somehow or not?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandydoo sandydoo sandydoo requested changes

Assignees
No one assigned
Labels
enhancement New feature or request flake Related to running in pure environments
Projects
None yet
Milestone
No milestone
4 participants
@mrcjkb @domenkozar @sandydoo @Scrumplex