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

Lazy trees #6530

Draft
wants to merge 390 commits into
base: master
Choose a base branch
from
Draft

Lazy trees #6530

wants to merge 390 commits into from

Conversation

edolstra
Copy link
Member

@edolstra edolstra commented May 12, 2022

Features

  • Flake inputs are now only copied to the Nix store if that's actually needed (fixing Copy local flakes to the store lazily #3121). That is, an operation like nix build nixpkgs#hello no longer copies the nixpkgs flake to the Nix store. Only an expression like src = ./. or nix.registry.nixpkgs.flake = nixpkgs; will cause an entire flake to be copied.
  • fetchTree can now apply patches to a tree, e.g. fetchTree { ...; patches = [ ./foo.patch ./bar.patch ]; } (Support flake references to patches #3920). Patches are applied in memory, so this doesn't require the entire tree to be materialized to disk.
  • The github fetcher now fetches zip archives instead of tarballs, and does not unpack them to disk. Depending on the filesystem, this can save a lot of disk space. (E.g. on ext4 with a 4 KiB block size, an unpacked Nixpkgs tree takes 252 MiB, whereas the zip archive takes 43 MiB.

Design

The evaluator no longer accesses the filesystem directly. Instead, it uses a virtual filesystem abstraction called InputAccessor, which has FS operations like readFile() . You can get the InputAccessor for an Input by calling Input::lazyFetch(), which unlike Input::fetch() does not (necessarily) copy the input to the Nix store.

The following InputAccessors currently exist:

  • FSInputAccessor: This directly accesses the host filesystem, optionally rooted at some prefix. This is used for both non-flake evaluation (e.g. nix-build ./foo.nix), for path:// flakes, and for (dirty or clean) Git working directories (git+file://). It optionally takes a set of allowed paths, which is used by the Git input type to ensure that you can't access untracked files.
  • MemoryInputAccessor: An in-memory filesystem, used to provide <nix/fetchurl.nix>.
  • ZipInputAccessor: An accessor that reads directly from a zipfile, using libzip. Used by the github fetcher.
  • PatchingInputAccessor: An adaptor that wraps another accessor whose readFile() applies a patch to the file returned by the underlying accessor.

Paths are represented inside the evaluator using the SourcePath type, which is a tuple of an InputAccessor and a path relative to the root of the input. So the SourcePath {input->lazyFetch(), "/flake.nix"} represents the file flake.nix inside some input.

Incompatible changes

The outPath attribute returned by fetchTree et al. is no longer a store path but a path (i.e. a SourcePath). This can cause some breakage, e.g. in NixOS, setting

nix.registry.nixpkgs.flake = nixpkgs;

causes the error A definition for option 'nix.registry.nixpkgs.to.path' is not of type 'string or signed integer or boolean or package'. Instead you have to write:

nix.registry.nixpkgs.flake = "${nixpkgs}";

to force the argument to be a store path.

To do

  • Fix many test failures.
  • Fix showing source code in error messages. Currently the error formatter doesn't know anything about SourcePaths.
  • Give a helpful message when trying to use untracked files in a git repo (Be smart and helpful when some files are missing during the evaluation #4507).
  • Fix the evaluation cache.
  • Fix flake locking.
  • Get rid of the narHash attribute in flake.lock? Computing it is expensive since it requires reading the entire source tree. However, we need it to be able to substitute flake inputs, but maybe we don't care about that.
  • What to do with path:// inputs? They're only locked by narHash, so if we don't have narHash, we can't lock path:// inputs.
  • Fix/improve subflake handling.
  • Update release notes / docs.

Context

@jaen
Copy link

jaen commented May 13, 2022

Sorry if this is dumb question, but does it have any bearing on support for git LFS and similar features (git-crypt git annex)? I could imagine the virtual file system accessor transparently handling reading the actual large file instead of the pointer file (this is what currently happens for me when I try to use git LFS for my secrets with sops-nix and it breaks the rebuild) to make this work.

Could this be a right use-case for this feature, or am I misunderstanding it?

@edolstra

This comment was marked as outdated.

@blaggacao
Copy link
Contributor

What to do with path:// inputs? They're only locked by narHash, so if we don't have narHash, we can't lock path:// inputs. maybe we don't care about that.

I agree. We don't care. This greately improves the UX of relative flakes within a repository.

Absolut paths are probably only sensible as a development helper.

@roberth
Copy link
Member

roberth commented May 26, 2022

Absolut paths are probably only sensible as a development helper.

and the relative paths like subflakes can be locked by locking their parent flake instead.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-31/19481/1

std::string readFile(const CanonPath & path) override
{
if (lstat(path).type != tRegular)
throw Error("file '%s' is not a regular file");
Copy link
Member

Choose a reason for hiding this comment

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

nit:

-throw Error("file '%s' is not a regular file");
+throw Error("file '%s' is not a regular file", path);

I am getting this error while running update:

nix run github:edolstra/nix?ref=lazy-trees#nix -- flake update --show-trace
warning: Git tree '/my/project/dir' is dirty
error: file '%s' is not a regular file

       … while updating the flake input 'mach-nix'

       … while updating the lock file of flake 'git+file:///my/project/dir?ref=refs%2fheads%2fmy-current-branch'

and mach-nix input is defined like this:

    mach-nix.url = "github:DavHau/mach-nix";                                                                                                                                                                        
    mach-nix.inputs.nixpkgs.follows = "nixpkgs";                                                                                                                                                                    
    mach-nix.inputs.flake-utils.follows = "flake-utils";         

Copy link
Member

Choose a reason for hiding this comment

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

Very strange, the error is with flake.lock:

error: file '/flake.lock' is not a regular file

       … while updating the flake input 'mach-nix'

       … while updating the lock file of flake 'git+file:///my/project/dir?ref=refs%2fheads%2fmy-current-branch'

Copy link
Member

Choose a reason for hiding this comment

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

Oh, mach-nix has a symlink for the flake.lock. This explains the error.

https://github.com/DavHau/mach-nix/blob/master/flake.lock

Copy link
Member

Choose a reason for hiding this comment

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

I guess readFile has to resolve symlinks now. This in turns asks the question of 1) do we want to support that, 2) Should we prevent symlinks that go out of the flake boundary and 3) do we accept that lazy flakes (that live in a zip archive) will have a different behavior regarding symlinks that escape the flake than flakes that live plainly in the store.

Copy link
Member

Choose a reason for hiding this comment

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

@edolstra This patch works for me 🥳 : layus@44d5bd4

I think it is safe because paths are rooted at the archive root. But is it always correct ? Could be.

@nrdxp
Copy link

nrdxp commented Jun 3, 2022

These changes seem to disallow a flake from setting a different version of itself as an input via a url. This was allowed before. To reproduce just run nix flake lock github:input-output-hk/cardano-node as an example. The following inputs triggers the problem:
https://github.com/input-output-hk/cardano-node/blob/7ee6467e288aee5021f048a1c898c05881060e4b/flake.nix#L44-L49

Not sure how or why this worked before and not now, just kinda an odd find I thought was worth mentioning.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-32/19865/1

@icetan
Copy link

icetan commented Jun 27, 2022

Wanted to try out relative paths to be able to use subflakes in a practical way, but I ran into some problems.

Root flake file flake.nix:

{
  inputs = {
    utils.url = "github:numtide/flake-utils/bee6a7250dd1b01844a2de7e02e4df7d8a0a206c";
    sub1.url = "path:./sub1";
  };
  outputs = { nixpkgs, utils, sub1, ... }:
    utils.lib.eachDefaultSystem (system: {
      packages = {
        inherit (sub1.packages.${system}) hello;
      };
    });
}

Subflake file sub1/flake.nix:

{
  inputs = {
    nixpkgs.url = "github:NixOS/nixpkgs/9a17f325397d137ac4d219ecbd5c7f15154422f4";
    utils.url = "github:numtide/flake-utils/bee6a7250dd1b01844a2de7e02e4df7d8a0a206c";
  };
  outputs = { nixpkgs, utils, ... }:
    utils.lib.eachDefaultSystem (system:
      let
        legacyPackages = import nixpkgs { inherit system; };
      in
      {
        packages = {
          inherit (legacyPackages) hello;
        };
      });
}

When I run nix flake show with latest from lazy-trees branch:

nix run github:edolstra/nix/04cb555aebfff68732cc7f0120def20449515eea -- flake show --show-trace

I get the output:

path:/home/icetan/src/nix/test-subflakes
└───packages
    ├───aarch64-darwin
error: in pure evaluation mode, 'fetchTree' requires a locked input, at (string):16:18

       … while realising the context of a path

       at «string»:20:19:

           19|
           20|           flake = import (sourceInfo + (if subdir != "" then "/" else "") + subdir + "/flake.nix");
             |                   ^
           21|

       … while evaluating anonymous lambda

       at «string»:10:13:

            9|     builtins.mapAttrs
           10|       (key: node:
             |             ^
           11|         let

       … from call site

       … while evaluating anonymous lambda

       at «string»:23:25:

           22|           inputs = builtins.mapAttrs
           23|             (inputName: inputSpec: allNodes.${resolveInput inputSpec})
             |                         ^
           24|             (node.inputs or {});

       … from call site

I expect something more like when running nix flake show using nix (Nix) 2.9.1:

path:/home/icetan/src/nix/test-subflakes?lastModified=1656334768&narHash=sha256-ODEqelm5y%2fVQkVkX11EWMWOHFpH+8wMZpEySuE0lPWY=
└───packages
    ├───aarch64-darwin
    │   └───hello: package 'hello-2.12'
    ├───aarch64-linux
    │   └───hello: package 'hello-2.12'
    ├───i686-linux
    │   └───hello: package 'hello-2.12'
    ├───x86_64-darwin
    │   └───hello: package 'hello-2.12'
    └───x86_64-linux
        └───hello: package 'hello-2.12'

@edolstra
Copy link
Member Author

@icetan Yeah, subflakes are broken, I'll fix it.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/running-nixos-rebuild-does-not-add-new-files-have-i-misunderstood-its-purpose/20664/10

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/import-custom-directories-files-to-package-path-on-nix-store/20991/7

{
auto path = file.getPhysicalPath();
if (!path)
throw Error("cannot open '%s' in an editor because it has no physical path", file);
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to write it to a temporary file to allow at least read-only operations on the file. By no means a blocker, but maybe a FIXME would be appropriate here?

src/libcmd/command.cc Outdated Show resolved Hide resolved
if (auto path = std::get_if<SourcePath>(&pos.origin))
return {*path, pos.line};
else
throw Error("'%s' cannot be shown in an editor", pos);
Copy link
Member

Choose a reason for hiding this comment

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

This would happen when the position of the lambda can't be determined, right? I think this error message doesn't reflect that very well.

src/libexpr/eval-cache.cc Outdated Show resolved Hide resolved
@lheckemann

This comment was marked as resolved.

@lheckemann
Copy link
Member

{
  inputs.nixpkgs.url = "github:nixos/nixpkgs/nixos-22.05";
  outputs = {nixpkgs, ...}: {
    defaultPackage.x86_64-linux = nixpkgs.legacyPackages.x86_64-linux.steam;
  };
}

Fails to build because of unfree as expected; however, the source location information in the error message is weird (should probably reference the input name if possible?):

error: Package ‘steam’ in /virtual/4/pkgs/games/steam/steam.nix:43 has an unfree license (‘unfreeRedistributable’), refusing to evaluate.

@lheckemann
Copy link
Member

One unfortunate consequence of this is that you can no longer just open a path mentioned in an error trace, which can make digging into code for debugging errors a lot more painful. Using --override-input to replace e.g. nixpkgs with a local path can probably help with this, but ideally there would be a way to do this for any input type as it was before lazy-trees.

I can think of several fancy ways to work around this, but they all seem quite expensive and this PR is already a lot bigger than it ideally would be; an option to restore the old behaviour of fetching and unpacking inputs to the store would be extremely helpful here.

Fancy too-big-for-this-PR ideas
  • nix edit could be expanded to allow opening files within inputs read-only
    • For extra fanciness, this could be read-write and Nix could output a patch to apply to the input once the editor is done
  • I previously mentioned this as a joke, but a FUSE filesystem (nix flake mount?) would provide a much more comfortable input-inspection facility and could also provide the ability to generate patches

@lheckemann

This comment was marked as outdated.

Comment on lines 469 to 471
evalSettings.restrictEval || evalSettings.pureEval
? std::optional<std::set<CanonPath>>(std::set<CanonPath>())
: std::nullopt,
Copy link
Member

Choose a reason for hiding this comment

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

This reads like 3 args for makeFSInputAccessor. It's still not perfect, but maybe

Suggested change
evalSettings.restrictEval || evalSettings.pureEval
? std::optional<std::set<CanonPath>>(std::set<CanonPath>())
: std::nullopt,
evalSettings.restrictEval || evalSettings.pureEval
? std::optional<std::set<CanonPath>>(std::set<CanonPath>())
: std::nullopt,

What would be nicer is factoring it out into an allowedPaths variable but I'm not sure we can do that in this constructor context (maybe rootFS should just be initialised in the constructor's body?)

@@ -525,14 +532,12 @@ EvalState::~EvalState()

void EvalState::allowPath(const Path & path)
{
if (allowedPaths)
allowedPaths->insert(path);
rootFS->allowPath(CanonPath(path)); // FIXME
Copy link
Member

Choose a reason for hiding this comment

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

Is this the symlinks-and-security-relevant stuff you mentioned?

@@ -2508,7 +2477,8 @@ void EvalState::printStats()
else
obj.attr("name", nullptr);
if (auto pos = positions[fun->pos]) {
obj.attr("file", (std::string_view) pos.file);
if (auto path = std::get_if<SourcePath>(&pos.origin))
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to cover the other variants (or at least stdin and string) in some way here too -- maybe even add some further metadata to allow distinguishing --apply from --expr strings. Nothing that needs to be resolved in this PR though IMHO.

src/libexpr/nixexpr.hh Outdated Show resolved Hide resolved
src/libexpr/eval.hh Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/proper-way-of-applying-patch-to-system-managed-via-flake/21073/24

@lheckemann
Copy link
Member

Not sure if intentional or not, but this fixes referring to local bare repos like nix flake metadata git+file:///path/to/repo.git 👍

This resolves an evaluation error in
https://api.flakehub.com/f/pinned/knightpp/modupdate/0.0.8/018d560c-d88c-7c2b-ba5f-918283ea4c47/source.tar.gz,
which does something like

  filter = path: type:
    builtins.elem (/. + path) [ ./go.mod ... ];

leading to

  error: a string that refers to a store path cannot be appended to a path

The hack is that if we append a virtual path to /., we discard the
orginal accessor and use the one from the right-hand side.
@adrian-gierakowski
Copy link

Is there a list that of known edge-cases and regressions one should be aware of when n case one would like to use this branch? Thank you!

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/is-this-the-current-state-of-things/45944/17

@erikarvstedt
Copy link
Member

This fails when NIX_PATH contains flake references:

NIX_PATH=nixpkgs=flake:nixpkgs nix eval --impure --expr '
(import <nixpkgs/nixos> {
  configuration = {};
}).vm.drvPath
'

Result:

error: cannot decode virtual path '/nix/store/lazylazy000000000000000000000004-source/pkgs/development/compilers/llvm/17/llvm/gnu-install-dirs.patch'

nrdxp added a commit to ekala-project/atom that referenced this pull request Aug 4, 2024
With this, we have essentially achieved NixOS/nix#6530 in pure nix.

The only remaining "impurity" in this sense would be the path passed
to the composer function, but this is actually a good thing, since
it doesn't eagerly copy everything to the nix store like flakes, but
instead only makes pure copies of files when they are actually accessed.

If the path doesn't exist on the file-system we will immediately fail
anyway, so it's "impurity" is irrelevant. There will be no way for
the user to escape the module system with these new enforcements, so
that means that we basically have a totally pure nix evaluation without
the high cost of having to copy the entire repo into the /nix/store
eagerly.
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/how-are-flake-dependencies-fetched/52638/4

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nix-team-member-suggests-removing-flakes-data-suggest-it-isnt-a-good-idea-please-remove-the-experimental-flag-instead/54959/78

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nix-team-member-suggests-removing-flakes-data-suggest-it-isnt-a-good-idea-please-remove-the-experimental-flag-instead/54959/104

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation feature Feature request or proposal fetching Networking with the outside (non-Nix) world, input locking flakes new-cli Relating to the "nix" command significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. tests with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Restrict builtins.isStorePath Support flake references to patches Copy local flakes to the store lazily