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

Add writeOnly options to the module system #49766

Closed
wants to merge 1 commit into from

Conversation

roberth
Copy link
Member

@roberth roberth commented Nov 4, 2018

Motivation for this change

I have found a situation, #49765 where it would really help to disallow using certain options outside the defining module.

The value will still be available only as options.foo.bar.internalValue and a options.foo.bar.value or config.foo.bar will yield an error.

The error message is configurable, so the module author can provide a hint of what else the module user should do.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@matthewbauer
Copy link
Member

Private seems kind of vague. How about writeOnly?

@roberth roberth changed the title Add private options to the module system Add writeOnly options to the module system Nov 5, 2018
@roberth
Copy link
Member Author

roberth commented Nov 5, 2018

Done. It mirrors readOnly quite faithfully, except for not being watertight.

@edolstra
Copy link
Member

edolstra commented Nov 5, 2018

The usefulness of this is a bit questionable if any module can still access values via privateValue.

@roberth
Copy link
Member Author

roberth commented Nov 5, 2018

I've updated the PR description to match the rename. It's internalValue now, to reduce the vocabulary size.

The usefulness of this is a bit questionable if any module can still access values via privateValue.

I agree partially. Indeed any module can access the value, but no module author will use internalValue as their first choice to get their info. There module author will have to

  • try config.foo
  • read the error message
  • not be helped or convinced by the message
  • somehow learn about internalValue, digging through the module system, at which point they must already have endured significant pain from their original problem
  • make the choice to ignore all warnings

If someone really ends up there, they probably really needed the value.

If it were easy, I would have written writeOnly to enforce the encapsulation, but it is not. It requires that modules are treated differently, depending on their own options. So it will require more complexity and may have the risk of being more strict, potentially causing infinite recursion problems.

@infinisil
Copy link
Member

Why not just use internal = true;? This way people won't even know about the option because it won't appear in the docs.

@roberth
Copy link
Member Author

roberth commented Nov 5, 2018

Users will need to set it. I have written this for use in config.nixpkgs.localSystem

Edit: It also applies to nixpkgs.pkgs. Module authors should use the pkgs argument instead.

@nbp
Copy link
Member

nbp commented Nov 10, 2018

If I understand correctly, you want to make uses of an option obsolete, and still be able to read its content for the time being?

Any reason not to use mkRenamedOptionModule, to produce the writeOnly error message on use, while reading the definitions from another internal option based on mkAliasDefinitions?

Adding extra attributes to evalOptionValue should be unnecessary nowadays, and would only increase the memory used by NixOS.

@roberth
Copy link
Member Author

roberth commented Nov 10, 2018

This feature works for options that are obsolete, as in config.nixpkgs.localSystem, but also for options usage outside the declaring module is a mistake, like nixpkgs.pkgs.

mkRenamedOptionModule doesn't seem to help much for either of these because it must remain possible for the user to set a value for these. nixpkgs.pkgs was intended to be set by the user and in case of nixpkgs.localSystem the idea was to provide backward compatibility for defining, but to disallow reading because its meaning changes when cross-compiling.

I agree that we should be careful with adding rarely used attributes everywhere in the tree. That should be a good reason to look into a different implementation that avoids internalValue, but instead injects the writeOnly values only into the declaring module. Such an implementation does not require the extra attribute everywhere and enforces the encapsulation, which may be beneficial.

The approach of just breaking config.nixpkgs.localSystem and friends may actually be a better option. Providing the backward compatibility is harder than it seems and will cause the old approach to live on as a bunch of confusing legacy options.

In conclusion, the only valid use case now seems to be config.nixpkgs.pkgs, which is not sufficient for me to write a better implementation for this. I think it is a valid use case and I would like to see a writeOnly or similar flag one day, but that day may not be soon.

@roberth roberth closed this Nov 10, 2018
@roberth
Copy link
Member Author

roberth commented Nov 10, 2018

@nbp Thanks for your good reviews 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants