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.24 #335342

Merged
merged 8 commits into from
Sep 17, 2024
Merged

nix: 2.18 -> 2.24 #335342

merged 8 commits into from
Sep 17, 2024

Conversation

roberth
Copy link
Member

@roberth roberth commented Aug 17, 2024

Description of changes

  • Deliver new features and fixes to regular users
  • Add the lib test suite to passthru. I think it already runs for nix aka stable as part of the normal CI, but this makes it easier to run by hand

Depends on #335458

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.

@roberth roberth requested a review from RaitoBezarius as a code owner August 17, 2024 10:37
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 17, 2024
@Mic92 Mic92 removed the request for review from RaitoBezarius August 17, 2024 11:13
@Mic92

This comment was marked as resolved.

@Mic92

This comment was marked as outdated.

@emilazy
Copy link
Member

emilazy commented Aug 17, 2024

I’m pretty strongly opposed to merging this when NixOS/nix#11098 hasn’t been addressed at all. The UX regression for macOS flakes users on 2.19+ is severe.

(I daily drive the newer versions, but I wouldn’t wish it on anyone else.)

@Mic92
Copy link
Member

Mic92 commented Aug 17, 2024

I’m pretty strongly opposed to merging this when NixOS/nix#11098 hasn’t been addressed at all. The UX regression for macOS flakes users on 2.19+ is severe.

(I daily drive the newer versions, but I wouldn’t wish it on anyone else.)

Does this also happen on arm64 because I cannot confirm those performance numbers?

@emilazy
Copy link
Member

emilazy commented Aug 17, 2024

I can’t speak to whether @cigrainger is on AArch64. The performance regression isn’t as bad for me as described in the original post for that issue, but everything involving flakes is still noticeably and painfully slower. If it turns out to be x86_64-darwin only and it’s decided that regressing the UX for those users is acceptable, then fair enough. It may be that there are other relevant conditions, though. For example, just off the top of my head, you passed --store, which eliminates the factor of the Nix store being on a different APFS volume.

@Mic92
Copy link
Member

Mic92 commented Aug 17, 2024

Ok. Would be great if you provide concrete numbers for different operations that you have seen being slower. I will try in a bit, if using the nix-daemon introduces a significant difference.

@emilazy
Copy link
Member

emilazy commented Aug 17, 2024

I’ll try to find the time to do some proper measurements. If other people can’t reproduce this then I don’t want to block on those grounds though; it just seemed like a lot of people found the upstream report for it to be a particularly specific thing.

@Mic92
Copy link
Member

Mic92 commented Aug 17, 2024

Result of nixpkgs-review pr 335342 run on aarch64-darwin 1

1 package marked as broken and skipped:
  • fusionInventory
30 packages failed to build:
  • attic-client
  • darwin.linux-builder
  • darwin.linux-builder-x86_64
  • haskellPackages.cabal2nix-unstable
  • haskellPackages.cli-nix
  • haskellPackages.cli-nix.doc
  • haskellPackages.niv
  • haskellPackages.niv.bin
  • haskellPackages.niv.data
  • haskellPackages.niv.doc
  • haskellPackages.nix-serve-ng
  • haskellPackages.nix-serve-ng.doc
  • haskellPackages.nix-thunk
  • haskellPackages.nix-thunk.doc
  • haskellPackages.nvfetcher
  • haskellPackages.nvfetcher.doc
  • haskellPackages.update-nix-fetchgit
  • haskellPackages.update-nix-fetchgit.doc
  • libnixxml
  • nix-doc
  • nix-plugins
  • nix-serve
  • nix-serve-ng
  • nix-simple-deploy
  • tests.devShellTools.nixos
  • tests.testers.lycheeLinkCheck.network
  • tests.testers.nixosTest-example
  • tests.testers.runNixOSTest-example
  • tests.trivial-builders.references
  • zon2nix
78 packages built:
  • appvm
  • attic-server
  • bundix
  • cabal2nix
  • cached-nix-shell
  • crate2nix
  • crystal2nix
  • devenv
  • dydisnix
  • haskellPackages.nix-paths
  • haskellPackages.nix-paths.doc
  • lua51Packages.luarocks-nix
  • luarocks-nix (luaPackages.luarocks-nix)
  • lua53Packages.luarocks-nix
  • lua54Packages.luarocks-nix
  • luajitPackages.luarocks-nix
  • luarocks-packages-updater
  • nim_lk
  • niv (niv.bin ,niv.data)
  • nix (nixVersions.stable)
  • nix-bundle
  • nix-direnv
  • nix-du
  • nix-index
  • nix-init
  • nix-pin
  • nix-prefetch
  • nix-prefetch-bzr
  • nix-prefetch-cvs
  • nix-prefetch-docker
  • nix-prefetch-git
  • nix-prefetch-hg
  • nix-prefetch-scripts
  • nix-prefetch-svn
  • nix-required-mounts
  • nix-required-mounts.dist
  • nix-template
  • nix-update
  • nix-update-source
  • nix-update-source.dist
  • nix-update.dist
  • nix-visualize
  • nix-visualize.dist
  • nix.dev (nixVersions.stable.dev)
  • nix.doc (nixVersions.stable.doc)
  • nix.man (nixVersions.stable.man)
  • nixci
  • nixos-anywhere
  • nixos-generators
  • nixos-shell
  • nixpkgs-review
  • nixpkgs-review.dist
  • nixtract
  • node2nix
  • npins
  • nurl
  • nvfetcher
  • python311Packages.nix-kernel
  • python311Packages.nix-kernel.dist
  • python312Packages.nix-kernel
  • python312Packages.nix-kernel.dist
  • sbomnix
  • sbomnix.dist
  • sonarr
  • swiftpm2nix (swiftPackages.swiftpm2nix)
  • terranix
  • tests.pkg-config.defaultPkgConfigPackages.nix-cmd
  • tests.pkg-config.defaultPkgConfigPackages.nix-expr
  • tests.pkg-config.defaultPkgConfigPackages.nix-main
  • tests.pkg-config.defaultPkgConfigPackages.nix-store
  • update-nix-fetchgit
  • vimPluginsUpdater
  • vulnix
  • vulnix.dist
  • vulnix.doc
  • vulnix.man
  • wp4nix
  • yarn2nix

I think we need to pin a few packages against older nix version. Some of them are using nix bindings.

@Mic92
Copy link
Member

Mic92 commented Aug 17, 2024

I’ll try to find the time to do some proper measurements. If other people can’t reproduce this then I don’t want to block on those grounds though; it just seemed like a lot of people found the upstream report for it to be a particularly specific thing.

I can probably also get hold on a native x86_64-darwin machine, but that would take me a few days.

@Mic92

This comment was marked as outdated.

@Mic92

This comment was marked as outdated.

@Mic92
Copy link
Member

Mic92 commented Aug 17, 2024

Result of nixpkgs-review pr 335342 run on aarch64-linux 1

1 package blacklisted:
  • nixos-install-tools
10 packages failed to build:
  • attic-client
  • haskellPackages.nix-serve-ng
  • haskellPackages.nix-serve-ng.doc
  • nix-doc
  • nix-plugins
  • nix-serve
  • nix-serve-ng
  • nix-simple-deploy
  • nixStatic
  • nixStatic.dev
102 packages built:
  • appvm
  • attic-server
  • bundix
  • cabal2nix
  • cached-nix-shell
  • crate2nix
  • crystal2nix
  • devenv
  • disko
  • dydisnix
  • fusionInventory
  • haskellPackages.cabal2nix-unstable
  • haskellPackages.cli-nix
  • haskellPackages.cli-nix.doc
  • haskellPackages.niv
  • haskellPackages.niv.bin
  • haskellPackages.niv.data
  • haskellPackages.niv.doc
  • haskellPackages.nix-paths
  • haskellPackages.nix-paths.doc
  • haskellPackages.nix-thunk
  • haskellPackages.nix-thunk.doc
  • haskellPackages.nvfetcher
  • haskellPackages.nvfetcher.doc
  • haskellPackages.update-nix-fetchgit
  • haskellPackages.update-nix-fetchgit.doc
  • libnixxml
  • lua51Packages.luarocks-nix
  • luarocks-nix (luaPackages.luarocks-nix)
  • lua53Packages.luarocks-nix
  • lua54Packages.luarocks-nix
  • luajitPackages.luarocks-nix
  • luarocks-packages-updater
  • nim_lk
  • niv (niv.bin ,niv.data)
  • nix (nixVersions.stable)
  • nix-bundle
  • nix-direnv
  • nix-du
  • nix-index
  • nix-init
  • nix-pin
  • nix-prefetch
  • nix-prefetch-bzr
  • nix-prefetch-cvs
  • nix-prefetch-docker
  • nix-prefetch-git
  • nix-prefetch-hg
  • nix-prefetch-scripts
  • nix-prefetch-svn
  • nix-required-mounts
  • nix-required-mounts.dist
  • nix-template
  • nix-update
  • nix-update-source
  • nix-update-source.dist
  • nix-update.dist
  • nix-visualize
  • nix-visualize.dist
  • nix.debug (nixVersions.stable.debug)
  • nix.dev (nixVersions.stable.dev)
  • nix.doc (nixVersions.stable.doc)
  • nix.man (nixVersions.stable.man)
  • nixci
  • nixos-anywhere
  • nixos-generators
  • nixos-shell
  • nixpkgs-review
  • nixpkgs-review.dist
  • nixtract
  • node2nix
  • npins
  • nurl
  • nvfetcher
  • outline
  • python311Packages.nix-kernel
  • python311Packages.nix-kernel.dist
  • python312Packages.nix-kernel
  • python312Packages.nix-kernel.dist
  • sbomnix
  • sbomnix.dist
  • sonarr
  • swiftpm2nix (swiftPackages.swiftpm2nix)
  • terranix
  • tests.devShellTools.nixos
  • tests.pkg-config.defaultPkgConfigPackages.nix-cmd
  • tests.pkg-config.defaultPkgConfigPackages.nix-expr
  • tests.pkg-config.defaultPkgConfigPackages.nix-main
  • tests.pkg-config.defaultPkgConfigPackages.nix-store
  • tests.testers.lycheeLinkCheck.network
  • tests.testers.nixosTest-example
  • tests.testers.runNixOSTest-example
  • tests.trivial-builders.references
  • update-nix-fetchgit
  • vimPluginsUpdater
  • vulnix
  • vulnix.dist
  • vulnix.doc
  • vulnix.man
  • wp4nix
  • yarn2nix
  • zon2nix

@lf-
Copy link
Member

lf- commented Aug 17, 2024

nix-doc should have its plugin turned off entirely. it's not required if :doc knows how to do lambdas, which I've heard rumours has been implemented in some cppnix version? idk i have just been eating ice cream this whole time.

@lf-
Copy link
Member

lf- commented Aug 17, 2024

nixStatic should be a hard blocker on merging this. If it's broken since it broke upstream somehow, I guess y'all need to fix that and roll a release.

@Mic92
Copy link
Member

Mic92 commented Aug 17, 2024

nixStatic should be a hard blocker on merging this. If it's broken since it broke upstream somehow, I guess y'all need to fix that and roll a release.

It's actually nixpkgs that is broken (perl-static), so we need to fix nixpkgs itself. nixStatic in the nix flake builds as it uses an older version of nixpkgs.

@Mic92
Copy link
Member

Mic92 commented Aug 17, 2024

nix-doc should have its plugin turned off entirely. it's not required if :doc knows how to do lambdas, which I've heard rumours has been implemented in some cppnix version?

How does one verify that it works? We could otherwise also pin it to an old version and leave it to its maintainers to fix it.

@Mic92
Copy link
Member

Mic92 commented Aug 17, 2024

nixStatic should be a hard blocker on merging this. If it's broken since it broke upstream somehow, I guess y'all need to fix that and roll a release.

It's actually nixpkgs that is broken (perl-static), so we need to fix nixpkgs itself. nixStatic in the nix flake builds as it uses an older version of nixpkgs.

Yep. Even on upstream/master the dependencies of nixStatic are broken. Not even libcurl does build

@Mic92 Mic92 mentioned this pull request Aug 17, 2024
13 tasks
@Mic92

This comment was marked as outdated.

@Mic92
Copy link
Member

Mic92 commented Sep 13, 2024

What I also found to be quite fast, especially w.r.t. to update is using nix's shallow clone feature:

    nixpkgs.url = "git+https://github.com/Mic92/nixpkgs?shallow=1";

I haven't bench-marked yet in depth but it feels to me that it also copies faster to the next store than the tarball cache.

@pan93412
Copy link
Contributor

devenv

cachix/devenv#1364 (comment)

Fixed in Devenv 1.1.0. It should not block this PR now.

@emilazy
Copy link
Member

emilazy commented Sep 13, 2024

We've restored the caching of github: and tarball-based store paths, avoiding expensive Nixpkgs copying DOC Python: python setup.py develop is not used anymore #11285

(BTW, after correcting the URL it looks like the backport hasn’t hit a 2.24 release yet, right?)

@roberth
Copy link
Member Author

roberth commented Sep 13, 2024

@emilazy I've fixed the link.
The change has been backported and released, but it doesn't show on the GitHub commit page.

$ git show --oneline ae486b2
ae486b291 Merge pull request #11446 from NixOS/mergify/bp/2.24-maintenance/pr-11285

$ git where ae486b2 
refs/remotes/upstream/2.24-maintenance
refs/remotes/upstream/latest-release
refs/tags/2.24.6
[ omitted a few lines ]
# Home Manager
programs.git.aliases.where = "for-each-ref '--format=%(refname)' --contains";

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

I share people’s concerns about the Nix release process of late, but it doesn’t seem like just shipping Nix 2.18 forever is a viable path forward. This bump has a lot more care put into it than the previous few times. Putting my 2¢ in that It seems worth trying, as long as we’re prepared to revert before 24.11 if necessary.

@wegank wegank added the 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people label Sep 14, 2024
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nar-unpacking-vulnerability-post-mortem/52301/1

Copy link
Member

@fabianhjr fabianhjr left a comment

Choose a reason for hiding this comment

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

Ran:

  • nix build .#nixosTests.misc

Thanks for adding some new testing. Seems like all major points have been addressed and further improvements can be done in future merge requests.

Failing test is also failing on the main branch with very similar log: https://hydra.nixos.org/build/272709812/nixlog/1

Tested switching locally

@fabianhjr fabianhjr merged commit e25a409 into master Sep 17, 2024
28 checks passed
@adamcstephens adamcstephens deleted the update-nix-2.24 branch September 17, 2024 21:43
@Aleksanaa
Copy link
Member

A nix-serve regression was mentioned in https://discourse.nixos.org/t/2024-09-16-nix-team-meeting-minutes-178-177/52270#bump-218-224-in-nixpkgs-4, but I didn't find relevant issues in https://github.com/NixOS/nix.

@fabianhjr
Copy link
Member

--- a/nix-serve.psgi
+++ b/nix-serve.psgi
@@ -22,6 +22,7 @@ BEGIN {
 my $app = sub {
     my $env = shift;
     my $path = $env->{PATH_INFO};
+    my $store = Nix::Store->new();
 
     if ($path eq "/nix-cache-info") {
         return [200, ['Content-Type' => 'text/plain'], ["StoreDir: $Nix::Config::storeDir\nWantMassQuery: 1\nPriority: 30\n"]];
@@ -29,9 +30,9 @@ my $app = sub {
 
     elsif ($path =~ /^\/([0-9a-z]+)\.narinfo$/) {
         my $hashPart = $1;
-        my $storePath = queryPathFromHashPart($hashPart);
+        my $storePath = $store->queryPathFromHashPart($hashPart);
         return [404, ['Content-Type' => 'text/plain'], ["No such path.\n"]] unless $storePath;
-        my ($deriver, $narHash, $time, $narSize, $refs, $sigs) = queryPathInfo($storePath, 1) or die;
+        my ($deriver, $narHash, $time, $narSize, $refs, $sigs) = $store->queryPathInfo($storePath, 1) or die;
         $narHash =~ /^sha256:(.*)/ or die;
         my $narHash2 = $1;
         die unless length($narHash2) == 52;
@@ -57,9 +58,9 @@ my $app = sub {
     elsif ($path =~ /^\/nar\/([0-9a-z]+)-([0-9a-z]+)\.nar$/) {
         my $hashPart = $1;
         my $expectedNarHash = $2;
-        my $storePath = queryPathFromHashPart($hashPart);
+        my $storePath = $store->queryPathFromHashPart($hashPart);
         return [404, ['Content-Type' => 'text/plain'], ["No such path.\n"]] unless $storePath;
-        my ($deriver, $narHash, $time, $narSize, $refs, $sigs) = queryPathInfo($storePath, 1) or die;
+        my ($deriver, $narHash, $time, $narSize, $refs, $sigs) = $store->queryPathInfo($storePath, 1) or die;
         return [404, ['Content-Type' => 'text/plain'], ["Incorrect NAR hash. Maybe the path has been recreated.\n"]]
             unless $narHash eq "sha256:$expectedNarHash";
         my $fh = new IO::Handle;
@@ -70,9 +71,9 @@ my $app = sub {
     # FIXME: remove soon.
     elsif ($path =~ /^\/nar\/([0-9a-z]+)\.nar$/) {
         my $hashPart = $1;
-        my $storePath = queryPathFromHashPart($hashPart);
+        my $storePath = $store->queryPathFromHashPart($hashPart);
         return [404, ['Content-Type' => 'text/plain'], ["No such path.\n"]] unless $storePath;
-        my ($deriver, $narHash, $time, $narSize, $refs) = queryPathInfo($storePath, 1) or die;
+        my ($deriver, $narHash, $time, $narSize, $refs) = $store->queryPathInfo($storePath, 1) or die;
         my $fh = new IO::Handle;
         open $fh, "-|", "nix", "dump-path", "--", $storePath;
         return [200, ['Content-Type' => 'text/plain', 'Content-Length' => $narSize], $fh];

Though still errors with

       > machine # [    7.426781] nix-daemon[852]: accepted connection from pid 838, user nix-serve
       > machine # [    7.464875] nix-serve-start[856]: warning: 'dump-path' is a deprecated alias for 'store dump-path'
       > machine # [    7.467184] nix-serve-start[856]: error: experimental Nix feature 'nix-command' is disabled; add '--extra-experimental-features nix-command' to enable it
       > machine #   0  229k    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0  0  229k    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0
       > machine # curl: (18) end of response with 234672 bytes missing
       > machine: output:

With

--- a/nixos/tests/nix-serve.nix
+++ b/nixos/tests/nix-serve.nix
@@ -2,6 +2,7 @@ import ./make-test-python.nix ({ pkgs, ... }:
 {
   name = "nix-serve";
   nodes.machine = { pkgs, ... }: {
+    nix.settings.experimental-features = [ "nix-command" ];
     services.nix-serve.enable = true;
     environment.systemPackages = [
       pkgs.hello

test passes

@fabianhjr
Copy link
Member

Upstreamed one part edolstra/nix-serve#60

@K900
Copy link
Contributor

K900 commented Sep 19, 2024

https://hydra.nixos.org/build/273031249/nixlog/6

Are we doing this again

@fabianhjr
Copy link
Member

oh no

@lf-
Copy link
Member

lf- commented Sep 19, 2024

holy crap it's the apparently intermittent bug that was reported in the nix matrix channel today. well i guess we have a repro now.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nar-unpacking-vulnerability-post-mortem/52301/29

@fabianhjr
Copy link
Member

fabianhjr commented Sep 19, 2024

https://github.com/NixOS/nix/blob/2.24.6/src/libexpr/eval.cc#L1965-L1973

I believe a bug might be present here but don't know enough c/c++, doesn't seem to have recent changes either though.

@fabianhjr
Copy link
Member

fabianhjr commented Sep 19, 2024

In particular my thought is that pos should be moved forward by l * sizeof(Value *) (the number of bytes written over to the new concated list) instead of by l (the number of elements copied over to the new list)

@Aleksanaa
Copy link
Member

That mkList is one: NixOS/nix@fecff52 since we used to only run this test with Nix 2.18. Also, the probability of recurrence of this bug seems relatively low, so it's a bit hard to reproduce. I've run that test locally 10 times without luck.

@fabianhjr
Copy link
Member

Thanks for the explanation, don't think I can further help with this UnU

@lf-
Copy link
Member

lf- commented Sep 19, 2024

Dawn has a full core dump of the bug run in the matrix, if that's helpful to anyone figuring it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: clean-up 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.