Lazy `fetchTree` `outPath` path values

[roberth]

Motivation

Improve performance, and make the fetchTree interface more capable while keeping it clean.

Description

This makes fetchTree return lazy InputAccessor-based SourcePaths instead of "cowardly" fetching them to the store and returning absolute "system" paths. It stays close to existing path semantics, including support for readFile "${toString p}/..", which some expressions rely on. It does not go as far as lazy-trees, but judging from the amount of change I could reuse, and how little of my own I had to add, lazy-trees will be a natural extension of this PR.

Done:

• Packages and NixOS evaluate as usual
  • no hash changes, based on my limited testing
• Flake is not added to store unless e.g.
  • "${flake.outPath}"
  • uses module system (needs clever lazy source strings, or a change to the module system)
• Note that the above is already an improvement over the status quo - always fetching to the store
• iirc 0.1s reduction on nixpkgs#hello, and 1s reduction on nixosTests.simple

Conclusion so far: Viable

TODO:

• [ ] Check the TODO and FIXMEs
• [ ] Update the test suite
  • probably some intended behavior change, such as removal of narHash in some observable places
  • possibly finds a bug
• [ ] Check performance again
• [ ] Cherry-pick the toString path behavior to lazy-trees

Context

• Resolves https://github.com/NixOS/nix/issues/10115

• Includes #10225

Priorities and Process

Add :+1: to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[ConnorBaker]

@roberth is there anything I (or someone in general) could do to try to help move this forward? Thank you for working on this :)

[roberth]

@roberth is there anything I (or someone in general) could do to try to help move this forward? Thank you for working on this :)

Hi @ConnorBaker,

Sorry for the late response; I had to take another look at this first, and it took a while to get around to it. I've rebased the branch, but I seem to have broken the search path somehow.

I see two ways forward, either

• re-do the PR with reduced scope
  • don't remove the narHash, if possible; should keep more flakes code the same
  • carefully look whether other changes were absolutely necessary
• keep going; just fix the remaining test failures, and also remove the commit delay the flake outPath semantics change for now

[nixos-discourse]

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

https://discourse.nixos.org/t/2024-04-29-nix-team-meeting-minutes-142/45020/1

[nixos-discourse]

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

https://discourse.nixos.org/t/2024-07-15-nix-team-meeting-minutes-161/49228/1

[nixos-discourse]

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

https://discourse.nixos.org/t/2024-08-07-nix-team-meeting-minutes-167/50287/1

[tomberek]

Reverting the last commit (4332b9a46711b37bcf73d7de81452efecc55cd75) gives us the following comparison with vanilla Nix:

Before

$ nix eval .#data --no-eval-cache | nixfmt
warning: Git tree '/home/tom/nix/t' is dirty
{
  fetchTree = "/nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source";
  fetchTreePath = "/nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source/ci";
  fetchTreePathStr = "/nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source/ci";
  originalStr = "/nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source";
  outPathRaw = "/nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source";
  outPathStr = "/nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source";
  pathRaw = /nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source;
  pathRawAdd = /nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source/ci;
  pathRawAddStr = "/nix/store/z0qs96vamg1r2ch0rml9pmsn8f002hvw-ci";
  pathStr = "/nix/store/rslrjkrdgd2ggxmlyckc53nv0pxjq5qj-3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source";
}
$ nix eval .#data --no-eval-cache -vvv |& grep copying.*-source
...
copying '/nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source/pkgs/development/libraries/glibc/nix-nss-open-files.patch' to the store...
copying '/nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source/pkgs/development/libraries/glibc/0001-Revert-Remove-all-usage-of-BASH-or-BASH-in-installed.patch' to the store...
copying '/nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source/pkgs/development/libraries/glibc/reenable_DT_HASH.patch' to the store...
copying '/nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source/ci' to the store...
copying '/nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source' to the store...

After (only the differences)

This has fewer copies to the store, going from 6s to 0.6s!

$ nix eval .#data --no-eval-cache | tr -cd '[[:print:]]' |nixfmt      # because of "»" characters
{
  fetchTree = "github:NixOS/nixpkgs/086a5ea5b3acc4c512f9ec154bfefba55efba4f3?narHash=sha256-LyZtQZiq2v2We5ODev6s9s2iUHNu/ZC8rHIYRh1BIzg%3D:";
  fetchTreePath = "github:NixOS/nixpkgs/086a5ea5b3acc4c512f9ec154bfefba55efba4f3?narHash=sha256-LyZtQZiq2v2We5ODev6s9s2iUHNu/ZC8rHIYRh1BIzg%3D:ci";
  fetchTreePathStr = "/nix/store/z0qs96vamg1r2ch0rml9pmsn8f002hvw-ci";
  originalStr = ...
  outPathRaw = "github:NixOS/nixpkgs/086a5ea5b3acc4c512f9ec154bfefba55efba4f3?narHash=sha256-LyZtQZiq2v2We5ODev6s9s2iUHNu/ZC8rHIYRh1BIzg%3D:";
  outPathStr = ...
  pathRaw = "github:NixOS/nixpkgs/086a5ea5b3acc4c512f9ec154bfefba55efba4f3?narHash=sha256-LyZtQZiq2v2We5ODev6s9s2iUHNu/ZC8rHIYRh1BIzg%3D:";
  pathRawAdd = "github:NixOS/nixpkgs/086a5ea5b3acc4c512f9ec154bfefba55efba4f3?narHash=sha256-LyZtQZiq2v2We5ODev6s9s2iUHNu/ZC8rHIYRh1BIzg%3D:ci";
  pathRawAddStr = ...
  pathStr = "/nix/store/3a07hs5zz57xf76gqq53i9jkmi3mhyp6-source";
}

Source

{
  inputs.nixpkgs.url = "github:NixOS/nixpkgs";
  outputs = _: {
    data = rec {
      fetchTree = (builtins.fetchTree (builtins.fromJSON (builtins.readFile ./flake.lock)).nodes.nixpkgs.locked).outPath;
      fetchTreePath = fetchTree + "/ci";

      # original = _.nixpkgs;
      originalStr = "${_.nixpkgs}";
      outPathRaw = _.nixpkgs.outPath;
      outPathStr = "${_.nixpkgs.outPath}";
      pathRaw = _.nixpkgs.legacyPackages.x86_64-linux.path;
      pathStr = "${_.nixpkgs.legacyPackages.x86_64-linux.path}";
      pathRawAdd = _.nixpkgs.legacyPackages.x86_64-linux.path + "/ci";
      pathRawAddStr = "${_.nixpkgs.legacyPackages.x86_64-linux.path + "/ci"}";
    };
  };
}

[tomberek]

Noticed an eval failure when using lib.fileset compared to normal. This is because the value retains a context that it otherwise loses.

A more critical location this happens is in lib.fileset (@infinisil ): A dynamic attribute name: https://github.com/NixOS/nixpkgs/blob/master/lib/fileset/internal.nix#L249

• looks like a path is implicitly converted to a string here, but retains a context

A minimal repro:

[tom@tframe:~/nix/t2]$ ~/.nix-profile-new/bin/nix eval .#value
{ gv00g760bh9xa2kp42z1c2wcv91p7yhy-source = 1; }

[tom@tframe:~/nix/t2]$ ../outputs/out/bin/nix eval .#value
error:
       … while evaluating the name of a dynamic attribute
         at flake.nix:4:7:
            3|     value = {
            4|       "${builtins.baseNameOf ./.}" = 1;
             |       ^
            5|     };

       error: the string 'gv00g760bh9xa2kp42z1c2wcv91p7yhy-source' is not allowed to refer to a store path (such as 'gv00g760bh9xa2kp42z1c2wcv91p7yhy-source')

[tom@tframe:~/nix/t2]$ cat flake.nix
{
  outputs = {...}: {
    value = {
      "${builtins.baseNameOf ./.}" = 1;
    };
  };
}

Perhaps this does the wrong thing when coerceToString happens regarding copyToStore or perhaps we need special handling the lazy path type? (https://github.com/NixOS/nix/blob/cfe66dbec325d5dcb601b642bd9c149ae1353147/src/libexpr-c/nix_api_external.cc#L108C48-L108C59)

[roberth]

       … while evaluating the name of a dynamic attribute
         at flake.nix:4:7:
            3|     value = {
            4|       "${builtins.baseNameOf ./.}" = 1;
             |       ^
            5|     };

Yeah that looks like baseNameOf is performing a conversion to string, which is unnecessary. I've pushed a fix. dirOf seems to be ok, but readFile has logic like state.store->isInStore(path.path.abs()) which does not account for the possibly non-system accessor. realisePath returns path values unchanged, so that is a thing now. It looks like the primops need to be audited for how they handle the .path field where they do.

[nixos-discourse]

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

https://discourse.nixos.org/t/2024-08-21-nix-team-meeting-minutes-171/50950/1

[nyabinary]

The performance label should probably be added to this, right?

[roberth]

We're turning this into a collaborative effort at #11367 (backed by an upstream branch for easy pushing)