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

lib.mkOptions: allow arbitrary option metadata #341199

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

hsjobeki
Copy link
Contributor

@hsjobeki hsjobeki commented Sep 11, 2024

Description of changes

Allows adding arbitrary metadata to options.
@roberth @infinisil I am not sure if this is a good idea. But I could need it to build a nice user interface around the modules.

Examples

foo = lib.mkOption {
      type = lib.types.str;
      meta.required = true;
};

foo-bar = lib.mkOption {
      type = lib.types.str;
      meta.title = "FooBar";
};

foo-bar = lib.mkOption {
      type = lib.types.str;
      meta.renderer = ''
         <input>
      '';
};

Justification for the examples:

(Especially when trying to build a more high level user interface)

  • If a field is required or not might not be derivable from just looking if an option has a default. The value might be set via some config which means it could be optional from the user perspective, although it is required from the evalModules perspective.
  • Options might want to specify how they want to be rendered.
  • Options might be mapped into different domain, which doesn't allow some special characters (-_* \/^) or just where the name of the option might not make sense for a certain user anymore. I.e. title = "FooBar"; would allow to display the option with a more explanatory label without creating an intermediate option, just for the purpose of rendering.
  • ... more examples possible

I am not sure if we should add those use-cases to the current keywords, or if we should just allow arbitrary names because i am not sure how the above described scenarios would ideally be represented through new keywords.

Alternatives

{ ... }@attrs

instead of explicit meta ? {}

passthru instead of meta, maybe it is more clear.

Could produce less thunks but also is less safe towards future extensions of mkOption attrs.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Sep 11, 2024
@roberth
Copy link
Member

roberth commented Sep 11, 2024

I think this is a good direction.
I've previously proposed something highly similar, but didn't get around to building it:

I'm a little concerned with the UX here because the attribute names won't be checked, or the values for that matter.
Perhaps we should typecheck these, and reject unknown/typo ones?
Performance won't be a problem as they don't need to be part of the main evaluation, ie that of config.

For it to remain extensible, the type checking would have to added as part of evalModules, which adds various "runtime" information to the options (such as value) and has access to the _module option namespace that we can use for declaring these types.
It only needs to say something like { meta = mapAttrs (checkOptionMeta config) opt.meta; } somewhere, where checkOptionMeta = config: name: value: if config?_module.metaTypes.${name} then ... else throw ...;.

Or would could remove the mapAttrs and make _module.metaType a types.lazyAttrsOf types.optionType containing a record, taking care of the merge functionality as well.

I think we'll also want to propagate the type information from a module evaluation to all the contained types. We already propagate context like the option prefix, so this would have to follow that pattern. I don't know if this makes the evaluation significantly more expensive. I guess this is the tricky part then.

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Sep 11, 2024

I thought of some freeform meta. As you mentioned in the issue, options cannot accept other labels as you called them. typechecking meta is also not done as of today. The only check is the restriction of the arguments passed to mkOption.

I'm not sure what would happen in

checkOptionMeta = config: name: value: if config?_module.metaTypes.${name} then ... else throw ...;

In the first ... you would check the type of each value?

In the else you wouldn't throw because every other attribute is freeform? Or would you want the user to define the remaining meta types? (type augmentation { internalTypes } & { userDefinedExtension } which means we should never enter the else)

@MattSturgeon
Copy link
Contributor

Perhaps we should typecheck these, and reject unknown/typo ones?

I think it makes more sense to allow any arbitrary meta values to be passed in, since mkOption should not need to know what a particular meta option is or why is was defined.

I thought of some freeform meta. The only check is the restriction of the arguments passed to mkOption.

I think meta being freeform is the correct approach. At most, we could assert that (if present), it is an attrset.

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Oct 9, 2024

@roberth What is missing until we can/want to decide if we want to merge this.
You proposed a roadmap here already: #305741

So should we initially forward the bespoke attributes (description, defaultText, ...) ?

Until we actually use them instead of the old ones. Or whats the plan for moving on with this idea?

@roberth
Copy link
Member

roberth commented Oct 9, 2024

since mkOption should not need to know what a particular meta option is or why is was defined.

It would be up to a few of the many return sub-values of evalModules, not mkOption. It can't even have the knowledge to check it - with that I technically agree; just not with the wider sentiment.
Unchecked attribute names allow typos to be ignored, and we don't allow this in package meta for example.

I'm ok to start free-form, if you think it's ok to introduce

So should we initially forward the bespoke attributes (description, defaultText, ...) ?

I'm not quite certain because I don't have a complete picture for how the migration would go.
My goals with this idea are to improve non-meta performance by reducing some allocations, and to tidy up the data model a bit. The cost-benefit of the latter is questionable, and of the prior it's unknown.

//-ing them in would imply a // operation in the function body of mkOption, where we have no laziness. (Besides the meta // where we do benefit from laziness)
This may have a measurable cost, so I'd like to have a more complete picture before committing to these changes.
Let's postpone that.


So then that means the main code may be ok, but I do have a question. How do you plan to use this field? It doesn't seem that it's propagated to the options and docs generation, or if it is, option declaration won't work.

Could you add a test case showing how you'd like to use this? We have tests in lib/tests/modules.sh with files in lib/tests/modules (and a few in lib/tests/misc.nix iirc)

Could you add documentation in nixos/doc/manual/development/option-declarations.section.md?

@MattSturgeon
Copy link
Contributor

My goals with this idea are to improve non-meta performance by reducing some allocations, and to tidy up the data model a bit.

That makes sense, however I think an additional goal can be to allow module maintainers and/or out-of-tree users to add additional (arbitrary) custom metadata to modules

It would be up to a few of the many return sub-values of evalModules, not mkOption. It can't even have the knowledge to check it - with that I technically agree; just not with the wider sentiment.

Sure, it makes sense that well-known attr names would be type-checked where they are used. Such as asserting that description is a string.

Unchecked attribute names allow typos to be ignored, and we don't allow this in package meta for example.

I think that's inevitable, if we want meta to be freeform. If we want to avoid this then we should avoid it being freeform, though as above I think there's more to gain from it being freeform than not.

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Oct 18, 2024

Okay so I have two motivations for doing this:

  • Performance / houskeeping of the hot path of options.
  • Dumping ground for adding metadata downstream and upstream. (please don't start the old discussion whether all features must be developed upstream - I think it is a good property to offer the flexibility).

My concrete scenario:

I have a set of predefined frontend components (input fields, field arrays, ...).
I can then dump the whole option tree with a function similar to the one that generates our documentation to dynamically build a frontend for the module.

I want the author of a module to define options with added information for rendering and appearance.
Because the option itself does only have a limited set of attributes and they cannot be chosen independently. This causes a tight coupling between the rendering and the module options evaluation and validation. I think decoupling is a good thing here, because not all properties of an option can be translated into appearance properties. In fact for good user-experience most of the time additional handcrafted information is needed. The additional information should probable be of a free-form because it might get very hard to impossible to find a generic abstraction over all rendering use-cases.
I could introduce a custom mkOption function in that downstream project but that would make those modules less portable, and potentially cause confusion.

JobWhenToRun = lib.mkOption {
      type = lib.types.str;
      meta.renderer = ''DateTime"
      meta.minDate = "now"
      ....
};

@roberth
Copy link
Member

roberth commented Oct 18, 2024

  • please don't start the old discussion whether all features must be developed upstream - I think it is a good property to offer the flexibility

That is not at all the discussion. The discussion is whether they should be checked; we've already settled that the declarations won't be exclusively in Nixpkgs.
I guess you weren't notified about #305741 (comment).

@MattSturgeon
Copy link
Contributor

Either meta could be free-form, or it could be checked by "declarations" in, say, _module.optionMeta. That would allow anyone to declare new meta fields, without centralizing them in Nixpkgs.
#305741 (comment) (emphasis mine)

Sounds like we're all on the same page 🙂

I hadn't considered a _module.optionMeta option, but it sounds like a good way to configure an extensible non-freeform (indirectly freeform?) meta attrset 😀

If that is the eventual goal, perhaps it'd be best to have it in place from the start, to avoid any downstream users extending meta without it and later running into breaking changes when it is introduced.

@roberth
Copy link
Member

roberth commented Oct 18, 2024

Came up with an implementation. Could use a few more tests for corner cases and other things we care about, and docs of course. For now I'm curious about the performance impact and whether anything needs to be added to make this practical.

I think inheriting meta attributes into submodules could be nice, but that requires changes to the merge function; same for attrTag support (although something ad hoc could work there too, not sure if it needs to rely on its context for that anyway).

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Oct 22, 2024

Thanks for doing the hard work.

I'd like to try to improve the error messages in this case.

options.foo = lib.mkOption {
  type = types.string;
};
 =>
error: The option `_modules.optionMeta.required' was accessed but has no value defined. Try setting the option.

This is a little bit confusing because the option is missing an attribute to mkOption. A user would never set _modules.optionMeta.required directly.

I tried various combinations of malformed meta options. Most of them produce confusing error messages. Although they might be technically correct.

I am not sure how to fix this, since checkOptionMeta uses evalModules internally, which in turn generates these error messages.

Maybe we need to fix this in a follow up PR. I thought to make evalModules accept additional error context. That can be printed for giving the error message more meaning. Maybe this could improve the overall error message situation, which is one of the weak points with the module system.

@hsjobeki
Copy link
Contributor Author

Also what is more performant?

mergeModules2' = _module: prefix: modules: configs:
vs
mergeModules2' = {_module, prefix, modules, configs}:

Because patterns are extendable with new arguments without breaking backwards compatibility.

… prefix

This feels like cheating, but it works out much better for the
error messages:

    nix-repl> :p (evalModules { modules = [ {
        options.foo = mkOption { };
        options._module.optionMeta.wonk = mkOption { };
      } ]; }).options.foo.meta
    error: The option `options.foo.meta.wonk' was accessed but has no value defined. Try setting the option.
@roberth
Copy link
Member

roberth commented Oct 22, 2024

Nix has optimizations for the positional style, so I would expect _module: prefix: modules: configs: to be faster, unfortunately.
Good thing these are internal.

I've changed the prefix to contain the option path. Feels like cheating, but it results in much better errors:

nix-repl> :p (evalModules { modules = [ {
    options.foo = mkOption { };
    options._module.optionMeta.wonk = mkOption { };
  } ]; }).options.foo.meta
error: The option `options.foo.meta.wonk' was accessed but has no value defined. Try setting the option.

Other error improvements may have to go into follow-ups, but I'm curious what they are and whether this solves a large part of it.

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

Successfully merging this pull request may close these issues.

5 participants