-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: master
Are you sure you want to change the base?
Lazy trees #6530
Conversation
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? |
This comment was marked as outdated.
This comment was marked as outdated.
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. |
and the relative paths like subflakes can be locked by locking their parent flake instead. |
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"); |
There was a problem hiding this comment.
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";
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
These changes seem to disallow a flake from setting a different version of itself as an input via a Not sure how or why this worked before and not now, just kinda an odd find I thought was worth mentioning. |
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 |
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 {
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 {
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 run github:edolstra/nix/04cb555aebfff68732cc7f0120def20449515eea -- flake show --show-trace I get the output:
I expect something more like when running 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' |
@icetan Yeah, subflakes are broken, I'll fix it. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
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 |
src/libcmd/command.cc
Outdated
{ | ||
auto path = file.getPhysicalPath(); | ||
if (!path) | ||
throw Error("cannot open '%s' in an editor because it has no physical path", file); |
There was a problem hiding this comment.
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/repl.cc
Outdated
if (auto path = std::get_if<SourcePath>(&pos.origin)) | ||
return {*path, pos.line}; | ||
else | ||
throw Error("'%s' cannot be shown in an editor", pos); |
There was a problem hiding this comment.
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.
This comment was marked as resolved.
This comment was marked as resolved.
{
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?):
|
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 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
|
This comment was marked as outdated.
This comment was marked as outdated.
src/libexpr/eval.cc
Outdated
evalSettings.restrictEval || evalSettings.pureEval | ||
? std::optional<std::set<CanonPath>>(std::set<CanonPath>()) | ||
: std::nullopt, |
There was a problem hiding this comment.
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
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?)
src/libexpr/eval.cc
Outdated
@@ -525,14 +532,12 @@ EvalState::~EvalState() | |||
|
|||
void EvalState::allowPath(const Path & path) | |||
{ | |||
if (allowedPaths) | |||
allowedPaths->insert(path); | |||
rootFS->allowPath(CanonPath(path)); // FIXME |
There was a problem hiding this comment.
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?
src/libexpr/eval.cc
Outdated
@@ -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)) |
There was a problem hiding this comment.
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.
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 |
Not sure if intentional or not, but this fixes referring to local bare repos like |
Would For example: |
I think it's OK to copy the entire source tree for a clean build, because it is supposed to be slow when it's huge, but it's better not to copy the tree for a development shell |
The
|
This fails when NIX_PATH=nixpkgs=flake:nixpkgs nix eval --impure --expr '
(import <nixpkgs/nixos> {
configuration = {};
}).vm.drvPath
' Result:
|
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.
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 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
waiting for NixOS/nix#6530 Signed-off-by: saygo-png <saygo.mail@proton.me>
Features
nix build nixpkgs#hello
no longer copies thenixpkgs
flake to the Nix store. Only an expression likesrc = ./.
ornix.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.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 likereadFile()
. You can get theInputAccessor
for anInput
by callingInput::lazyFetch()
, which unlikeInput::fetch()
does not (necessarily) copy the input to the Nix store.The following
InputAccessor
s 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
), forpath://
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 thegithub
fetcher.PatchingInputAccessor
: An adaptor that wraps another accessor whosereadFile()
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 anInputAccessor
and a path relative to the root of the input. So theSourcePath
{input->lazyFetch(), "/flake.nix"}
represents the fileflake.nix
inside some input.Incompatible changes
The
outPath
attribute returned byfetchTree
et al. is no longer a store path but a path (i.e. aSourcePath
). This can cause some breakage, e.g. in NixOS, settingcauses 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:to force the argument to be a store path.
To do
SourcePath
s.narHash
attribute inflake.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.path://
inputs? They're only locked bynarHash
, so if we don't havenarHash
, we can't lockpath://
inputs.Context