Tracking issue: remove with lib from meta

[lucasew]

Tracking description

Remove with lib from meta sections is a common nitpick that comes into reviews. with is a useful feature but it's overuse can often some silent eval issues that otherwise would be mostly caught with static analysis.

While experimenting with removing it treewide retroactively, I created #371665 which is too big to merged in one go so this issue was created to track the dismembering of that big PR into smaller human reviewable PRs.

The whole effort can be separated in the following stages:

• Add a CI workflow to find common issues around this. introduce npv_169 to detect top level withs nixpkgs-vet#142
• Retroactively migrate the whole repository in small increments using the chungus PR as a starting point.

Reference Issue(s)/PR(s)

• ...

Follow-up issues/notes

Additional context

Add any other context about the problem here.

---

Add a 👍 reaction to issues you find important.

[ethancedwards8]

What about using with lib.maintainers; for the meta.maintainers list?

[lucasew]

What about using with lib.maintainers; for the meta.maintainers list?

For these trivial cases where you use with to define a list it's fine.

The problem is when you add basically a with with an attrset inside, or with nested structures (that's how I am modeling the lint rule until I can think on something better.

BTW the chungus PR is not meant to be merged at once but be dismembered into small PRs that anyone can use the chungus to have a starting point and tweak possible issues.

The chungus approach was much simpler to flow through the process. I can in one hour edit like thousands of files and use substitution (I copypasted substituteInPlace to my bashrc-like scheme) for some simple replacement scenarios.

[Frontear]

In my honest opinion I don't find this something worth nit-picking over. The common complaint against using with is that it makes attribute use ambiguous for static analysis, and that's a very good argument when we consider using it in a global level (such as with lib; as the top of a nix file). I am in full agreement that we should NOT accept such a thing, but meta is a small attribute set block that does not cause any large issues.

I think the usage of meta = with lib; {} should be something left to the maintainer/author of the derivation. I do not think it's something that should have a specific requirement, especially because keeping or removing it has no practical benefit overall.

[SuperSandro2000]

Also such treewide changes always have the potential to cause eval issues and create merge conflicts.

[Atemu]

If I'm refactoring a derivation anyways, I typically remove such cases but I don't think this needs a treewide because it just simply doesn't cause any concrete issues.

There's much more pressing issues that we should spend our time on.

[niklaskorz]

One of the main issues I see right now, that has been emphasized by the creation of #373573 and #373548, is that there are still more occurences of meta = with lib; { ... } than the now preferred pattern.
This can be a source of frustration for new contributors, who either learn by reading existing package definitions or just wanted to do a simple update PR that didn't even touch the meta section.
I think it's fine to point out in bigger refactorings of a package, and also of course for completely new packages, but a treewide change would be benefitial in providing improved learning material and removing this nit from PRs that are unrelated to where the with lib; occured.

As mentioned here by other comments, the alternative to a treewide would be to resist the temptation to request a removal of with lib; on every opportunity and simply refactoring it yourself when you come accross it in your own PRs.

[lucasew]

The ratchet check I am introducing into nixpkgs-vet would already let the user know if new occurences of a open ended with is introduced, so in a init scenario it would already fail CI. For existing packages it would be green, like how it's happening with pkgs/by-name.

This, as discussed in nixpkgs architecture, is already the expected behaviour so it seems the check is ready.