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

nix: 2.18 -> 2.22 #315262

Merged
merged 1 commit into from
May 29, 2024
Merged

nix: 2.18 -> 2.22 #315262

merged 1 commit into from
May 29, 2024

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented May 28, 2024

I tested this version on my system without any issues, but on the other hand it has better error handling i.e. for infinite recursion.

This version has been now also cooking in the nix-install-action for 2 weeks: cachix/install-nix-action#206

Description of changes

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.11 Release Notes (or backporting 23.11 and 24.05 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.

@Mic92 Mic92 requested review from RaitoBezarius and Ma27 as code owners May 28, 2024 06:20
@Mic92 Mic92 removed request for RaitoBezarius and Ma27 May 28, 2024 06:36
@Mic92
Copy link
Member Author

Mic92 commented May 28, 2024

Removed request from Raito and Ma27, because they now switched to lix.
@Ericson2314 should we add the nix core team back as reviewers?

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels May 29, 2024
@paparodeo
Copy link
Contributor

I tested this version on my system without any issues,

one issue i noticed is when using :e in nix repl the screen of my editor (vim) is not correctly drawn when moving the cursor.

@Mic92
Copy link
Member Author

Mic92 commented May 29, 2024

I tested this version on my system without any issues,

one issue i noticed is when using :e in nix repl the screen of my editor (vim) is not correctly drawn when moving the cursor.

This is fixed in master: NixOS/nix#10617
I cherry-picked the fix in 2.22 now as well.

@paparodeo
Copy link
Contributor

I tested this version on my system without any issues,

one issue i noticed is when using :e in nix repl the screen of my editor (vim) is not correctly drawn when moving the cursor.

This is fixed in master: NixOS/nix#10617 I cherry-picked the fix in 2.22 now as well.

Thanks! looks like NixOS/nix#10617 was reverted in favor of NixOS/nix#10754

@ofborg ofborg bot requested a review from lovesegfault May 29, 2024 13:39
I tested this version on my system without any issues, but on
the other hand it has better error handling i.e. for infinite recursion.

This version has been now also cooking in the nix-install-action for 2
weeks: cachix/install-nix-action#206
Copy link
Contributor

@paparodeo paparodeo left a comment

Choose a reason for hiding this comment

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

tested nix repl works great! thanks!

@lovesegfault lovesegfault merged commit 7225a63 into NixOS:master May 29, 2024
23 of 24 checks passed
@K900
Copy link
Contributor

K900 commented May 30, 2024

This regressed the misc test: https://hydra.nixos.org/build/261759976

@mweinelt
Copy link
Member

Which blocks the unstable channels and should be fixed asap.

@RaitoBezarius
Copy link
Member

Mistakes are fine but I told about the misc tests so many times about the Nix maintenance and I am quite disconcerted by this happening so obviously again.

If Nix maintenance cannot have a reasonable enough QA, it's OK, there's no shame to that. In that case, I advise to switch the default implementation of Nix in Nixpkgs to be Lix, we are old Nix maintainers in Nixpkgs and we have a bit more experience on how to run this.

We are back to pre-code ownership QA levels now, this is not good :/.

cc @fricklerhandwerk

@kanashimia
Copy link
Member

kanashimia commented May 30, 2024

Newer Nix also broke nix edit, it now doesn't work without adding --impure, which was extremely annoying to me. At least they unbroke nix shell n#gcc in Nix 2.22 compared to 2.21.
Noting this as it wasn't mentioned.

@Mic92
Copy link
Member Author

Mic92 commented May 30, 2024

Mistakes are fine but I told about the misc tests so many times about the Nix maintenance and I am quite disconcerted by this happening so obviously again.

Critics taken, this was my first time bumping nix and it wasn't clear to me that misc is a test needed for nix. I now added to passthru tests: #315878. Any other NixOS tests we should take into account?

If Nix maintenance cannot have a reasonable enough QA, it's OK, there's no shame to that. In that case, I advise to switch the default implementation of Nix in Nixpkgs to be Lix, we are old Nix maintainers in Nixpkgs and we have a bit more experience on how to run this.

I would like to ask you not to cause further rift in the community. Calling Nix developer incompetent is helping no one.

We are back to pre-code ownership QA levels now, this is not good :/.

cc @fricklerhandwerk

@Mic92
Copy link
Member Author

Mic92 commented May 30, 2024

Newer Nix also broke nix edit, it now doesn't work without adding --impure, which was extremely annoying to me. At least they unbroke nix shell n#gcc in Nix 2.22 compared to 2.21. Noting this as it wasn't mentioned.

Thanks. I wasn't aware of NixOS/nix#9652. I look into it.

@RaitoBezarius
Copy link
Member

Critics taken, this was my first time bumping nix and it wasn't clear to me that misc is a test needed for nix. I now added to passthru tests: #315878. Any other NixOS tests we should take into account?

No problem, but please coordinate with the Nix maintenance team, no offense to you, Mic, but I am tired of the disorganized situation where I repeat things over months multiple times to different people.

In general, upgrading the stable version of Nix is a very involved operation and maybe people should first work on writing a proper checklist before upgrading it, I am sad this didn't happen even though we discussed it in the Nix maintenance team meeting to the best of my knowledge.

I would like to ask you not to cause further rift in the community. Calling Nix developer incompetent is helping no one.

This N-th untested merge is causing further rift, unfortunately. It's hardly respectful to people's time after a327fd0 and so many discussions in those sorts of PRs and the powerless feeling that no matter what you say, it seems like it is absolutely not heard or considered in any meaningful way.

I am not calling Nix developers incompetent, Nix developers mentioned multiple times they did not have the bandwidth to take care of the QA process for Nix in Nixpkgs, I am just saying it is hard to hear: "We cannot take care of it" and "We will just yolo merge things" at the same time, I am offering an honest alternative option: just do not advertise something you cannot take care of. So please do not put words in my mouth about things I didn't say.


What I am asking for, which I think is reasonable, for the past months, is to stop breaking people's package manager when it could have been avoided. If that is not possible, this will cause further rift, yes.

@vcunat
Copy link
Member

vcunat commented May 30, 2024

This also broke devenv, a nixpkgs-unstable channel blocker. Now this PR is reverted, so it's not urgent, but I expect the update will be retried eventually. /cc maintainers: @domenkozar, @drupol

@Mic92
Copy link
Member Author

Mic92 commented May 30, 2024

No problem, but please coordinate with the Nix maintenance team, no offense to you, Mic, but I am tired of the disorganized situation where I repeat things over months multiple times to different people.

Who is the Nix maintenance team? I only see ma27 and you as codeowner.

@RaitoBezarius
Copy link
Member

No problem, but please coordinate with the Nix maintenance team, no offense to you, Mic, but I am tired of the disorganized situation where I repeat things over months multiple times to different people.

Who is the Nix maintenance team? I only see ma27 and you as codeowner.

https://nixos.org/community/teams/nix/ AFAIK, we were just Nix maintainers in Nixpkgs to help with the current state of things, but I hope that the Nix maintenance team gets more involved to provide a polished experience to everyone involved.

@Mic92
Copy link
Member Author

Mic92 commented May 30, 2024

In general, upgrading the stable version of Nix is a very involved operation and maybe people should first work on writing a proper checklist before upgrading it, I am sad this didn't happen even though we discussed it in the Nix maintenance team meeting to the best of my knowledge.

You must have this checklist than. Could you give it to me? Otherwise I need to approximate to my best abilities.

@Mic92
Copy link
Member Author

Mic92 commented May 30, 2024

In general, upgrading the stable version of Nix is a very involved operation and maybe people should first work on writing a proper checklist before upgrading it, I am sad this didn't happen even though we discussed it in the Nix maintenance team meeting to the best of my knowledge.

You must have this checklist than. Could you give it to me? Otherwise I need to approximate to my best abilities.

#314065 (comment)

@Mic92
Copy link
Member Author

Mic92 commented May 30, 2024

@Ma27 are you still maintaining nix in nixpkgs or not?

@RaitoBezarius
Copy link
Member

In general, upgrading the stable version of Nix is a very involved operation and maybe people should first work on writing a proper checklist before upgrading it, I am sad this didn't happen even though we discussed it in the Nix maintenance team meeting to the best of my knowledge.

You must have this checklist than. Could you give it to me? Otherwise I need to approximate to my best abilities.

Yes, of course, Nix maintenance in Nixpkgs starts where Nix maintenance testing stops, i.e.

  • Build over all supported systems
  • Build static binaries and test them
  • Cross compile to classical supported systems
  • Run i686, misc tests & all installer tests using that specific Nix
  • Run library eval tests (normally, ofborg takes care of this for you, but double check it)
  • Verify that nixos-enter behave as expected (a catastrophic regression for the stable Nix in general)
  • Build Hydra with that Nix and verify it works out of the box
  • Verify that the new Nix's fetchers work with things like npins, with some interesting cases, e.g. private forges, allRefs = true;, etc.
  • Evaluate ALL of nixpkgs with nix-eval-jobs which tests two things: nix-eval-jobs compat and Nixpkgs evaluation ~correctness

I do not care about Flakes personally, so I don't do any special regression testing on this, but I would imagine that some people would do so.

I do some special tests based on my own infrastructure and my own model of the current type of regressions that Nix are creating:

  • Garbage collect previous Nix stable version's store and verify it terminates in reasonable time and does not encounter any regressions in terms of "valid store paths" (due to the recent leading dot store path filtering issue)

And that's what I can come up with, on the spot, as I am at GPN right now. A generic checklist can start like this, each Nix stable bumps are special because reading the release notes are critical to have ideas of "what to test". Reviewing the issue list with the tag bug is also important to see what are release blockers bugs or not, IMHO. With that in mind, the generic checklist can be refined to a specialized checklist to a specific bump.

@edolstra
Copy link
Member

In that case, I advise to switch the default implementation of Nix in Nixpkgs to be Lix, we are old Nix maintainers in Nixpkgs and we have a bit more experience on how to run this.

No, this is NixOS, not LixOS. You are free to create your own distro, of course.

I think it would be good for the Nix team to take ownership of maintaining the Nix package in Nixpkgs, but that should be discussed in the team.

It might not be realistic to make the maintainers of the Nix package responsible for every breakage in a dependent package. E.g. in the case of devenv breaking because of a patch that no longer applies, I feel that's really the responsibility of the devenv package maintainer.

Verify that nixos-enter behave as expected (a catastrophic regression for the stable Nix in general)

I believe this is covered by the NixOS installer tests.

@Ericson2314
Copy link
Member

I don't think it's fair to dogpile on @Mic92 when a bunch of people who were doing this are stepping down. It's reasonable, if not ideal, that human knowledge gets lost and rediscovered in the process.

It may even be that bumping Nix eagerly is a good thing --- so more people are mad at the Nix team for whatever breakage there is, provided that it doesn't grind Nixpkgs itself to a halt too much. Some bugs do the latter for sure, but other bugs just do the former.

@Ma27
Copy link
Member

Ma27 commented May 30, 2024

It may even be that bumping Nix eagerly is a good thing --- so more people are mad at the Nix team for whatever breakage there is, provided that it doesn't grind Nixpkgs itself to a halt too much

Just for the sake of completeness, for that we do have nixVersions.latest & even nixVersions.git.
The latter is advertised for precisely that in 24.05 btw :)

@Mic92
Copy link
Member Author

Mic92 commented May 30, 2024

It might not be realistic to make the maintainers of the Nix package responsible for every breakage in a dependent package. E.g. in the case of devenv breaking because of a patch that no longer applies, I feel that's really the responsibility of the devenv package maintainer.

My suggestion is to keep a list of consumers to ping, when upgrading the nix package manager. I for instance often use these notifications to know when I have to re-check my own packages that have tight coupling with nix as a build dependency i.e. nix-eval-jobs and harmonia.
@domenkozar and @oxalica should be probably also on this list.

@edolstra
Copy link
Member

We could also set up a small Hydra jobset for testing Nix upgrades, similar to release-small.nix. That jobset could include any reverse dependencies about whose breakage we might want to be notified, like devenv.

@RaitoBezarius
Copy link
Member

No, this is NixOS, not LixOS. You are free to create your own distro, of course.

Having Lix as a default implementation does not change the fact this is NixOS, it's just not using NixOS/nix as an implementation. I do not see the problem, this is still Nix. It would be nice to provide clear arguments on why this would be a problem.

I think it would be good for the Nix team to take ownership of maintaining the Nix package in Nixpkgs, but that should be discussed in the team.

I think this was submitted multiple times to the team and I thought that @fricklerhandwerk was considering taking the ownership. I am not sure what is the blocker here.

It might not be realistic to make the maintainers of the Nix package responsible for every breakage in a dependent package. E.g. in the case of devenv breaking because of a patch that no longer applies, I feel that's really the responsibility of the devenv package maintainer.

We could also set up a small Hydra jobset for testing Nix upgrades, similar to release-small.nix. That jobset could include any reverse dependencies about whose breakage we might want to be notified, like devenv.

There's plenty of ways to do it and you just mentioned one. Ideally, we review the reverse dependency closure and try to be reasonable to mention breakages to dependants. In this case, this is a release blocker, so this is a non-discussion. A release blocker needs to be tested before the PR is merged?

I believe this is covered by the NixOS installer tests.

It tests quite basic feature, i.e. building a non-nixpkgs crafted derivation. There's room for a bit more coverage, with a UX orientation, i.e. "can I recover my NixOS system using that new Nix?" which is one clear regression we had in the past and took months to fix and we shipped a release of NixOS without this feature.

I don't think it's fair to dogpile on @Mic92 when a bunch of people who were doing this are stepping down. It's reasonable, if not ideal, that human knowledge gets lost and rediscovered in the process.

I don't really understand this argument, I thought it was pretty clear this is not a blame game, but we owe to our users a working Nix implementation. Mic92 didn't merge this, it's lovesegfault who did. Mic92 is reacting and I'm providing my input.

If you are telling me this is dogpiling, then I fear that there's no room to talk about the uncomfortable truth that we are running at the risk of breaking our users all the time and this is not really serious of us.

We are not that reckless with, arguably, equally important part of the distribution. Why should we be here?

It may even be that bumping Nix eagerly is a good thing --- so more people are mad at the Nix team for whatever breakage there is, provided that it doesn't grind Nixpkgs itself to a halt too much. Some bugs do the latter for sure, but other bugs just do the former.

I think it's important to get a coherent story straight, what you are saying feels disaligned with NixOS/nix#10288. I do not understand why this seems to be a reoccurring pattern (disalignement in the public opinions of Nix maintenance team members).


It's very frustrating that I have to appear as the angry dude of this PR. I will repeat my motivations to make them clear: I am not deriving any pleasure to dunk on the Nix maintenance team or the OP of this PR or the merger of this PR. People do mistakes, that's totally fine. But I am disappointed to see yet again a communication problem just after we "step down" (the PR is not even merged, this PR was opened 2 days ago and merged quite quickly…). It seems like to me that people didn't understand why Nix stable was Nix 2.18 for a while and stayed this way and we had no plan to upgrade. What I don't understand is why people don't inquire, don't ask, etc. If that's how we are to behave in a social project on such a critical piece of software for which we don't have good recovery stories, then we must accept that we will break users and people will ragequit over this. I personally don't want have anything to do with this, that's why I am adamant about ensuring we deliver a reasonable quality.

When we do systemd bumps, we try to be careful, we run more complicated smoke testing, we test more scenarios. Nix is supposed to be, to some extent, simpler than systemd.

Please drop the defensive behavior, it seems needless to me, just own the responsibilities, focus on good quality releases for Nix, focus on better testing scenarios and let's have a great QA process. And immediately, I will stop feeling like having the same discussions for months and that nothing has changed and people doesn't care.

Nix has no reason to be so different from the ambiant quality of critical packages in Nixpkgs.

@Ericson2314
Copy link
Member

Ericson2314 commented May 30, 2024

But I am disappointed to see yet again a communication problem just after we "step down"

But that's the precise time I'd expect to see a communication problem! That's why I am not disappointed/whatever, and I don't think you should be either.

@nyabinary
Copy link
Contributor

No, this is NixOS, not LixOS. You are free to create your own distro, of course.

I would assume the Nix part of NixOS is referencing to the language, not the specific implementation of it, or am I wrong on that assumption?

@RaitoBezarius
Copy link
Member

But I am disappointed to see yet again a communication problem just after we "step down"

But that's the precise time I'd expect to see a communication problem! That's why I am not disappointed/whatever, and I don't think you should be either.

This might be me but I fail to understand why we plead that it is normal to repeat the same actions and expect different outcomes… Why should I not be disappointed to see the same things happen after trying so hard to tell people that this will happen if nothing change? It is even more frustrating when folks agree there's something to change and then the same things happen to me. So, can you explain me why I should not be disappointed by the current state of things?

Here's a proposal to give hope: let's pledge that we will not let this re-occur and we will be all cautious together about next bumps, we will go through the checklist and show to the community that we go through it.

@Mic92
Copy link
Member Author

Mic92 commented May 30, 2024

I feel like we reached a point in this discussion, where everything that was to say, has been said. I will work on some documentation similar to what we have for the Rust compiler, so that we know what needs to be tested for a nix release.

@NixOS NixOS locked as off-topic and limited conversation to collaborators May 30, 2024
@vcunat

This comment was marked as duplicate.

@Ericson2314
Copy link
Member

I think having documentation / paper trail / automated tests, versus a series of conversations that didn't have their intended effect, will be much better.

@Mic92
Copy link
Member Author

Mic92 commented May 31, 2024

@kanashimia I also fixed you nix edit issue: NixOS/nix#10814

@lovesegfault
Copy link
Member

I was under the impression that the new Nix releases were already undergoing more thorough testing, and that the responsibility had been transferred to the Nix maintainers, given this has been discussed for a while.

As a result, when I saw a PR from a well-known maintainer I trust, with fixes for issues in the release, and not much discussion, I merged it. I had been running 2.22 since its release, and hadn't seen major issues either.

I'm sorry for the disruption, and I'll refrain from participating in future Nix bumps, it's simply not worth the stress and toxicity.

@domenkozar
Copy link
Member

I don't think that's the right approach, we should fix the workflow so that everyone is comfortable doing the bumps.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.