Documentation: style guide for use of with

[lolbinarycat]

Problem

i see a lot of license = with licenses; [ mit ] in PRs recently.

compared to the simpler license = licenses.mit, this is both longer and more syntactically complex, taking longer to read for humans, and possibly longer to evaluate too.

Proposal

this should be avoided for single-license packages (which is most of them).

this would not apply to meta.maintainers, for the following reason:

• it is more likely to have multiple values
• it is much more likely to have extra values added in the future.
• maintainers is plural, which implies a list

i would be willing to write this into pkgs/README.md but i'm not going to open a PR until i've gotten some feedback, it wouldn't make sense for me to single-handedly decide what the best practice is.

---

Add a 👍 reaction to issues you find important.

[eclairevoyant]

relevant: #208242

[lolbinarycat]

the nix best practices page seems to imply that with should basically never be used, this seems very extreme and would require rewriting basically every file in nixpkgs.

if we instead just recommend limiting its use to meta = with lib; { } and maintainers = with maintainers; [ ], that would allow most unproblematic usages of with to continue, while discouraging unusual and sometimes problematic usages.

[AndersonTorres]

i see a lot of license = with licenses; [ mit ] in PRs recently.

In my opinion, license should be a list, rather than "a list of licenses or a single license". The "singleton", even if being more typical, looks like an exception and not a rule.

So, as a personal preference, I am writing meta.license as a list unconditionally.

maintainers is plural, which implies a list

License should be called licenses, precisely because there is at least one multiple-licensed project.
But now it is too late to change that historical inconsistency...

seems to imply that with should basically never be used, this seems very extreme and would require rewriting basically every file in nixpkgs.

There is more: we didn't find a decent replacement for with yet. The best we can do is to limit its use to predictable and easy-to-not-be-fooled examples.

To me, the exceptions are single-line statements, like:

1. Lists, with the restriction that the with applies to each and every element:

      with X; [ A B C D ];
      # is shorter than
      [ X.A X.B X.C X.D ];
   
      # however, if X.C and X.D does not exist, use:
      (with X; [ A B ]) ++ [ C D ];
     # equivalent to
      [ X.A X.B C D ];

2. the infamous toPythonApplication:

      aocd = with python3Packages; toPythonApplication aocd;
      # is shorter than
      aocd = python3Packages.toPythonApplication python3Packages.aocd;

meta = with lib; { }

This was discouraged too.

#293767

[lolbinarycat]

This was discouraged too.

correction: this is no longer encouraged.

as best i am able to tell, the only person who has a major problem with meta = with lib; is you. i am trying to document consensus, and one person's opinion does not consensus make.

i can drop meta = with lib from "acceptable" to "discouraged" if you find someone to second your opinion, i suppose.

[Aleksanaa]

correction: this is no longer encouraged.

as best i am able to tell, the only person who has a major problem with meta = with lib; is you. i am trying to document consensus, and one person's opinion does not consensus make.

I made a similar point here: #292120 (comment)

To put it bluntly, this needs to be discussed. We cannot force change through partial adoption of previously accepted views without discussion. As far as the previous PRs and issues I have seen, other committers have expressed objections. Relevant changes should not be merged until controversy is settled.

[Atemu]

I've also yet to see a good reason why the 5 lines of the (usually trivial) meta attribute are somehow too large of a scope to sensibly apply with.

For entire package definitions or modules, that reason is easy to find as their code usually is not trivial where non-intuitive with; bugs can creep up comparatively easily. (I've been bitten by a file-global with; triggering infinite recursion a few weeks ago again.)
For rather small trivial scopes such as individual lists or a small attrset, this reason does not apply.

[AndersonTorres]

the 5 lines of the (usually trivial) meta attribute

1. Technically the current meta recognizes 10 sub-attributes;

2. "It just works" is not a serious reason to make an exception to nested with;

3. The argument can be reversed: if the scope is so small, then there is no tactile gain in squeezing some bytes in it;

4. Further, it brings consistency to the use of with, avoiding arguments of "small enough":

   • No nested with
   • One line is small, two or more is big.

5. Being blunt, I have seen no reason for the guidelines above, besides "saving 10 characters per line"

   • it would be no more than 100 bytes per file (per worst hypothesis usage on meta attrs)
   • or less than 8MB on the whole repo (worst hypothesis usage of each and every entry of find ./my-local-clone-of-nixpkgs).

[lolbinarycat]

@AndersonTorres trying to convince me is pointless, because it doesn't matter what i think. i'm trying to document consensus, not my own opinion.

(for the record, my only strong opinion is that with lib.maintainers should be allowed)

[AndersonTorres]

1. You opened a PR associated with this issue (doc: add styleguide on use of with to CONTRIBUTING.md #294383) - and in ready-for-review status, rather than as a draft.

   You even marked it as closing this issue.
   You are being inconsistent here.

2. It makes few to no sense to document consensus without polling opinions.

3. Many posts here - including those you are 👍-ing - express opinions and arguments, but only now that I am answering and arguing on them you suddenly remember you are merely documenting consensus?

[lolbinarycat]

You opened a PR associated with this issue (#294383) - and in ready-for-review status, rather than as a draft.

yes, because i want feedback on it (i.e. review)

It makes few to no sense to document consensus without polling opinions

correct! that's why i opened this issue before creating the PR!
i also asked peoples' opinions in the matrix dev chatroom.

all of the actions i have taken i have done with the intent to get other people's opinions.

Many posts here - including those you are 👍-ing - express opinions and arguments, but only now that I am answering and arguing on them you suddenly remember you are merely documenting consensus?

i want to get several different perspectives on the issue.

they were a new voice, which is helpful

i am already well aware of your opinion, and you continuing to repeat it does nothing to establish consensus.

[eclairevoyant]

I've finally found a good example: version can come from both lib and the derivation itself.

This has now reared its head in an actual package (see https://github.com/NixOS/nixpkgs/pull/298316/files) where someone converted the mkDerivation call to use the fixed-point parameter (mkDerivation (final: { ... })), but missed changing one of the references to version within meta:

  meta = with lib; {
    changelog = "https://gitlab.archlinux.org/pacman/pacman/-/raw/v${version}/NEWS";
  };

which in turn broke the changelog. No eval error, because version easily came from lib.

For another example where this would cause further issues, version is an input for meta.platforms in citrix-workspace:

  meta = with lib; {
    platforms = [ "x86_64-linux" ] ++ optional (versionOlder version "24") "i686-linux";
  };

In other words, were this converted to use the fixed-point parameter, and someone missed a spot to use said param, this would break meta.platforms (!)

And to someone unaware of the scoping rules of with, they may easily miss that version is not meant to come from lib, nor will the LSP or even nix itself warn them of this.

Perhaps it's time to put the footgun down and get rid of meta = with lib; too.

[Aleksanaa]

Good catch!

And to someone unaware of the scoping rules of with, they may easily miss that version is not meant to come from lib, nor will the LSP or even nix itself warn them of this.

The rewritten nixd will probably be able to warn this!

[AndersonTorres]

I don't know if I am more impressed with such an awful example of unexpected with, or with Pacman in Nixpkgs.

[lolbinarycat]

well, i stand corrected!

guess it's time to discourage with lib in meta.

[RossComputerGuy]

guess it's time to discourage with lib in meta.

I'm still on the fence with the removal of with lib in meta. In the example which was given, couldn't you have version' and lib.version to explicitly state which ones? Or you could use let..in and inherit (lib) ...? Imo, the removal of with lib; in meta and leaving everything to have the "long form" of pulling data does not look as clean as the with lib; way.

[eclairevoyant]

I cannot get behind making packaging harder if the only potential benefit is it (sometimes) looks "cleaner".

As for using let exprs, that still entails removing with.

[RossComputerGuy]

I cannot get behind making packaging harder if the only potential benefit is it (sometimes) looks "cleaner".

Understandable, though do we have to remove all with lib;'s when setting meta? In the example you gave, there's two version variables. Not all packages will have duped variables. With the move to what #119942 implemented, maybe we could still use with lib; as there would be finalAttrs.version and so lib.version wouldn't collide. With smaller or simpler packages, maybe we could still use with lib; and if issues do arise then we could make the change?

[AndersonTorres]

does not look as clean as the with lib; way.

NixOS/rfcs#110 is trying something to solve this

[eclairevoyant]

It seems the check added in #330066 will get us closer to cleaning up meta = with lib; 🎉

[infinisil]

Unfortunately that specific error reporting is turned off again for now, see #330454 😅