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 1 commit 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.

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