fix!: file type flake inputs are always recursive hashed
Well that is a mess. I don't like breaking derivation compatibility, but I like non-deterministic evaluation much less. We can break compat if we own up to it, which this does. Fixes: https://git.lix.systems/lix-project/lix/issues/750 Change-Id: Ic9e2407393f1d42c2be604f80b4aa11bc872bc23
This commit is contained in:
Jade Lovelace
@@ -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.
|
||||
@@ -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<Result<DownloadFileResult>> downloadFile(
|
||||
const std::string & url,
|
||||
const std::string & name,
|
||||
bool locked,
|
||||
Headers headers = {});
|
||||
Headers headers = {},
|
||||
FileIngestionMethod ingestionMethod = FileIngestionMethod::Flat);
|
||||
|
||||
struct DownloadTarballResult
|
||||
{
|
||||
|
||||
@@ -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<Result<DownloadFileResult>> downloadFile(
|
||||
ref<Store> 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> 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();
|
||||
|
||||
@@ -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 - <<EOF
|
||||
nix eval --raw --impure --file - <<EOF
|
||||
let
|
||||
tree = builtins.fetchTree { type = "file"; url = "file://$PWD/test_input"; };
|
||||
in
|
||||
assert (tree.narHash == "$input_hash");
|
||||
tree
|
||||
assert (tree.narHash == "$test_input_hash");
|
||||
tree.outPath
|
||||
EOF
|
||||
}
|
||||
|
||||
test_substitution () {
|
||||
# fetch a URL that will never work, make sure it hits the existing one in
|
||||
# the store if it exists
|
||||
nix eval --raw --impure --file - <<EOF
|
||||
let
|
||||
tree = builtins.fetchTree { type = "file"; url = "file:///dev/null"; narHash = "$test_input_hash"; };
|
||||
in
|
||||
tree.outPath
|
||||
EOF
|
||||
}
|
||||
|
||||
@@ -113,5 +123,21 @@ EOF
|
||||
EOF
|
||||
}
|
||||
|
||||
test_fetch_file
|
||||
test_file_flake_input
|
||||
fetch_file_path=$(test_fetch_file)
|
||||
fetch_substitute_path=$(test_substitution)
|
||||
|
||||
# Substituting with a narHash should give you the same path as fetching it
|
||||
# directly, which should give you the same path as throwing it in the store
|
||||
# with recursive ingestion mode.
|
||||
# Regression test for https://git.lix.systems/lix-project/lix/issues/750
|
||||
[[ "$fetch_file_path" == "$fetch_substitute_path" ]]
|
||||
|
||||
# n.b. these are done after the first one which directly fetches the file in
|
||||
# order to make sure that substitution is *not* at play in test_fetch_file
|
||||
flat_hash_path=$(nix store add-file --name source ./test_input)
|
||||
recursive_hash_path=$(nix store add-path --name source ./test_input)
|
||||
|
||||
# Fetching the file should give you a recursive-hashed path
|
||||
[[ "$flat_hash_path" != "$recursive_hash_path" ]]
|
||||
[[ "$fetch_file_path" == "$recursive_hash_path" ]]
|
||||
|
||||
Reference in New Issue
Block a user