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: explicitly mention pkgs as module argument #32

Merged
merged 1 commit into from
Mar 17, 2024

Conversation

korrat
Copy link
Contributor

@korrat korrat commented Mar 15, 2024

The previous change made in PR #31 caused failures when depending on pkgs in modules captured by haumea. This is due to the behaviour of nixpkgs.lib.nixos.evalModules which does not look up all arguments but only those captured by name explicitly, in addition to config, options, and lib passed by default.

This patch fixes those failures by explicitly making pkgs part of the argument list for the module returned by load, so that evalModules applies the correct argument list.

The previous change made in PR divnix#31 caused failures when depending on `pkgs` in modules captured by haumea. This is due to the behaviour of `nixpkgs.lib.nixos.evalModules` which does not look up all arguments but only those captured by name explicitly, in addition to `config`, `options`, and `lib` passed by default.

This patch fixes those failures by explicitly making `pkgs` part of the argument list for the module returned by `load`, so that `evalModules` applies the correct argument list.
@korrat
Copy link
Contributor Author

korrat commented Mar 15, 2024

I have tested this with https://github.com/Lord-Valen/configuration.nix, replacing all the FIXMEs. No errors there.

args: let
args @ {pkgs, ...}: let
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a module system without pkgs is at least conceivable, so we should preemptively do pkgs ? {}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@whs-dot-hk what's your thought, here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think so, with a default {} is better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wouldn't work, because applyModuleArgs would fail. You cannot specify default arguments for module functions: NixOS/nixpkgs#243303.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wouldn't work, because applyModuleArgs would fail. You cannot specify default arguments for module functions: NixOS/nixpkgs#243303.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow! Thanks for curating the reference here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, can confirm the default do nothing (such as for pkgs) with mytest, when there is no config._module.args.mytest = "test"; there will be an error even a default "" is given (mytest ? "")

@whs-dot-hk
Copy link
Contributor

I also think transformer with default

with haumea.lib.transformers; [
            liftDefault
            (hoistLists "_imports" "imports")
          ]

would be great if it can pass down from findLoad wdyt?

@korrat
Copy link
Contributor Author

korrat commented Mar 17, 2024

I also think transformer with default

with haumea.lib.transformers; [
            liftDefault
            (hoistLists "_imports" "imports")
          ]

would be great if it can pass down from findLoad wdyt?

Sounds like a good idea. I've seen a few hives which created custom overrides of load/findLoad just to adjust the transformer. For these use cases, allowing an override would certainly be beneficial.

Perhaps the loader could be treated similarly? Have it default to scopedImport and allow overriding for flexibility.

However, I wouldn't include those changes as part of a quick fix PR. Maybe we include it in the load refactor in #33, instead?

@blaggacao
Copy link
Collaborator

Yeah, let's go step by step. Merging... Thx!

@blaggacao blaggacao merged commit e7e3873 into divnix:main Mar 17, 2024
@korrat korrat deleted the fix-load-args branch March 17, 2024 19:55
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