hercules-ci / flake-parts

❄️ Simplify Nix Flakes with the module system
https://flake.parts
MIT License
728 stars 41 forks source link

Exposing a flakeModule that depends on other flake modules without defining in the same file #121

Open terlar opened 1 year ago

terlar commented 1 year ago

What is the best practice to expose a flakeModule that depends on other flake modules?

I have something like this: flake.nix:

...
  outputs = inputs:
    inputs.flake-parts.lib.mkFlake {inherit inputs;} {
      systems = ["x86_64-linux" "x86_64-darwin" "aarch64-darwin"];
      imports = [./modules.nix];
    };
...

modules.nix:

{inputs, ...}:

{
  imports = [inputs.flake-parts.flakeModules.flakeModules];
  flake.flakeModules.default = import ./my-module.nix;
}

my-module.nix:

{inputs, ...}: {
  imports = [input.flake-dep1.flakeModule input.flake-dep2.flakeModule];
}

I currently have 4 solutions, none of them feel optimal.

  1. Define the module in the same file, so I can access the outer scope.
  2. Pass a top-level argument to the module (then the module can't be imported directly via imports, but one needs to do (import ./modules.nix { inherit inputs; }) for example)
  3. Have a wrapper for the module file inside the modules.nix, { imports = [...inputs, ./my-module.nix]; }
  4. Split out the module as a module within the current flake that then defines the module.

I guess there are no _module.args.inputs or something that could be utilized?

What is the best practices for this kind of thing?

srid commented 1 year ago

I currently require the user to manually import all the dependencies, in https://github.com/Platonic-Systems/mission-control for example

image

But I guess what we probably want is some sort of module dependency system.

roberth commented 1 year ago

I currently require the user to manually import all the dependencies

This touches on a deeper discussion about how we want to manage module dependencies, besides the syntax we use to import them:

Let's purpose this thread for the more "syntax-level" details.

roberth commented 1 year ago
{inputs, ...}:

{
  imports = [inputs.flake-parts.flakeModules.flakeModules];

This is making an assumption about the inputs of the final flake, which we should try to avoid. We can't know for sure that inputs.flake-parts exists. Of course this one is quite common, but even this one isn't great because it doesn't allow flake-parts to be wrapped into some other flake. Maybe someone uses inputs.companyflake.mkFlake with some pre-included modules or very restricted syntax, or whatever they think helps their company flakes. A more realistic example is the possibility of bundling modules. In that case, probably only one or two direct inputs are about flake-parts modules, and the rest are transitive dependencies. There's also the idea that when flakes allows arbitrary names for dependencies, flake-parts should probably allow that too.

  flake.flakeModules.default = import ./my-module.nix;

It's better not to use import. import turns a path-based module reference into an anonymous module (just a function) without remembering the path. This affects error messages and deduplication. flake.flakeModules.default = ./my-module.nix; will preserve the file name.

  1. Define the module in the same file, so I can access the outer scope.

This can be combined with a separate file.

2. Pass a top-level argument to the module (then the module can't be imported directly via imports, but one needs to do (import ./modules.nix { inherit inputs; }) for example)

This creates the input name dependency that we should try to avoid.

What is the best practices for this kind of thing?

I don't have a definitive answer yet, but these are competing solutions:

importApply

Use this library function to fix the forgotten path problem when defining modules, while still making things from the lexical scope available to the module.

      # Pass arguments to a module inside a function inside a file, while preserving
      # the file name behavior of the module system.
      importApply =
        modulePath: staticArgs:
          lib.setDefaultModuleLocation modulePath (import modulePath staticArgs);

This should perhaps be added to Nixpkgs. Here's a usage example with NixOS instead of flake-parts.

Separation of technical concerns

Here's an example of a module that always uses the exact same package in its flake-parts module, regardless of what the end-user pkgs ends up being.

# foologic/flake.nix
foologic@{ getSystem, ... }:
{
  flake.flakeModules.foologic = {
    imports = [
      # foologic.nix defines perSystem.foologic.package among other things
      ./foologic.nix
    ];
    config = {
      # reach into all the options that need values from the foologic flake
      perSystem = { system, ... }: {
        foologic.package = (foologic.getSystem system).packages.default;
      };
    };
  };
}

Separation of concerns is good, but I don't like having to separate things for technical reasons. How to divide up the domain should be up to the module author, not arbitrary technical requirements, ideally.

This doesn't work perfectly yet, but this should fix it

terlar commented 1 year ago

To clarify this was not part of the exposed module, this was the flake itself and how it defined the module, so I guess that should be fine:

{inputs, ...}:

{
  imports = [inputs.flake-parts.flakeModules.flakeModules];

I managed to get it to work with the importApply solution. As well as fixing all my usage to not use import where unnecessary (thank you for that hint, somehow I thought it would be weird to expose the module file itself). Is it also okay to expose an attribute-set as the module? or is that bad practice?

For example:

  flake.flakeModules = {
    default = {
      imports = [
        config.flake.flakeModules.moduleA
        config.flake.flakeModules.moduleB
      ];
    };
roberth commented 1 year ago

Yeah flakeModules.flakeModules should also take care of giving identifiers to your modules, even if they were anonymous like the attrset module syntax in your example. So your example is fine.

(just to add) importApply seems like a good practice regardless of what flakeModules.flakeModules manages to fix up, because solving the problem completely is nicer than having to worry about whether the problem was solved sufficiently.

roberth commented 1 year ago

Adding importApply:

This solves the location part. The "key" aspect (dedup and disabledModules) can not reasonably be covered by it, so this will be the responsibility of the flake.*Modules options.

Atry commented 1 year ago

As mentioned in https://github.com/hercules-ci/flake-parts/pull/146#issuecomment-1538653513 , importApply does not solve all the problems.

I think the easier solution is to turn flakeModulesOption into a submodule instead of a lazyAttrsOf, because lazyAttrsOf is not really lazy.

flakeModulesOption = mkOption {
  apply = builtins.mapAttr (name: module: rec { imports = [ module ]; key = "${inputs.self}#flakeModules.${name}" }; _file = key; });
  type = types.submoduleOf {
    modules = [
      {
        # Making it be a freeformType for backward compatibility only
        config._module.freeformType = types.lazyAttrsOf types.deferredModule;
      }
    ];
  };
};

Then a flake module can be defined as an option to reference other flake modules:

# myModule1.nix

# Note that the inputs are from the lexical scope, not the user's root inputs, solving https://github.com/hercules-ci/flake-parts/issues/104
{flake-parts-lib, inputs, ...}:{
  option.flake = flake-parts-lib.mkSubmoduleOption {
    flakeModules = flake-parts-lib.mkSubmoduleOption {
      myModule1 = flake-parts-lib.mkDeferredModuleOption {
        options.myOption = mkOption { default = "my default value"; };
      };
    };
  };
}
# myModule2.nix
{flake-parts-lib, inputs, ...}:{
  imports = [ ./myModule1.nix ];
  option.flake = flake-parts-lib.mkSubmoduleOption {
    flakeModules = flake-parts-lib.mkSubmoduleOption (flakeModules: {
      myModule2 = flake-parts-lib.mkDeferredModuleOption {
        imports = [ flakeModules.config.myModule1 ];
        config.myOption = "my overridden value";
      };
    });
  };
}

If mapAttr does not work, we can provideflake-parts-lib.mkDeduplicableModuleOption to create an options of deferred module with an apply function to add the key to the module, equivalent to:

options.myModule1 = mkOption {
  apply = ...
  type = types.deferredModuleWith {
    default = {};
    staticModules = [
      {
        options.myOption = mkOption { default = "my default value"; };
      }
    ];
  };
};
Atry commented 1 year ago

By turning flakeModulesOption into a submodule, we will not need importApply. Instead, we can provide the current inputs from the lexical scope by default

Atry commented 1 year ago

We might want to move apply from flakeModulesOption to user defined modules' apply, because mapAttrs destroys laziness. Providing a flake-parts-lib.mkDeduplicableModuleOption could help user to create an option with the apply function that adds key to the module.

roberth commented 1 year ago

How would mkDeduplicableModuleOption work? Why would it have to be in options instead of config?

mapAttrs destroys laziness.

You might be thinking of mapAttrs' instead. mapAttrs is as lazy as can be. The attrNames are taken from the second argument without calling the function, and the function is only called individually for each attribute. What else could you want? (without changing the attrset data type; strict names, lazy values)

Atry commented 1 year ago

How would mkDeduplicableModuleOption work?

Updated https://github.com/hercules-ci/flake-parts/issues/121#issuecomment-1538846854 to describe mkDeduplicableModuleOption

Why would it have to be in options instead of config?

Because I want to avoid lazyAttrsOf, which would result in infinite loop

Atry commented 1 year ago

Also we can call mkFlake twice to use a flake's own flake module, to address the comment. The first mkFlake call renders the flake module, the second mkFlake call creates its output from the rendered flake module.

Atry commented 1 year ago

If mapAttrs works, we might not need mkDeduplicableModuleOption. Just use mkDeferredModuleOption to create the module and let mapAttrs add keys to them.

roberth commented 1 year ago

I want to avoid lazyAttrsOf, which would result in infinite loop

We have tests for lazyAttrsOf, so you can paint me skeptical. Would love to have a reproducer for whatever would be wrong with it, but I find it unlikely that lazyAttrsOf would be in the critical path. If anything, I would expect the problem to be with config.

I can't verify any of this right now, and these discussions have already taken up a lot of time. I'll have to resume this later, but feel free to try things in the meantime.

Also we can call mkFlake twice to use a flake's own flake module,

Doing it twice smells a lot like we have a need for fixpoint iteration then, and that would not be feasible. I'd prefer to have a very restricted step that only processes flakeModules without the incomplete context of the first step. I would suppose that the incomplete context is unnecessary anyway. But yes, the logic I was proposing for localFlakeModules and all that resembles the flake-parts.flakeModules.flakeModules module a lot. Implementing it with the module system seems like a good idea, but I wouldn't reuse mkFlake for it. (I can sort of imagine perhaps some other "dependency management" options to end up in that little module context, although those are probably not very helpful ones without additions to Nix's flakes feature. Ideally flakes itself would just bear such responsibilities. But let's not get ahead of ourselves on this)

Atry commented 1 year ago

Do you mean we can already reference other modules under the same repository via imports = [topLevel.config.flake.flakeModules.myOtherModule]?

If the claim is true, we should directly close this issue, because referencing a flake module from flake.nix is another problem, which can be solved by calling mkFlake twice.

Atry commented 1 year ago

Doing it twice smells a lot like we have a need for fixpoint iteration then, and that would not be feasible.

Why do you think it's not feasible? Sbt supports recursively solving sbt plugins for sbt plugins according to this document.

Atry commented 1 year ago

I think we don't have to change mkFlake to introduce the fixed point, because the trick on the user end would be good enough. I created #155 to update the document.

Atry commented 1 year ago

It turned out that what resulted in infinite loop is self.outPath, not lazyAttrsOf.

On Mon, May 8, 2023 at 6:08 PM Robert Hensing @.***> wrote:

I want to avoid lazyAttrsOf, which would result in infinite loop

We have tests for lazyAttrsOf, so you can paint me skeptical. Would love to have a reproducer for whatever would be wrong with it, but I find it unlikely that lazyAttrsOf would be in the critical path. If anything, I would expect the problem to be with config.

I can't verify any of this right now, and these discussions have already taken up a lot of time. I'll have to resume this later, but feel free to try things in the meantime.

Also we can call mkFlake twice to use a flake's own flake module,

Doing it twice smells a lot like we have a need for fixpoint iteration then, and that would not be feasible. I'd prefer to have a very restricted step that only processes flakeModules without the incomplete context of the first step. I would suppose that the incomplete context is unnecessary anyway. But yes, the logic I was proposing for localFlakeModules and all that resembles the flake-parts.flakeModules.flakeModules module a lot. Implementing it with the module system seems like a good idea, but I wouldn't reuse mkFlake for it. (I can sort of imagine perhaps some other "dependency management" options to end up in that little module context, although those are probably not very helpful ones without additions to Nix's flakes feature. Ideally flakes itself would just bear such responsibilities. But let's not get ahead of ourselves on this)

— Reply to this email directly, view it on GitHub https://github.com/hercules-ci/flake-parts/issues/121#issuecomment-1539064253, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAES3OSAN2HW6GNCQJRHBIDXFGKJFANCNFSM6AAAAAAVJNYJAI . You are receiving this because you commented.Message ID: @.***>

Atry commented 3 days ago

I updated #197 to utilize partitions to solve this issue. I think this issue can be considered fixed.