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

Merged
merged 1 commit into from
Dec 8, 2024
Merged

feat: support cargo/runtime dependencies #396

merged 1 commit into from
Dec 8, 2024

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
Closed
@sandydoo sandydoo mentioned this pull request Jun 12, 2024
Closed
@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 Show resolved Hide resolved
@DaRacci
Copy link

DaRacci commented Dec 6, 2024

I've been using this and it seems to be in a good position will this be merged soon?

@sandydoo sandydoo merged commit d8c02f0 into cachix:master Dec 8, 2024
4 checks passed
@mrcjkb mrcjkb deleted the clippy branch December 8, 2024 13:51
vlaci added a commit to vlaci/git-hooks.nix that referenced this pull request Jan 6, 2025
@vlaci
fix: pre-commit errors due to polluted git directory
775bf6b 
After cachix#396, `reuse` hook no longer works, as it considers untracked
files added by `pre-commit`. Before the above PR, the git repository
was in `$HOME/src`, so files added in `$HOME` didn't affect the
outcome of the hooks themselves.

This change re-adds the `src` subdirectory.

A failing build output:

     $ nix build --no-link -L .\#checks.x86_64-linux.pre-commit
     Running phase: unpackPhase
     unpacking source archive /nix/store/g1p6g2ylflj4fnzdwlc368737kxk0p9f-source
     source root is source
     Running phase: patchPhase
     Running phase: updateAutotoolsGnuConfigScriptsPhase
     Running phase: configurePhase
     no configure script, doing nothing
     Running phase: buildPhase
     Running: $ pre-commit run --all-files
     reuse....................................................................Failed
     - hook id: reuse
     - exit code: 1
     # MISSING COPYRIGHT AND LICENSING INFORMATION
     The following files have no copyright and licensing information:
     * .cache/pre-commit/README
     * .cache/pre-commit/db.db
     * .gitconfig
     # SUMMARY
     * Bad licenses: 0
     * Deprecated licenses: 0
     * Licenses without file extension: 0
     * Missing licenses: 0
     * Unused licenses: 0
     * Used licenses: MIT
     * Read errors: 0
     * Files with copyright information: 16 / 19
     * Files with license information: 16 / 19
     Unfortunately, your project is not compliant with version 3.3 of the REUSE Specification :-(
     # RECOMMENDATIONS
     * Fix missing copyright/licensing information: For one or more files, the tool
       cannot find copyright and/or licensing information. You typically do this by
       adding 'SPDX-FileCopyrightText' and 'SPDX-License-Identifier' tags to each
       file. The tutorial explains additional ways to do this:
       <https://reuse.software/tutorial/>
@vlaci vlaci mentioned this pull request Jan 6, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandydoo sandydoo sandydoo approved these changes

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