vimUtils.buildVimPlugin: hard error if called without pname+version

[SuperSandro2000]

Since #193323 this is required otherwise building the wrapper for vim/neovim fails with a very hard to debug error.

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• 22.11 Release Notes (or backporting 22.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
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[MangoIV]

you didn't change the documentation accordingly, also this is apparently a duplicate of my PR

[MangoIV]

I also don't agree with erring when calling without version because not all calls to this function are generated.

[SuperSandro2000]

pname should always be used in combination with version. Only supplying pname is not something that is usually supported and mkDerivation would error inside of nix when done.

[lilyball]

Instead of requiring pname/version as inputs, can we please just use lib.getName/lib.getVersion as necessary? The only reason for requiring pname/version up front that I can think of is worries that future refactoring will reintroduce new references to drv.pname/drv.version.

If you are intentionally trying to guard against that, we could just synthesize pname/version for the call to mkDerivation if necessary. However, this will mean that the derivation could end up with version = "" which is a bit odd, so I'm not terribly fond of this. We could instead provide just name + pname if the user supplies pname alone, which shouldn't cause a problem in mkDerivation (at a glance, mkDerivation only requires version if pname is supplied and name is not). In this case providing name directly is better than providing version = "" as the latter will synthesize the name like "${name}-".

This could look something like

let
  version = attrs.version or (lib.getVersion name);
  drv = stdenv.mkDerivation (attrs
    # allow specifying name or pname alone
    // (if attrs?name then { name = namePrefix + name; }
      else if !attrs?version then { name = namePrefix + attrs.pname; }
      else {})
    // (lib.optionalAttrs (version != "") { inherit version; })
    // {
      # ensure the resulting derivation always has a pname
      pname = namePrefix + attrs.pname or (lib.getName name);
      # …

This way the resulting derivation will include pname always, but it will be generated from name if not otherwise provided. version will only be defined if provided or included in name, so that way we don't end up with drv.version == "" (and so far nobody seems to care about the version of vim plugins).

[lilyball]

Having said all that, I'm still more in favor of just "always use lib.getName/lib.getVersion", because it feels odd to me that saying

buildVimPlugin {
  name = "foo";
  inherit src;
}

would produce a derivation with

{
  name = "foo";
  pname = "foo";
}

Perhaps there's precedent for this, for derivations synthesizing a pname when not otherwise provided, but if so I'm not aware of it.

[lilyball]

It occurs to me that probably the best solution is to just add a vim test that constructs a plugin with only name. This should catch future refactors that reintroduce references to drv.pname and avoids unnecessary complexity.

[SuperSandro2000]

I also don't agree with erring when calling without version because not all calls to this function are generated.

Yes, this is intended. When you right now don't pass pname+version you get an error with no trace where the error is created.

Instead of requiring pname/version as inputs, can we please just use lib.getName/lib.getVersion as necessary?

This would need to be changed in packdir but for that I would like feedback from @teto which I can't get because he has blocked me which breaks collaboration.

This could look something like

The code example is way to complex and complicated for what we are doing here.

It occurs to me that probably the best solution is to just add a vim test that constructs a plugin with only name. This should catch future refactors that reintroduce references to drv.pname and avoids unnecessary complexity.

We have that, kinda. https://github.com/NixOS/nixpkgs/pull/193323/files#diff-ef18e443be3c1c6d3542cac9e3c5ed844ef3f3c4a6d16dbd17bd82f9dcc5e5caR76

---

Apparently this was already fixed in #194483 but no on cared to make me aware of that.