Module system: detect and report erroneous diamond dependency

[roberth]

Describe the bug

A correct diamond dependency looks like this

    A
  /   \         |  depends on; module A depends on B, etc
 B     C        v
  \   / 
    D

The module system handles that just fine, deduplicating D, based on key or imported file name.

However, in a more distributed setting, it may happen that the intermediate dependencies don't reference the same version of D.

    A
  /   \ 
 B     C
 |     | 
 D1    D2

Depending on the contents of D1 and the very similar D2, this may or may not result in an error.
Specifically, this is a bug when D1 and D2 happen to be mergeable at the module system level, but not in practice; for example if both add package d to an list of packages to be added to a single environment.

In other cases, where the definitions can not be merged, the problem is merely a hard to understand error message. The error will be about some specific option, whereas the root causes is D1 and D2 were expected to be the same, but were not.

Steps To Reproduce

Steps to reproduce the behavior:

1. Use the module system in a distributed context, such as flakes
2. Have a common flake that multiple other flakes will want to use in their modules
3. Combine those other flakes into a single configuration; forget to use follows, or run into the flakes bug where updates don't propagate the way they should.

Expected behavior

A clear error message describing the root cause: D1 and D2 were expected to be the same module but were not. Also suggest a solution or link to a document that explains it.

Towards a solution

It seems that we don't have the right data to act on this today. A module can declare its "role" (D in the example) with key, but the deduplication logic can't detect the conflict, because file could refer to B or C when D happens to be an "anonymous module with key". Maybe this is a module system implementation artifact and not a consequence of the established syntax for modules and therefore solvable entirely within the context of the current module system <-> module interface, but I'm not sure about it.

Screenshots

Additional context

• No great way to close over flake inputs hercules-ci/flake-parts#104
• Flakes: Make avoiding dependency diamonds easier somehow. nix#5570

Notify maintainers

Metadata

Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

[user@system:~]$ nix-shell -p nix-info --run "nix-info -m"
output here

[Atry]

A workaround is to let D be a path instead of a function or a set. It seems that the module system can deduplicate a module that is a path but cannot deduplicate it if it is a function or a set.

[roberth]

Correct.

The module system handles that just fine, deduplicating D, based on key or imported file name.

However, there's a good chance two versions of D will be referenced, in which case the file locations are different and the deduplication will not occur, which leads to errors that do not describe the root cause.

EDIT: updated the description to say not same version of D.

[Atry]

I think it's the final application flake's responsibility to ensure diamond dependencies reference the same copy of the module by specifying follows. However, when the module is a function, it cannot be deduplicated even if the application properly uses follows.

[Atry]

I think the hashed file name approach works as expected and is consistent with other Nix practices, for example, having different paths of the same version of glibc with different settings.

[clhodapp]

If the user "properly" uses .follows, what is there to deduplicate?

[jfly]

@mightyiam and I just ran into a flavor of this diamond dependency issue: our flake has 3 NixOS modules structured like this:

1. Module Common.
2. Module A, which imports Common
3. Module B, which imports Common

The way we have our flake structured results in Common being an "anonymous module with no key". We're getting errors from the nixpkgs module system complaining that Common is declaring options that already exist. The problem goes away if we declare a key in our Common module. I put together a SSCCE here: https://github.com/jfly/nixos-modules-diamond-dependency.

I am left with some questions:

• Should all "anonymous modules" explicitly declare a key?
  • If yes:
    • Is this documented anywhere?
    • Could we make this more discoverable? Perhaps by changing the nixpkgs module system to error out if a key is not specified?
  • If no: what is the right way to solve the issue described in this sample repo?
  • Or perhaps the answer is "ideally no, but it's an unsolved problem"? I think that might be what was discussed over here: lib.importApply: init #230588 (comment).

[roberth]

The downside of adding a key is that it could erroneously deduplicate modules that are actually distinct, leading to only one of them taking effect, silently ignore the other(s). (ie the problem at the start of the issue description)

So it's a tradeoff where we have to try and pick the lesser evil on a case by case basis until we have a mechanism by which this conflict can be detected.

• Should all "anonymous modules" explicitly declare a key?

No, anonymous modules that are only part of the imports of a named or keyed module don't need a key if they are "inline" and not in a let binding somewhere. For example:

# file.nix
{ someFlake }: {
  imports = [
    # These don't need keys
    (mkRemovedModuleOption <...>)
    ({ ... }: { foo = true; })
    
    # These do
    someFlake.modules.foo.bar
  ];
}

• this sample repo?

import anonymizes; it consumes the path and doesn't give it back. Never use it on a module, unless you want to apply a module's effect twice, which would be strange. Just never use it. It's bad for deduplication and bad for error message quality.

importApply does not degrade error message quality, but still does not deduplicate:

# flake.nix outputs, bad
nixosModules.foo = {
  imports = [
    # This might actually work if foo.nix only has mergeable things in it
    # It'd be best to turn foo.nix into a function that's in an attribute, and name it like mkRemovedOptionModule etc.
    (importApply ./foo.nix { inherit self; })
    (importApply ./foo.nix { self = inputs.nixpkgs; })
  ];
}

---

For flakes in particular, I would recommend to always set the file and key generically.
If you use flake-parts, this module sets a file and key for you.

• Is this documented anywhere?

TBD. The reference documentation is already incomplete, and this is an emergent behavior, so yeah. But this needs to be documented 👍
I think what needs to be done is that the module system gets documented with the same tooling as the rest of lib. That way at least the importApply docs are more visible.

$ nix run nix/2.24.8 repl nixpkgs
nix-repl> :doc lib.modules.importApply
Function importApply
[...]

This would at least give a hint, but a section about deduplication or perhaps even best practices would be in order.

[jfly]

Thank you for the quick, and wonderfully complete answer.

TBD. The reference documentation is already incomplete, and this is an emergent behavior, so yeah. But this needs to be documented 👍

Can I help with this? Would https://nix.dev/tutorials/module-system/ be the right place to document it?

[roberth]

As far as I know, we don't have any resources yet for designing and implementing a new module system application, let alone an ecosystem, which is where this comes in.

I guess we could do it there, and perhaps some of it could be in tutorial form, but it's mostly not in actionable form, but rather just informative. This suggests that perhaps the Nixpkgs manual would be a better place for it.

Currently the module system's reference documentation is split between Nixpkgs and NixOS manuals, with little more than evalModules being documented in the Nixpkgs manual. In all fairness an effort to move the docs to Nixpkgs didn't get off the ground, and I'm probably to blame for it, so if you would prefer to position it along the NixOS-based reference content for now, then that's also ok.