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

fix: avoid eval error by discouraging overzealus module signatures #416

Closed
wants to merge 1 commit into from

Conversation

blaggacao
Copy link
Contributor

No description provided.

@blaggacao
Copy link
Contributor Author

Stuff like this:

error: attribute 'pre-commit-hooks' missing

       at /nix/store/h3vlwnvhbkjlck4ps694kmz20jwvgscc-source/lib/modules.nix:483:28:

          482|         builtins.addErrorContext (context name)
          483|           (args.${name} or config._module.args.${name})
             |                            ^
          484|       ) (lib.functionArgs f);
(use '--show-trace' to show detailed location information)
/nix/store/1c147j530hi4p2v5dlpg5x9dm309pivi-source/direnv_lib.s

The following consumption pattern is fixed by this change:

{
  inputs.devenv.url = "github:cachix/devenv?dir=src/modules";
}

@blaggacao blaggacao force-pushed the refactor-avoid-eval-errors branch from 68ed06a to 7de4420 Compare February 18, 2023 22:00
@blaggacao
Copy link
Contributor Author

boah, I guess we need an integration point for services....

error: The option `pre-commit' does not exist. Definition values:
       - In `/nix/store/xyp27kgag9yp8ldix8rsg38wpr0xv1ff-source/src/modules/languages/rust.nix':
           {
             _type = "if";
             condition = false;
             content = {
               tools = {
           ...
(use '--show-trace' to show detailed location information)

@blaggacao
Copy link
Contributor Author

Uncomfortable, but works:

{
  eval = module: (lib.evalModules {
    modules = [
        (devenv.modules + /top-level.nix)
        {
          disabledModules = []
            ++ (listEntries (devenv.modules + /integrations))
            ++ (listEntries (devenv.modules + /languages))
          ;
        }
        module
    ];
    specialArgs = {
      pkgs = nixpkgs;
      inherit inputs;
    };
  }).config;

Maybe this should become an issue?

Copy link
Contributor

@bobvanderlinden bobvanderlinden left a comment

Choose a reason for hiding this comment

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

I like that this makes pre-commit-hooks less special 👍 Makes sense to pass is as an input in specialArgs.

Comment on lines +5 to +9
or (throw ''
To use integrations.pre-commit, you need to add the following to your flake inputs:

inputs.pre-commit-hooks.url = "github:cachix/pre-commit-hooks.nix";
'');
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to refer to devenv.yaml, instead of flakes, as that is the default way people are using devenv. The same is done in the Ruby module:

nixpkgs-ruby = inputs.nixpkgs-ruby or (throw ''
To use languages.ruby.version or languages.ruby.versionFile, you need to add the following to your devenv.yaml:
inputs:
nixpkgs-ruby:
url: github:bobvanderlinden/nixpkgs-ruby
'');

@domenkozar
Copy link
Member

The issue with this change is that it will break for all users that won't use devenv from master, as soon as they upgrade.

We need to provide backwards compatible way to transition (it should just work for devenv 0.5.1 or lower).

@blaggacao
Copy link
Contributor Author

Please take over!

@domenkozar
Copy link
Member

For anyone that picks this up:

  • instructions show should devenv.yaml snippet (there could be a function that chooses based on config.devenv.flakesIntegration which format to pick)
  • make sure it doesn't break backwards compatibility

@domenkozar domenkozar force-pushed the main branch 4 times, most recently from e82dccf to 2cae5ec Compare March 2, 2023 11:27
@domenkozar domenkozar marked this pull request as draft July 5, 2023 09:51
@domenkozar
Copy link
Member

This should be done on top of #745

@domenkozar
Copy link
Member

Closing as it's completely out of date, if someone wants to pick it up, please do.

@domenkozar domenkozar closed this Mar 21, 2024
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.

3 participants