Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wolfgangwalther merged 1 commit into from
Sep 21, 2025
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions pkgs/by-name/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,16 +62,12 @@ The above expression is called using these arguments by default:
```

But the package might need `pkgs.libbar_2` instead.
While the function could be changed to take `libbar_2` directly as an argument,
this would change the `.override` interface, breaking code like `.override { libbar = ...; }`.
So instead it is preferable to use the same generic parameter name `libbar`
and override its value in [`pkgs/top-level/all-packages.nix`](../top-level/all-packages.nix):
While the `libbar` argument could explicitly be overridden in `all-packages.nix` with `libbar_2`, this would hide important information about this package from its interface.
The fact that the package requires a certain version of `libbar` to work should not be hidden in a separate place.
It is preferable to use `libbar_2` as a argument name instead.

```nix
{
libfoo = callPackage ../by-name/so/some-package/package.nix { libbar = libbar_2; };
}
```
This approach also has the benefit that, if the expectation of the package changes to require a different version of `libbar`, a downstream user with an override of this argument will receive an error.
This is comparable to a merge conflict in git: It's much better to be forced to explicitly address the conflict instead of silently keeping the override - which might lead to a different problem that is likely much harder to debug.
Comment on lines +69 to +70
Copy link
Contributor

@SomeoneSerge SomeoneSerge Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@wolfgangwalther wolfgangwalther Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate?

Copy link
Contributor

@SomeoneSerge SomeoneSerge Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@wolfgangwalther wolfgangwalther Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@eclairevoyant eclairevoyant Sep 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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 🤷

Copy link
Contributor

@SomeoneSerge SomeoneSerge Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Member

@emilazy emilazy Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@SomeoneSerge SomeoneSerge Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

@wolfgangwalther wolfgangwalther Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


## Manual migration guidelines

Expand Down
Loading