pkgs/by-name/README: explicitly suggest version specific override interfaces

[wolfgangwalther]

The current advice of "keeping the override interface" is actively bad, because it hides certain expectations of a package function in an undiscoverable place. Ideally, all information about a package is in one, single place instead.

Version-specific argument names, if required, also have the benefit of creating errors with downstream overrides, much like merge conflicts do. Instead of possibly silently breaking certain behavior, they make a change in expectations clear - which might feel annoying when upgrading, but is ultimately much less problematic down the road.

Resolves #404946, as proposed in #404946 (comment)

Things done

• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[rhendric]

This is non-blocking feedback.

I'm sympathetic to the argument being made here — if a package is known to build with libbar_2 and not libbar_3, it should depend on libbar_2 explicitly and not promise more than it can fulfill — but I think that there are other considerations in some cases. We're currently having a disagreement in #443954 over this for JRE attributes. The JRE is an abstract dependency of some packages (the one I maintain, maptool, downloads a precompiled JAR and runs it with a ‘suitable’ JRE). While it may be that some JREs work better than others, I see that choice as a Nixpkgs integration concern, not as a concern of the specific package. It is an integration concern because the ‘best’ JRE depends on the set of JREs that Nixpkgs makes available — it could even be a JRE provided by a user in an overlay. It is a different situation from a package that only uses API available in libbar_2 and not libbar_3, in that, as the maintainer of the maptool package, I don't know what specifically maptool requires from the JRE provided to it, and I have just gone with something that seems to work. In light of that, it is incorrect and misleading to have the maptool derivation depend on temurin-bin-21, and force users to override that with temurin-bin-21 = some-other-jre;.

Consequently, I think that the documentation should be more nuanced than what is proposed here. Allow for the possibility that depending on an abstract or a concrete package attribute may be appropriate, and that the choice depends on how much the package ‘knows’ about what it needs. I'd even be reasonably happy with language like ‘in most cases’ favoring the concrete attribute, just as long as there's enough wiggle room in the guidelines that nobody on a tree-wide crusade can use it to justify coming for maptool without proposing an equally dependency inversion principle–compliant solution.

[philiptaron]

I agree with the removal of the previous advice. Breaking earlier is better, and while it causes some issues for users, they're good issues to have -- the "merge conflict" metaphor is well-chosen.

What needs more substantiation and advice, in my view, is the "but what if libbar_2 is needed?" question. That very well may be done in a separate PR, but I'm writing it up here.

Too many derivations experience temporary breakage as a result of a version bump then end up pinning older versions for years to sometimes decades. That's the case whether the pinning is done in all-packages.nix, whether it's done in an override, or whether it's done by adjusting the inputs in the callPackage style.

[Frontear]

Agreed with both this PR and @philiptaron mention of needing more clarity surrounding other circumstances that can "break" the interface.

I'd like to see a more universal policy that's applicable for all sorts of derivation changes, so that we can settle the problems regarding interface changes easier. That can come in a different PR discussion of course.

[emilazy]

While it may be that some JREs work better than others, I see that choice as a Nixpkgs integration concern, not as a concern of the specific package. It is an integration concern because the ‘best’ JRE depends on the set of JREs that Nixpkgs makes available — it could even be a JRE provided by a user in an overlay. It is a different situation from a package that only uses API available in libbar_2 and not libbar_3, in that, as the maintainer of the maptool package, I don't know what specifically maptool requires from the JRE provided to it, and I have just gone with something that seems to work.

It seems like the correct thing to do in this case is to just take a dependency on plain jdk. Pinning a specific version should only be done when you do have reason to express a preference – especially pinning a very old version.

I do think there is a legitimate use case for “you can give me whatever X you want and I consider this public API, but in lieu of you doing so I will use this specific X that is distinct from the default Nixpkgs X”, although it is not the common case. In that case, I think we should just document a convention for how to name these parameters to not collide with the global X. Say, in this case, jdk_maptool ? jdk8, “the maptool version of the JDK” (per our conventions, jdk8 should actually be jdk_8, although there is a lot of variance throughout the tree here). Having it in by-name still offers better locality, works better with the merge bot, and gets us further along trimming down all-packages.nix.

[emilazy]

(I think it is unfortunate in general that callPackage/.override couple intentional public API in terms of flags and selected dependencies that expect injection with a “hook into any dependency in ways that are coupled to the implementation details of the derivation” interface that is more like .overrideAttrs‐style monkey‐patching. Both are useful, but it should be clear which one you are using (and we should continue working to remove use of the latter from Nixpkgs itself). But I don’t think changing something as fundamental as that is in scope for this.)

[rhendric]

In that case, I think we should just document a convention for how to name these parameters to not collide with the global X.

That seems like unnecessary churn should we get something like option 3 of #404946, which would resolve the code locality concerns without changing the override interface.

[emilazy]

I believe that the cases where my suggestion is relevant are few, and would essentially be background noise in terms of breaking changes to .override interfaces given that any change to package dependencies almost invariably breaks those. For instance, I don’t think it makes sense for maptool to pin anything other than plain jdk if, as you say, it works with the default and you have no particular insight into what the integration decision should be – and in that case, you do not need to define such a parameter at all, and maptool.override { jdk = …; } works fine to express “instead of the default Nixpkgs integration decision for packages that want a JDK, pick this other one”. We would only need to change the name where a package explicitly anticipates that multiple variants or versions of a dependency may be freely substituted in as public API offered to users, but where the default integration choice in Nixpkgs is not appropriate as a default for that package. Most cases where the latter is true are cases where you do want to explicitly encode that there is a limitation on versions that will work – which is in tension (but not complete contradiction) with the first criterion.

I think there are broader improvements to by-name that could be made, but that there are complicated trade‐offs and we shouldn’t block this on them. I think that your proposal in #404946 (comment) is a bad trade‐off: it does not help address the biggest pain points of the by-name calling interface preventing complete migration, like packages that themselves need to provide multiple versions of themselves or complete package scopes, and it violates the locality principle in the same way as all-packages.nix overrides. For instance, you no longer have any idea what foo is in a package.nix that says { foo, … }: … – something that is currently only violated by the all-package.nix overrides that this PR recommends against.

[rhendric]

I don’t think it makes sense for maptool to pin anything other than plain jdk if, as you say, it works with the default

I didn't say that. The program doesn't reliably open a window when run with jdk, at this time. If this changes in the future, I'd gladly switch to using it.

and you have no particular insight into what the integration decision should be

The trouble is that my ‘particular insight’ is negative, not positive. I don't know that MapTool needs temurin-bin-21; I just know that it needs a JRE and that jdk doesn't cut it. I haven't tested it with GraalVM or Semeru or any other JVMs that Nixpkgs ships, but I support other people trying them out and using them if they like. I selected Temurin because that's what upstream develops against and it works.

So since there's no way for me to express via the callPackages interface ‘jre, but don't use the default jdk’, the best I can do is express ‘jre’ and provide out-of-band advice to use the JRE that upstream develops against.

We would only need to change the name where a package explicitly anticipates that multiple variants or versions of a dependency may be freely substituted in as public API offered to users, but where the default integration choice in Nixpkgs is not appropriate as a default for that package.

Yes, this.

I think that your proposal in #404946 (comment) is a bad trade‐off

I don't want to get too off-topic in this thread; regardless of what I think is best, I believe all of your feedback about my comment applies to the OP's recommendation as well and you should have that debate in that issue if you want.

As far as it is relevant to this thread, I take and agree with your point that the override interface mixes abstract and concrete dependencies, or what you call ‘dependencies that expect injection’ and ‘monkey-patching’, and that this is unfortunate. I would also support a cleaner software-encoded separation between the two. I don't think that it follows that we should ignore the dependency inversion principle and ignore the distinction between the categories ourselves, replacing abstract dependencies with concrete ones in modules (in the software architecture sense, not the NixOS sense) that are loosely coupled to the details of the concrete implementations. I bring up #404946 because it gives us a way to avoid this while improving locality (an adjacent file is more local than all-packages.nix). But if you object to #404946, then I don't think that locality should be used as an argument to sacrifice dependency inversion. The former is an internal issue only, while the latter is an external one that affects the behavior of Nixpkgs as a system that other people use. We should solve our internal code-organizational issues with internal code-organizational solutions, not by changing interfaces, even if the purity of the interface in question is already somewhat suspect.

All that said, again, my feedback is non-blocking, I'm not really expecting to be able to reverse your opinion on this, and I ask only that the official advice leave space for nuance here and not leave the impression that it is now open season on any hand-written dependency injection you see.

[emilazy]

Does my proposal not address your use case, then?

{
  …,
  temurin-bin-21,
  jdk_maptool ? temurin-bin-21,
}:

… jdk_maptool …

I think some churn in the name is acceptable, here, for the benefits of the uniform approach, and to avoid the confusion of a jdk that is not actually our jdk even in its “primary” form.

The trouble is that my ‘particular insight’ is negative, not positive. I don't know that MapTool needs temurin-bin-21; I just know that it needs a JRE and that jdk doesn't cut it. I haven't tested it with GraalVM or Semeru or any other JVMs that Nixpkgs ships, but I support other people trying them out and using them if they like. I selected Temurin because that's what upstream develops against and it works.

This case seems exactly where @wolfgangwalther’s point of “if you are overriding a pinning decision that was made for a reason, it should be clear that you are doing so, and if the situation around that pin changes in the package you should be notified of the ‘semantic merge conflict’ that results so you can decide what to do” is applicable. pkgs.maptool.override { jdk = pkgs.jdk; } looks very different to pkgs.maptool.override { temurin-bin-21 = pkgs.jdk; } – which in this case seems like a good thing, since the former looks like it should be a harmless no‐op but in fact “breaks your warranty”. If it turns out that there are multiple working JDKs and user demand for a defined API for choice between them, a jdk_maptool parameter could be added later to support this flexibility. (Or you could add it now, of course.)

I agree that my opposition also applies to option 3 in #404946, and prefer other solutions to the fundamental problem. I would like disjoint override interfaces for public API knobs and arbitrary dependencies, in time. But I think my proposal here is perfectly fine in the meantime and has distinct advantages over the “external overrides of packages that already exist” approach that @wolfgangwalther seeks to deprecate here.

We do want to have an approach that doesn’t involve all-packages.nix, because all-packages.nix is prone to merge conflicts and cannot work with the merge bot – so there will be an incentive to do things in a way that keeps solely to by-name, regardless. (It’s true that the only PR adjusting such a pin that that could be merged with the merge bot at present would be one opened by a committer, but carefully expanding this kind of ability to distribute maintenance load to non‐committers is part of why we’ve set up the merge bot and had such a large migration to by-name.)

[wolfgangwalther]

The trouble is that my ‘particular insight’ is negative, not positive. I don't know that MapTool needs temurin-bin-21; I just know that it needs a JRE and that jdk doesn't cut it. I haven't tested it with GraalVM or Semeru or any other JVMs that Nixpkgs ships, but I support other people trying them out and using them if they like.

The fact that there are differences, that are relevant to MapTool, between all of these means that the abstraction "any jdk" isn't enough. You need a new abstraction "any jdk_with_certain_properties". Now, if these properties are specific to MapTool, then we end up at jdk_maptool.

It's not a useful abstraction to have, though, because it is not defined anywhere in a way like "X, Y and Z are jdk_maptool".

As long as that's not the case, I think the best thing to do is actually to use temurin-bin-21. It doesn't matter whether there are other jdks that might work. It matters that this does work.

(I'm not even sure whether jdk is the abstraction above, or whether that would be "any JRE", where jdk is only one implementation of it. If that's the case, my case is even stronger, because the abstraction "any JRE" is apparently nowhere defined anway)

I selected Temurin because that's what upstream develops against and it works.

That in itself is quite a strong argument for any kind of temurin-... argument to begin with. "Upstream expects Termurin, but I might override it with something else"?

[wolfgangwalther]

After discussing a bit with @emilazy about that jdk_maptool example, what resonates more with me for this specific case, if exposing temurin-bin-21 feels wrong, is adding withJDK ? temurin-bin-21. Aka, Emily intends this to be part of the explicitly defined public API, while I understood jdk_maptool as still some part of the dependency injection mechanism. Calling it withJDK helps me see it as something else, maybe it helps you, too, @rhendric.

Personally, I think both of temurin-bin-21 directly or withJDK ? temurin-bin-21 as arguments are fine - and both match the suggestion made in this PR.

[Frontear]

If we were ever to go about "formalizing" the override interface, I think we should make it so that only with* style flags are considered stable, everything else is fair game at being changed at any time, based on the derivations needs.

In that sense, for a package needing any JDK, we do:

{
  lib,
  stdenv,
  fetchFromGitHub,

  openjdk21,

  # Stable API
  withJDK ? openjdk21
}:
stdenv.mkDerivation { ... }

This is also nice at formalizing an expected way to override certain things, so that consumers dont have to change their attributes name based on whatever the derivation is doing.

Then further along with this, any changes that change the openjdk21 part should no longer be blocking issues, since they aren't the intended area to override things anyways. A with* flag explicitly indicates to the consumer that changing things around is acceptable, from this entrypoint. If it's not there, then an override is using internal details, and we don't honour stability for them.

[rhendric]

I think we should make it so that only with* style flags are considered stable, everything else is fair game at being changed at any time, based on the derivations needs.

If this (or something like it; there are a lot of enable* and *Support parameters that express the same thing and should perhaps get the same treatment) becomes the documented standard, I would accept it as a compromise.

I would suggest the following, though:

{
   concrete-foo-123 ? null,  # the `? null` here is the new thing
   withFoo ? concrete-foo-123,
}:
# optionally assert that withFoo is not null, unless the derivation can build without a foo
...

The package shouldn't communicate that it has a dependency on concrete-foo-123 when withFoo can be specified instead, for any infrastructure using function argument metadata to discover such things.

[emilazy]

I am okay with withJdk or similar as a colour for that particular bikeshed. (It’s a bit ambiguous since you’d usually expect that to be a boolean meaning “use a JDK at all?”, but one can always disambiguate with withJdkPackage or such.)

I think it’s orthogonal to the specific thing being discussed in this part of the README – I don’t think we want every single version pin to be like this for precisely the reason given here, and many package maintainers prefer to expose fewer intended‐as‐stable‐API flags rather than more, given the support burden of the combinatorial explosion in integration and limits of automated validation – and not in contradiction to it, but we could add a note to the place feature flags are documented to say that they can also be of package type? TBH, we don’t have coherent guidelines on flags anyway, given the wide variation in naming they have. Standardizing that stuff seems like a separate task.

[pbsds]

I'm comfortable with considering the override interface unstable on nixos-unstable, but what about backports to release-xx.yy? I think before we go ahead with this, that we should at least consider if we should update our backport criteria or consider codefying something of a opt-in stable subset of the override interface like #444420 (comment) proposes

[Frontear]

I'm comfortable with considering the override interface unstable on nixos-unstable, but what about backports to release-xx.yy?

Do you mean packages which are updated on master and then backported? I would like to agree that changing the override interface here should constitute a breaking change, but only because of the choice to backport.

[wolfgangwalther]

For the specific case discussed in this issue, which is pinned versions as arguments, I don't think these should be considered breaking for stable either. After all, we're not considering updating everything that has reverse dependencies a breaking change either.

[Frontear]

For the specific case discussed in this issue, which is pinned versions as arguments, I don't think these should be considered breaking for stable either.

Okay yeah you're right, because this circles back to the argument of what we should honour as "official interfaces" for overrides, and that's bogging down this specific issue from moving forward. Deciding the line can't be done in this PR, it's gotta be discussed elsewhere.

[emilazy]

I think we have never worried about backporting a backwards‐compatible “foo: 1.2 → 1.3” commit that happens to drop a dependency on libbar, which is a breaking .override interface change too.

[pbsds]

Just because most of us here have not worried about that in the past does not mean that it was "correct". To be clear, I'm in favor of permitting it, I'm just making sure we fully explore the problem space.

[pbsds]

I'm putting my approval here, because the open question in my comment above may not be actionable

[emilazy]

Right. Well, I think in the limit of “a package cannot stop depending on another package on a stable release, because it’d break overrides”… that’s basically just fully accepting Hyrum’s law as policy, I think. There’s nothing you could do there other than not update the package; the newer version will make any overrides ineffective. We don’t keep .overrideAttrs stable on release branches, either, which is why I think .override regrettably mixes explicit API and monkey‐patching.

Explicit feature flags intended as stable API are things we should be careful about breaking on stable branches, of course, even when those feature flags are of package type. (But if the flag is to enable or disable a dependency, it still may be unavoidable.)

[alyssais]

Ack.

[SomeoneSerge]

Electron is one example of a package that would require an exception to this rule

[wolfgangwalther]

Could you elaborate?

[SomeoneSerge]

So, this is anecdotal evidence of course, but I had been using logseq for a year or two at some point, and there was barely a commit where I didn't have to do something like logseq.override { electron_27 = final.electron; }, each time with a different number. The pin would usually be marked insecure, and whichever version electron would actually be compatible was fairly random.

Sentiment basically is: out-of-tree customization is the one (technical) feature about Nixpkgs that, IMO and only maybe, excuses all our other crimes and debts, so advertising a policy or recommendation that, applied at large, might make it even harder to maintain overlays does not seem like a desirable tradeoff to me.

[wolfgangwalther]

I'm not sure I understand why you think there should be an exception made here. What I understand is that you think logseq had pinned an electron version that was just wrong because it was insecure and/or the package was broken with it. Instead, it could have pinned a different, working version or could have just used electron without a pin.

But.. how is that related to this PR? This just sounds like logseq was not well maintained.

[eclairevoyant]

To put it another way, if logseq supports latest electron, it should just use latest electron instead of pinning. And if it doesn't support latest electron, then the pin is correct, and your override is at least risky - hence the override interface pointing out said risk is appropriate.

Also, upstream itself has so many repeat issues where they are behind on updating the electron version:

• Update to Electron 24 / 25 logseq/logseq#8702
• Electron 25 EOL logseq/logseq#10681
• Electron 27 is end of life logseq/logseq#11303
• Electron 28 has reached end-of-life logseq/logseq#11378
• Electron version in use is outdated and marked insecure logseq/logseq#11644

So if you're unhappy with that, don't use logseq IMO.

I also don't think it's that hard to change one line of code but 🤷

[SomeoneSerge]

So if you're unhappy with that, don't use logseq IMO.

Indeed, I no longer do

I also don't think it's that hard to change one line of code

One line, perhaps. That's why I'm uncomfortable advertising this approach as "preferable"

[emilazy]

Does the withElectronPackage approach discussed above, if agreed‐upon by the package maintainer, not address this?

[SomeoneSerge]

Does the withElectronPackage approach discussed above, if agreed‐upon by the package maintainer, not address this?

It indeed does, but it's still vapourware: we'll do another mass-rename, tamper with the git-blames, deprecate the many withXXXX ? true odd cases, and then find ourselves having to repeat the process once we finally have a tool to replace the convention. Of the suggestions linked from the parent issue so far, the "dream2nix-inspired" deps pattern in #273815 seems like the closest to actually attempting to address the underlying problem.

[wolfgangwalther]

It's the discussion in this PR that started with jdk_maptool, turned into withJDK in my comment in #444420 (comment) and ended up as withJdkPackage in #444420 (review).

[philiptaron]

Assuming there's no overlap with real packages, one way to determine in (say) nixpkgs-vet whether or not there's been a breaking change to the non-dependency-injected interface is to intersect the pkgs with the functionArgs, then subtract that from the functionArgs. What remains is the set of arguments which are part of the stable interface of the package, and can be warned on if changed.

[pbsds]

That's quite a wide net to detect custom args, that assumes the scope is pkgs. And args being custom does not neccesarily imply they are stable. I think it's better to explicitly opt int to considering args stable with a comment.