NixOS / nix

Nix, the purely functional package manager
https://nixos.org/
GNU Lesser General Public License v2.1
12.52k stars 1.5k forks source link

Dynamic config options don't work as expected #916

Open danbst opened 8 years ago

danbst commented 8 years ago

Moved from https://github.com/NixOS/nixpkgs/issues/15699

Issue description

I have this error when defining bind mounts for containers:

building Nix...
building the system configuration...
error: dynamic attribute β€˜"/test"’ at /etc/nixos/configuration.nix:82:5 already defined at /etc/nixos/configuration.nix:81:5

Steps to reproduce

First container builds ok, but second results into error.

  containers.test_works = {
    bindMounts."/test".hostPath = "/tmp";
    bindMounts."/test".isReadOnly = false;
    config = {};
  };

  containers.test_doesnt = {
    bindMounts."/${"test"}".hostPath = "/tmp";
    bindMounts."/${"test"}".isReadOnly = false;
    config = {};
  };

But this one builds:

  containers.test_wrks2 = {
    bindMounts."/${"test"}" = {
      hostPath = "/tmp";
      isReadOnly = false;
    };
    config = {};
  };

Technical details

domenkozar commented 8 years ago

cc @shlevy

shlevy commented 8 years ago

Augh, thanks. Will try to look into this soon, but I'm afraid the answer may end up being "it's too complex to do right, sorry".

shlevy commented 8 years ago

OK after looking at this I don't think it's worth the complexity and overhead to do this right, sorry.

copumpkin commented 8 years ago

@shlevy it seems like it would be nice to at least print a warning that it'll behave in unexpected ways, if this pattern appears to get used in the wild.

But at the same time, I wonder if this is a bit like NP problems: some things are hard to do right but it's easy to detect when the hard thing applies. Others are hard to do right and it's hard to even detect when the hard thing applies. If this is the latter I guess the warning might also be hard πŸ˜„ but in the former case, a warning seems vastly superior than silent unintuitive behavior.

shlevy commented 8 years ago

It's not silent, it's an error. And yeah, it's one of those cases where if you do the work to detect it you might as well fix it.

copumpkin commented 8 years ago

Oh sorry, I must've confused my memory of this issue with another one πŸ˜„ an error seems fine then.

shlevy commented 8 years ago

Hmm, actually I think I might see how to do this somewhat simply...

shlevy commented 8 years ago

Nope, never mind :grin:

So, the problem is that the attribute paths a.b.c = 1 is broken down to a = { b = { c = 1; } }; }; (and combined with other relevant paths) at parsing time, but dynamic attributes are resolved at eval time. By the time we know that two dynamic attributes collide, we've lost the nesting structure. We'd have to store that structure in the expression at parse time, and then when we find collisions at eval time have to refer back to the old attributes to see if the structure matches suitably.

copumpkin commented 8 years ago

Oh well!

Ericson2314 commented 8 years ago

This is grossly imperative and I'm not really suggesting it, but can't we arithmetically recur and union like how modules are combined?

shlevy commented 8 years ago

@Ericson2314 Oh, wait, I had in mind that { foo = { bar = 1; }; foo.baz = 2; } would be an error but actually it's not. OK this makes this at least potentially doable... Still guessing it will be too hard for some reason though, will take a look.

shlevy commented 8 years ago

Hmm, not sure how we'd handle the dynamic version of this case:{ foo = { bar = 1; }; foo.bar.baz = 2; }. When the outer set is eval'd, I guess we could have the thunk associated with bar be 1 // { baz = 2; }, but that will be a confusing error message when it fails...

shlevy commented 8 years ago

Actually this fails at parse time, so we can probably check for a literal ExprAttrs, OK: { foo = { bar = 1; }; foo.baz = 2; }

shlevy commented 8 years ago

Ah, the reverse case is an error, so yeah this can't work easily. { foo.baz = 2; foo = { bar = 1; }; } fails.

stale[bot] commented 3 years ago

I marked this as stale due to inactivity. → More info

stale[bot] commented 2 years ago

I closed this issue due to inactivity. → More info

apraga commented 2 years ago

It looks like the issue is still there ? In a flake.nix, there is the same error with

 packages.${system}.default = generateConfig;
 packages.${system}.test= myPackage;

On nix 2.11.

klDen commented 2 years ago

It looks like the issue is still there ? In a flake.nix, there is the same error with

 packages.${system}.default = generateConfig;
 packages.${system}.test= myPackage;

On nix 2.11.

Hey @apraga , to solve this, I merged the two calls into one e.g. in your case:

packages.${system} = {
  default = generateConfig;
  test= myPackage;
};
apraga commented 2 years ago

@klDen That’s what I did. Thanks for the suggestion :)

jaythomas commented 1 month ago

Confirmed this issue still occurs with the current version of nix and nixos.