lib/plugins: enforce one url per plugin

[MattSturgeon]

As per #3653 (comment)

We already assert that a URL is defined. Now we also asserts that multiple URLs are not defined.

As an example, this is what the errors typically look like:

$ nix eval .#docs
«error: The option `plugins.bullets.url' has conflicting definition values:
- In `/nix/store/2ys1mzni2v8m7j6bxlvn6ib7rx9aj0r8-source/plugins/by-name/bullets': "https://github.com/bullets-vim/bullets.vim"
- In `/nix/store/4bdm5fcwix168v4l6rr9p7a9iafgp6zs-source/pkgs/applications/editors/vim/plugins/generated.nix:1881': "https://github.com/bullets-vim/bullets.vim/"
Use `lib.mkForce value` or `lib.mkDefault value` to change the priority on any of these definitions.
»

[DataHearth]

LGTM! Good feature to compact package definition👌

[daniellaing]

I like this, great work!
Maybe it's worth adding a small descriptive error message to help explain why there are conflicting definitions? Something like "Plugin URL is already definied in nixpkgs, no need to define it here." or similar.

[MattSturgeon]

Maybe it's worth adding a small descriptive error message to help explain why there are conflicting definitions? Something like "Plugin URL is already definied in nixpkgs, no need to define it here." or similar.

Good suggestion. I initially had a more bespoke solution with errors along those lines, but I decided to leverage the module system's merge function instead.

This strikes a balance between not needing to re-implement much logic vs having full control.

Most of the error message comes from the module system, which is why it talks about options and definitions. The parts we can influence without re-writing the merge function are the "locations"; i.e. the "option path" and the "definition locations".

I figured since this is likely to only be seen in new plugin PRs and in lockfile update PRs, the current error is fine.

I did put some effort into getting sane locations for the definitions; e.g. the meta.homepage location is the actual in-store location where homepage is defined in nixpkgs.

EDIT: maybe we could add some "error context" though? This would show up above the error in the stack trace as a "...while evaluationplugins.foo's URL" context line.

[MattSturgeon]

EDIT: maybe we could add some "error context" though? This would show up above the error in the stack trace as a "...while evaluationplugins.foo's URL" context line.

I've just tested this, and using builtins.addErrorContext doesn't help here.

The error gets thrown by the module system from another context, so wrapping addErrorContext around the meta or url attributes doesn't show up in the stack trace, even with --show-trace.

I think the current solution is fine, but as always, if someone has ideas for how to improve it I'd welcome a PR. 😁