diff --git a/doc/manual/rl-next/flake-file-input-correction.md b/doc/manual/rl-next/flake-file-input-correction.md new file mode 100644 index 000000000..7c9d0c08f --- /dev/null +++ b/doc/manual/rl-next/flake-file-input-correction.md @@ -0,0 +1,17 @@ +--- +synopsis: "Flake inputs/`builtins.fetchTree` invocations with `type = \"file\"` now have consistent (but different from previous versions) resulting paths" +issues: [fj#750] +cls: [2864] +category: "Breaking Changes" +credits: [jade] +--- + +Previously `fetchTree { type = "file"; url = "...", narHash = "sha256-..."; }` could return a different result depending on whether someone has run `nix store add-path --name source ...` on a path with the same `narHash` as the flake input/`fetchTree` invocation (or if such a path exists in an accessible binary cache). + +In the past `type = "file"` flake inputs were, in contrast to all other flake inputs, hashed in *flat* hash mode rather than *recursive* hash mode. +The difference between the two is that *flat* mode hashes are just what you get from `sha256sum` of a single file, whereas *recursive* hashes are the SHA256 sum of a NAR (Nix ARchive, a deterministic tarball-like format) of a file tree. + +Much of flakes assumes that everything is recursive-hashed including `nix flake archive`, substitution of flake inputs from binary caches, and more, which led to the substitution path code being taken if such a path is present, yielding a different store path non-deterministically. + +To fix this non-deterministic evaluation bug, we needed to break derivation hash stability, so some Nix evaluations now produce different results than previous versions of Lix. +Lix now has consistent behaviour with CppNix 2.24 with respect to `file` flake inputs: they are *always* recursively hashed. diff --git a/lix/libfetchers/fetchers.hh b/lix/libfetchers/fetchers.hh index f40389160..1e965b756 100644 --- a/lix/libfetchers/fetchers.hh +++ b/lix/libfetchers/fetchers.hh @@ -1,6 +1,7 @@ #pragma once ///@file +#include "lix/libstore/content-address.hh" #include "lix/libutil/result.hh" #include "lix/libutil/types.hh" #include "lix/libutil/hash.hh" @@ -214,7 +215,8 @@ kj::Promise> downloadFile( const std::string & url, const std::string & name, bool locked, - Headers headers = {}); + Headers headers = {}, + FileIngestionMethod ingestionMethod = FileIngestionMethod::Flat); struct DownloadTarballResult { diff --git a/lix/libfetchers/tarball.cc b/lix/libfetchers/tarball.cc index 77b37a1a8..e87098f68 100644 --- a/lix/libfetchers/tarball.cc +++ b/lix/libfetchers/tarball.cc @@ -1,5 +1,6 @@ #include "lix/libfetchers/fetchers.hh" #include "lix/libfetchers/cache.hh" +#include "lix/libstore/content-address.hh" #include "lix/libstore/filetransfer.hh" #include "lix/libstore/globals.hh" #include "lix/libfetchers/builtin-fetchers.hh" @@ -14,12 +15,46 @@ namespace nix::fetchers { +/* Note [Recursive hashing of file inputs]: + * We recursively hash `file` inputs to be consistent with the way that we look + * up the paths in binary caches (which is assumed all over the place in + * flakes, like in `nix flake archive`). + * + * It would also be easier to always hash downloadFile outputs as recursive, + * but there are a *whole bunch* of random usages of downloadFile for stuff + * from channel tarballs to GitHub API that I haven't fully figured out what + * they are doing or what the implications are for changing them. It seems that + * for things that are not directly *themselves* flake inputs, the intent is + * that they are flat-hashed, but flake inputs are always assumed + * recursive-hashed. + * + * So we make the flake usage of it do the thing that's right for flakes, and + * everything else we leave as before. + * + * Quoth Dr. Eelco Dolstra: https://github.com/NixOS/nix/pull/6548#discussion_r877921756 + * > A problem with the use of downloadFile() is that it uses + * > FileIngestionMethod::Flat instead of FileIngestionMethod::Recursive. + * > Currently it's assumed that all flake inputs use recursive+sha256 with a + * > name of "source". This allows inputs to be substituted using the narHash + * > attribute in the lock file (see Input::computeStorePath()). However, the + * > lazy trees branch will probably remove the ability to substitute inputs + * > anyway... + * + * In other words, the feature was supposed to have been implemented consistent + * with other flake input types as recursive-hashed, but it was forgotten + * before merging it into CppNix, and was later fixed by some version between + * 2.19 and 2.24, which we are now consistent with. + * + * See: https://github.com/edolstra/flake-compat/pull/44 + */ + kj::Promise> downloadFile( ref store, const std::string & url, const std::string & name, bool locked, - Headers headers) + Headers headers, + FileIngestionMethod ingestionMethod) try { // FIXME: check store @@ -27,6 +62,7 @@ try { {"type", "file"}, {"url", url}, {"name", name}, + {"ingestionMethod", uint64_t(ingestionMethod)}, }); auto cached = TRY_AWAIT(getCache()->lookupExpired(store, inAttrs)); @@ -60,7 +96,8 @@ try { throw; } - // FIXME: write to temporary file. + // FIXME: write to temporary file. or stream it. or like. anything but load + // the entire thing in memory. Attrs infoAttrs({ {"etag", res.etag}, {"url", res.effectiveUri}, @@ -77,16 +114,18 @@ try { } else { StringSink sink; sink << dumpString(data); - auto hash = hashString(HashType::SHA256, data); + auto flatHash = [&]() { return hashString(HashType::SHA256, data); }; + // See Note [Recursive hashing of file inputs]. + auto narHash = hashString(HashType::SHA256, sink.s); ValidPathInfo info { *store, name, FixedOutputInfo { - .method = FileIngestionMethod::Flat, - .hash = hash, + .method = ingestionMethod, + .hash = ingestionMethod == FileIngestionMethod::Flat ? flatHash() : narHash, .references = {}, }, - hashString(HashType::SHA256, sink.s), + narHash, }; info.narSize = sink.s.size(); auto source = AsyncStringInputStream { sink.s }; @@ -277,7 +316,8 @@ struct FileInputScheme : CurlInputScheme fetch(ref store, const Input & input) override try { auto file = - TRY_AWAIT(downloadFile(store, getStrAttr(input.attrs, "url"), input.getName(), false)); + // See Note [Recursive hashing of file inputs]. + TRY_AWAIT(downloadFile(store, getStrAttr(input.attrs, "url"), input.getName(), false, {}, FileIngestionMethod::Recursive)); co_return {std::move(file.storePath), input}; } catch (...) { co_return result::current_exception(); diff --git a/tests/functional/fetchTree-file.sh b/tests/functional/fetchTree-file.sh index 7dbc7667c..0401b1fef 100644 --- a/tests/functional/fetchTree-file.sh +++ b/tests/functional/fetchTree-file.sh @@ -4,17 +4,27 @@ clearStore cd "$TEST_ROOT" +echo foo > test_input +test_input_hash="$(nix hash path test_input)" + test_fetch_file () { - echo foo > test_input - - input_hash="$(nix hash path test_input)" - - nix eval --impure --file - <