Skip to content
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.

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

treewide: pin zig package in top-level instead of the function #307268

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

AndersonTorres
Copy link
Member

Description of changes

In order to obtain a more by-name -compatible override interface.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@wegank
Copy link
Member

wegank commented Apr 28, 2024

Merge conflict, please rebase.

Also, should we move zig = zig_0_11; to aliases, so that every zig in Nixpkgs is guaranteed to be overridden?

@AndersonTorres
Copy link
Member Author

Also, should we move zig = zig_0_11; to aliases, so that every zig in Nixpkgs is guaranteed to be overridden?

Since Zig is still too unstable, the rationale we adopted is:

  • each package that uses zig should unconditionally set the suitable version for this package at top-level (as shown in this PR);
    • before the by-name hierarchy, we did it inside the expression. Now with by-name, we should do it outside.
  • as a convenience (for e.g. nix-shell users), the zig handler should be set to the latest version we packaged.

I believe setting zig in aliases.nix will not be a good move, given the second point above.

@Aleksanaa
Copy link
Member

Aleksanaa commented Apr 29, 2024

I personally don't get the need of pinning it this way, and it makes the version used less obvious. As you have stated it is often not the case that the latest zig version works for all packages (right?), so we can just leave it as-is. Also people are less likely to override it (for what?), unlike other runtime dependencies

@AndersonTorres
Copy link
Member Author

I personally don't get the need of pinning it this way

This is to keep the .override interface uniform and predicatble.

"Why a uniform .override interface is useful?"
Because a uniform .override interface is easier to override, facilitating the use of overlays and other methods of customization.

You can look at the Nixpkgs Manual for some interesting examples and concepts:

https://nixos.org/manual/nixpkgs/stable/#sec-overlays-alternatives
https://nixos.org/manual/nixpkgs/stable/#sec-overlays-alternatives-mpi
https://nixos.org/manual/nixpkgs/stable/#usage-1-pkgs.zlib.override

And a code that overrides zig = prev.zig_0_11; is way cleaner than zig_0_9 = prev.zig_0_11;

, and it makes the version used less obvious.

How exactly? The version is clearly stated at all-packages.nix.

As you have stated it is often not the case that the latest zig version works for all packages (right?)

Yes. Zig makes some breaking changes in versions before 1.0, and sometimes the upstream developer did not update his code yet.

Because of it, we had that unwritten rule I wrote above.

Also people are less likely to override it (for what?), unlike other runtime dependencies

I don't think it is a sufficient reason to reject this patch.
The "less likely" people that want to override it will have an annoyance to deal, and well, rejecting minorities' concerns is not a nice thing.

@Aleksanaa
Copy link
Member

Short question: will we have things like callPackage ... { gtk = gtk4; }?


I find you to be much more adventurous than most of us in dealing with such issues. At present, I even doubt if there is a complete consensus on "keep overriding interface", especially in practice. Can we open an issue first to discuss whether this type of modifications are valuable and whether we should enforce them? If you see an example today, change it, and give your reasons, and tomorrow you see another example, things can get even more confusing if the cycle continues.

@Aleksanaa
Copy link
Member

Aleksanaa commented Apr 29, 2024

How exactly? The version is clearly stated at all-packages.nix.

nixos-search and some lsps do not indicate exactly how they are called. And all-package.nix is becoming fat and annoying, as by-name is already actively avoiding it.

The "less likely" people that want to override it will have an annoyance to deal, and well, rejecting minorities' concerns is not a nice thing.

Isn’t it said that zig “breaks often”? Do we really need to take so much care (they can still do if we don't) of users who need to use override to jump between major versions instead of just adding some patches and parameters? I even doubt whether there are really more than 3 such users?

@AndersonTorres
Copy link
Member Author

AndersonTorres commented Apr 30, 2024

Short question: will we have things like callPackage ... { gtk = gtk4; }?

I don't think so. GTK3 and GTK4 are basically different softwares that share a common repo, like Python 2 (abandonware) and 3.

Usually a project needs to keep some abstraction layer in order to support different versions of GTK, and rarely it is possible to compile something which supports both at runtime. It is more typical to have gtk3,gtk4 as build inputs and use some selector like gtkVersion ? "3".


I find you to be much more adventurous than most of us in dealing with such issues.

Thanks!
This is a bug and a feature!

At present, I even doubt if there is a complete consensus on "keep overriding interface", especially in practice.

With Emacs we used some guards in order to warn users about deprecated and renamed parameters.
Hey, the NixOS module system is full of backward-compatibility layers.

And the same by-name hierarchy you cited strives to keep the overriding interface.

Also, the override interface was cited not a long time ago: #300468 (comment)

So I believe that consistency in the names of inputs is a desirable outcome. The whole system is directed towards it.

Can we open an issue first

If the technical arguments given above can't overcome your inertia, what else can?
An issue?
An RFC?
Or maybe an assembly?


nixos-search and some lsps do not indicate exactly how they are called.

is this a bug or a feature?

And all-package.nix is becoming fat and annoying, as by-name is already actively avoiding it.

Yes, and in the foreseeable future this shortcomings will be dealt by by-name too.

Isn’t it said that zig “breaks often”?

Yes, and we took some actions in order to deal with upstreams that did not catch the memos - as we typically do with everything else in Nixpkgs.

Do we really need to take so much care (they can still do if we don't)

"so much"? It is at most seven lines per package, most of them even less.

Also, I am doing this right away here by my own free will, doing that work in advance. Why are you scorning the work I did?

I even doubt whether there are really more than 3 such users?

What is the threshold to overcome that inertia argument? Four? Ten? Maybe a hundred?

P.S.: this is the second time an accepted RFC is treated as nothing.

@AndersonTorres AndersonTorres mentioned this pull request May 1, 2024
13 tasks
Copy link
Contributor

@iFreilicht iFreilicht left a comment

Choose a reason for hiding this comment

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

I agree with this change. It makes the packages fit the by-name guidelines specific mention of this case. As I understand, these were part of the RFC introducing by-name, so I don't think this is the place for re-negotiating that.

I also ran nixpkgs-review on this PR for both x86_64-linux and aarch64-darwin, and no packages were built, proving that no inputs were changed.

Additionally, this blocks #308248, which is part of the 24.05 milestone.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1614

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants