From 071dd2b3a4e6c0b2106f1b6f14ec26e153d97446 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 16 Feb 2024 17:00:07 +0100 Subject: [PATCH 1/2] Input: Replace 'locked' bool by isLocked() method It's better to just check whether the input has all the attributes needed to consider itself locked (e.g. whether a Git input has an 'rev' attribute). Also, the 'locked' field was actually incorrect for Git inputs: it would be set to true even for dirty worktrees. As a result, we got away with using fetchTree() internally even though fetchTree() requires a locked input in pure mode. In particular, this allowed '--override-input' to work by accident. The fix is to pass a set of "overrides" to call-flake.nix for all the unlocked inputs (i.e. the top-level flake and any --override-inputs). --- src/libexpr/flake/call-flake.nix | 61 ++++++++++++++--------- src/libexpr/flake/flake.cc | 84 ++++++++++++++++++++------------ src/libexpr/flake/flake.hh | 7 +++ src/libexpr/flake/lockfile.cc | 17 ++++--- src/libexpr/flake/lockfile.hh | 7 +-- src/libexpr/primops/fetchTree.cc | 6 +-- src/libfetchers/fetchers.cc | 11 ++--- src/libfetchers/fetchers.hh | 12 +++-- src/libfetchers/git.cc | 7 ++- src/libfetchers/github.cc | 5 ++ src/libfetchers/mercurial.cc | 5 ++ src/libfetchers/path.cc | 5 ++ src/libfetchers/tarball.cc | 5 ++ src/nix-env/nix-env.cc | 2 +- src/nix/flake.cc | 2 +- tests/functional/fetchGit.sh | 4 +- 16 files changed, 155 insertions(+), 85 deletions(-) diff --git a/src/libexpr/flake/call-flake.nix b/src/libexpr/flake/call-flake.nix index 4beb0b0fe..d0ccb1e37 100644 --- a/src/libexpr/flake/call-flake.nix +++ b/src/libexpr/flake/call-flake.nix @@ -1,20 +1,52 @@ -lockFileStr: rootSrc: rootSubdir: +# This is a helper to callFlake() to lazily fetch flake inputs. + +# The contents of the lock file, in JSON format. +lockFileStr: + +# A mapping of lock file node IDs to { sourceInfo, subdir } attrsets, +# with sourceInfo.outPath providing an InputAccessor to a previously +# fetched tree. This is necessary for possibly unlocked inputs, in +# particular the root input, but also --override-inputs pointing to +# unlocked trees. +overrides: let lockFile = builtins.fromJSON lockFileStr; + # Resolve a input spec into a node name. An input spec is + # either a node name, or a 'follows' path from the root + # node. + resolveInput = inputSpec: + if builtins.isList inputSpec + then getInputByPath lockFile.root inputSpec + else inputSpec; + + # Follow an input path (e.g. ["dwarffs" "nixpkgs"]) from the + # root node, returning the final node. + getInputByPath = nodeName: path: + if path == [] + then nodeName + else + getInputByPath + # Since this could be a 'follows' input, call resolveInput. + (resolveInput lockFile.nodes.${nodeName}.inputs.${builtins.head path}) + (builtins.tail path); + allNodes = builtins.mapAttrs (key: node: let sourceInfo = - if key == lockFile.root - then rootSrc - else fetchTree (node.info or {} // removeAttrs node.locked ["dir"]); + if overrides ? ${key} + then + overrides.${key}.sourceInfo + else + # FIXME: remove obsolete node.info. + fetchTree (node.info or {} // removeAttrs node.locked ["dir"]); - subdir = if key == lockFile.root then rootSubdir else node.locked.dir or ""; + subdir = overrides.${key}.dir or node.locked.dir or ""; outPath = sourceInfo + ((if subdir == "" then "" else "/") + subdir); @@ -24,25 +56,6 @@ let (inputName: inputSpec: allNodes.${resolveInput inputSpec}) (node.inputs or {}); - # Resolve a input spec into a node name. An input spec is - # either a node name, or a 'follows' path from the root - # node. - resolveInput = inputSpec: - if builtins.isList inputSpec - then getInputByPath lockFile.root inputSpec - else inputSpec; - - # Follow an input path (e.g. ["dwarffs" "nixpkgs"]) from the - # root node, returning the final node. - getInputByPath = nodeName: path: - if path == [] - then nodeName - else - getInputByPath - # Since this could be a 'follows' input, call resolveInput. - (resolveInput lockFile.nodes.${nodeName}.inputs.${builtins.head path}) - (builtins.tail path); - outputs = flake.outputs (inputs // { self = result; }); result = diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 451780c89..022d39cdb 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -365,6 +365,7 @@ LockedFlake lockFlake( std::map overrides; std::set explicitCliOverrides; std::set overridesUsed, updatesUsed; + std::map, StorePath> nodePaths; for (auto & i : lockFlags.inputOverrides) { overrides.insert_or_assign(i.first, FlakeInput { .ref = i.second }); @@ -535,11 +536,13 @@ LockedFlake lockFlake( } } - computeLocks( - mustRefetch - ? getFlake(state, oldLock->lockedRef, false, flakeCache, inputPath).inputs - : fakeInputs, - childNode, inputPath, oldLock, lockRootPath, parentPath, !mustRefetch); + if (mustRefetch) { + auto inputFlake = getFlake(state, oldLock->lockedRef, false, flakeCache, inputPath); + nodePaths.emplace(childNode, inputFlake.storePath); + computeLocks(inputFlake.inputs, childNode, inputPath, oldLock, lockRootPath, parentPath, false); + } else { + computeLocks(fakeInputs, childNode, inputPath, oldLock, lockRootPath, parentPath, true); + } } else { /* We need to create a new lock file entry. So fetch @@ -584,6 +587,7 @@ LockedFlake lockFlake( flake. Also, unless we already have this flake in the top-level lock file, use this flake's own lock file. */ + nodePaths.emplace(childNode, inputFlake.storePath); computeLocks( inputFlake.inputs, childNode, inputPath, oldLock @@ -596,11 +600,13 @@ LockedFlake lockFlake( } else { - auto [sourceInfo, resolvedRef, lockedRef] = fetchOrSubstituteTree( + auto [storePath, resolvedRef, lockedRef] = fetchOrSubstituteTree( state, *input.ref, useRegistries, flakeCache); auto childNode = make_ref(lockedRef, ref, false); + nodePaths.emplace(childNode, storePath); + node->inputs.insert_or_assign(id, childNode); } } @@ -615,6 +621,8 @@ LockedFlake lockFlake( // Bring in the current ref for relative path resolution if we have it auto parentPath = canonPath(state.store->toRealPath(flake.storePath) + "/" + flake.lockedRef.subdir, true); + nodePaths.emplace(newLockFile.root, flake.storePath); + computeLocks( flake.inputs, newLockFile.root, @@ -707,14 +715,6 @@ LockedFlake lockFlake( flake.lockedRef.input.getRev() && prevLockedRef.input.getRev() != flake.lockedRef.input.getRev()) warn("committed new revision '%s'", flake.lockedRef.input.getRev()->gitRev()); - - /* Make sure that we picked up the change, - i.e. the tree should usually be dirty - now. Corner case: we could have reverted from a - dirty to a clean tree! */ - if (flake.lockedRef.input == prevLockedRef.input - && !flake.lockedRef.input.isLocked()) - throw Error("'%s' did not change after I updated its 'flake.lock' file; is 'flake.lock' under version control?", flake.originalRef); } } else throw Error("cannot write modified lock file of flake '%s' (use '--no-write-lock-file' to ignore)", topRef); @@ -724,7 +724,11 @@ LockedFlake lockFlake( } } - return LockedFlake { .flake = std::move(flake), .lockFile = std::move(newLockFile) }; + return LockedFlake { + .flake = std::move(flake), + .lockFile = std::move(newLockFile), + .nodePaths = std::move(nodePaths) + }; } catch (Error & e) { e.addTrace({}, "while updating the lock file of flake '%s'", flake.lockedRef.to_string()); @@ -736,30 +740,48 @@ void callFlake(EvalState & state, const LockedFlake & lockedFlake, Value & vRes) { - auto vLocks = state.allocValue(); - auto vRootSrc = state.allocValue(); - auto vRootSubdir = state.allocValue(); - auto vTmp1 = state.allocValue(); - auto vTmp2 = state.allocValue(); + experimentalFeatureSettings.require(Xp::Flakes); - vLocks->mkString(lockedFlake.lockFile.to_string()); + auto [lockFileStr, keyMap] = lockedFlake.lockFile.to_string(); - emitTreeAttrs( - state, - lockedFlake.flake.storePath, - lockedFlake.flake.lockedRef.input, - *vRootSrc, - false, - lockedFlake.flake.forceDirty); + auto overrides = state.buildBindings(lockedFlake.nodePaths.size()); - vRootSubdir->mkString(lockedFlake.flake.lockedRef.subdir); + for (auto & [node, storePath] : lockedFlake.nodePaths) { + auto override = state.buildBindings(2); + + auto & vSourceInfo = override.alloc(state.symbols.create("sourceInfo")); + + auto lockedNode = node.dynamic_pointer_cast(); + + emitTreeAttrs( + state, + storePath, + lockedNode ? lockedNode->lockedRef.input : lockedFlake.flake.lockedRef.input, + vSourceInfo, + false, + !lockedNode && lockedFlake.flake.forceDirty); + + auto key = keyMap.find(node); + assert(key != keyMap.end()); + + override + .alloc(state.symbols.create("dir")) + .mkString(lockedNode ? lockedNode->lockedRef.subdir : lockedFlake.flake.lockedRef.subdir); + + overrides.alloc(state.symbols.create(key->second)).mkAttrs(override); + } + + auto & vOverrides = state.allocValue()->mkAttrs(overrides); auto vCallFlake = state.allocValue(); state.evalFile(state.callFlakeInternal, *vCallFlake); + auto vTmp1 = state.allocValue(); + auto vLocks = state.allocValue(); + vLocks->mkString(lockFileStr); state.callFunction(*vCallFlake, *vLocks, *vTmp1, noPos); - state.callFunction(*vTmp1, *vRootSrc, *vTmp2, noPos); - state.callFunction(*vTmp2, *vRootSubdir, vRes, noPos); + + state.callFunction(*vTmp1, vOverrides, vRes, noPos); } static void prim_getFlake(EvalState & state, const PosIdx pos, Value * * args, Value & v) diff --git a/src/libexpr/flake/flake.hh b/src/libexpr/flake/flake.hh index d5ad3eade..19b680c56 100644 --- a/src/libexpr/flake/flake.hh +++ b/src/libexpr/flake/flake.hh @@ -103,6 +103,13 @@ struct LockedFlake Flake flake; LockFile lockFile; + /** + * Store paths of nodes that have been fetched in + * lockFlake(); in particular, the root node and the overriden + * inputs. + */ + std::map, StorePath> nodePaths; + Fingerprint getFingerprint() const; }; diff --git a/src/libexpr/flake/lockfile.cc b/src/libexpr/flake/lockfile.cc index 3e99fb2d4..2c16dc802 100644 --- a/src/libexpr/flake/lockfile.cc +++ b/src/libexpr/flake/lockfile.cc @@ -38,7 +38,7 @@ LockedNode::LockedNode(const nlohmann::json & json) , isFlake(json.find("flake") != json.end() ? (bool) json["flake"] : true) { if (!lockedRef.input.isLocked()) - throw Error("lock file contains mutable lock '%s'", + throw Error("lock file contains unlocked input '%s'", fetchers::attrsToJSON(lockedRef.input.toAttrs())); } @@ -134,10 +134,10 @@ LockFile::LockFile(const nlohmann::json & json, const Path & path) // a bit since we don't need to worry about cycles. } -nlohmann::json LockFile::toJSON() const +std::pair LockFile::toJSON() const { nlohmann::json nodes; - std::unordered_map, std::string> nodeKeys; + KeyMap nodeKeys; std::unordered_set keys; std::function node)> dumpNode; @@ -194,12 +194,13 @@ nlohmann::json LockFile::toJSON() const json["root"] = dumpNode("root", root); json["nodes"] = std::move(nodes); - return json; + return {json, std::move(nodeKeys)}; } -std::string LockFile::to_string() const +std::pair LockFile::to_string() const { - return toJSON().dump(2); + auto [json, nodeKeys] = toJSON(); + return {json.dump(2), std::move(nodeKeys)}; } LockFile LockFile::read(const Path & path) @@ -210,7 +211,7 @@ LockFile LockFile::read(const Path & path) std::ostream & operator <<(std::ostream & stream, const LockFile & lockFile) { - stream << lockFile.toJSON().dump(2); + stream << lockFile.toJSON().first.dump(2); return stream; } @@ -243,7 +244,7 @@ std::optional LockFile::isUnlocked() const bool LockFile::operator ==(const LockFile & other) const { // FIXME: slow - return toJSON() == other.toJSON(); + return toJSON().first == other.toJSON().first; } bool LockFile::operator !=(const LockFile & other) const diff --git a/src/libexpr/flake/lockfile.hh b/src/libexpr/flake/lockfile.hh index 5a1493404..57a7202a2 100644 --- a/src/libexpr/flake/lockfile.hh +++ b/src/libexpr/flake/lockfile.hh @@ -59,14 +59,15 @@ struct LockFile typedef std::map, std::string> KeyMap; - nlohmann::json toJSON() const; + std::pair toJSON() const; - std::string to_string() const; + std::pair to_string() const; static LockFile read(const Path & path); /** - * Check whether this lock file has any unlocked inputs. + * Check whether this lock file has any unlocked inputs. If so, + * return one. */ std::optional isUnlocked() const; diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index 1997d5513..b4d9a6189 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -24,8 +24,6 @@ void emitTreeAttrs( bool emptyRevFallback, bool forceDirty) { - assert(input.isLocked()); - auto attrs = state.buildBindings(100); state.mkStorePathString(storePath, attrs.alloc(state.sOutPath)); @@ -176,8 +174,8 @@ static void fetchTree( fetcher = "fetchGit"; state.error( - "in pure evaluation mode, %s requires a locked input", - fetcher + "in pure evaluation mode, '%s' will not fetch unlocked input '%s'", + fetcher, input.to_string() ).atPos(pos).debugThrow(); } diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index 9a534c1e2..363ad018e 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -45,12 +45,8 @@ static void fixupInput(Input & input) // Check common attributes. input.getType(); input.getRef(); - if (input.getRev()) - input.locked = true; input.getRevCount(); input.getLastModified(); - if (input.getNarHash()) - input.locked = true; } Input Input::fromURL(const ParsedURL & url, bool requireTree) @@ -140,6 +136,11 @@ bool Input::isDirect() const return !scheme || scheme->isDirect(*this); } +bool Input::isLocked() const +{ + return scheme && scheme->isLocked(*this); +} + Attrs Input::toAttrs() const { return attrs; @@ -222,8 +223,6 @@ std::pair Input::fetch(ref store) const input.to_string(), *prevRevCount); } - input.locked = true; - return {std::move(storePath), input}; } diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index 036647830..472fba6f4 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -29,7 +29,6 @@ struct Input std::shared_ptr scheme; // note: can be null Attrs attrs; - bool locked = false; /** * path of the parent of this input, used for relative path resolution @@ -71,7 +70,7 @@ public: * Check whether this is a "locked" input, that is, * one that contains a commit hash or content hash. */ - bool isLocked() const { return locked; } + bool isLocked() const; bool operator ==(const Input & other) const; @@ -121,7 +120,6 @@ public: std::optional getFingerprint(ref store) const; }; - /** * The `InputScheme` represents a type of fetcher. Each fetcher * registers with nix at startup time. When processing an `Input`, @@ -196,6 +194,14 @@ struct InputScheme */ virtual std::optional getFingerprint(ref store, const Input & input) const { return std::nullopt; } + + /** + * Return `true` if this input is considered "locked", i.e. it has + * attributes like a Git revision or NAR hash that uniquely + * identify its contents. + */ + virtual bool isLocked(const Input & input) const + { return false; } }; void registerInputScheme(std::shared_ptr && fetcher); diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 97ef35b51..87d114276 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -737,8 +737,6 @@ struct GitInputScheme : InputScheme ? getLastModified(repoInfo, repoInfo.url, *repoInfo.workdirInfo.headRev) : 0); - input.locked = true; // FIXME - return {accessor, std::move(input)}; } @@ -775,6 +773,11 @@ struct GitInputScheme : InputScheme else return std::nullopt; } + + bool isLocked(const Input & input) const override + { + return (bool) input.getRev(); + } }; static auto rGitInputScheme = OnStartup([] { registerInputScheme(std::make_unique()); }); diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index e6fbece13..76f94337b 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -280,6 +280,11 @@ struct GitArchiveInputScheme : InputScheme return {accessor, input}; } + bool isLocked(const Input & input) const override + { + return (bool) input.getRev(); + } + std::optional experimentalFeature() const override { return Xp::Flakes; diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc index 55e2eae03..a5f55a44e 100644 --- a/src/libfetchers/mercurial.cc +++ b/src/libfetchers/mercurial.cc @@ -347,6 +347,11 @@ struct MercurialInputScheme : InputScheme return makeResult(infoAttrs, std::move(storePath)); } + bool isLocked(const Input & input) const override + { + return (bool) input.getRev(); + } + std::optional getFingerprint(ref store, const Input & input) const override { if (auto rev = input.getRev()) diff --git a/src/libfetchers/path.cc b/src/libfetchers/path.cc index d3b0e475d..276fd1b36 100644 --- a/src/libfetchers/path.cc +++ b/src/libfetchers/path.cc @@ -87,6 +87,11 @@ struct PathInputScheme : InputScheme writeFile((CanonPath(getAbsPath(input)) / path).abs(), contents); } + bool isLocked(const Input & input) const override + { + return (bool) input.getNarHash(); + } + CanonPath getAbsPath(const Input & input) const { auto path = getStrAttr(input.attrs, "path"); diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index 3b7709440..1d80fd880 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -260,6 +260,11 @@ struct CurlInputScheme : InputScheme url.query.insert_or_assign("narHash", narHash->to_string(HashFormat::SRI, true)); return url; } + + bool isLocked(const Input & input) const override + { + return (bool) input.getNarHash(); + } }; struct FileInputScheme : CurlInputScheme diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index 1f311733b..5e3de20c5 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -143,7 +143,7 @@ static void getAllExprs(EvalState & state, } /* Load the expression on demand. */ auto vArg = state.allocValue(); - vArg->mkString(path2.path.abs()); + vArg->mkPath(path2); if (seen.size() == maxAttrs) throw Error("too many Nix expressions in directory '%1%'", path); attrs.alloc(attrName).mkApp(&state.getBuiltin("import"), vArg); diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 4504bb22e..131589f35 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -224,7 +224,7 @@ struct CmdFlakeMetadata : FlakeCommand, MixJSON if (auto lastModified = flake.lockedRef.input.getLastModified()) j["lastModified"] = *lastModified; j["path"] = store->printStorePath(flake.storePath); - j["locks"] = lockedFlake.lockFile.toJSON(); + j["locks"] = lockedFlake.lockFile.toJSON().first; logger->cout("%s", j.dump()); } else { logger->cout( diff --git a/tests/functional/fetchGit.sh b/tests/functional/fetchGit.sh index 856c0e534..3f2d0d5fb 100644 --- a/tests/functional/fetchGit.sh +++ b/tests/functional/fetchGit.sh @@ -70,7 +70,7 @@ path2=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$repo; rev = \" [[ $(nix eval --raw --expr "builtins.readFile (fetchGit { url = file://$repo; rev = \"$rev2\"; } + \"/hello\")") = world ]] # But without a hash, it fails -expectStderr 1 nix eval --expr 'builtins.fetchGit "file:///foo"' | grepQuiet "fetchGit requires a locked input" +expectStderr 1 nix eval --expr 'builtins.fetchGit "file:///foo"' | grepQuiet "'fetchGit' will not fetch unlocked input" # Fetch again. This should be cached. mv $repo ${repo}-tmp @@ -211,7 +211,7 @@ path6=$(nix eval --impure --raw --expr "(builtins.fetchTree { type = \"git\"; ur [[ $path3 = $path6 ]] [[ $(nix eval --impure --expr "(builtins.fetchTree { type = \"git\"; url = \"file://$TEST_ROOT/shallow\"; ref = \"dev\"; shallow = true; }).revCount or 123") == 123 ]] -expectStderr 1 nix eval --expr 'builtins.fetchTree { type = "git"; url = "file:///foo"; }' | grepQuiet "fetchTree requires a locked input" +expectStderr 1 nix eval --expr 'builtins.fetchTree { type = "git"; url = "file:///foo"; }' | grepQuiet "'fetchTree' will not fetch unlocked input" # Explicit ref = "HEAD" should work, and produce the same outPath as without ref path7=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = \"file://$repo\"; ref = \"HEAD\"; }).outPath") From 09d76e512a468ad65bedaeda56871de7043849b0 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 21 Feb 2024 12:08:18 +0100 Subject: [PATCH 2/2] GitArchiveInputScheme: Require a NAR hash --- src/libfetchers/github.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index 76f94337b..a48c99a0b 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -282,7 +282,11 @@ struct GitArchiveInputScheme : InputScheme bool isLocked(const Input & input) const override { - return (bool) input.getRev(); + /* Since we can't verify the integrity of the tarball from the + Git revision alone, we also require a NAR hash for + locking. FIXME: in the future, we may want to require a Git + tree hash instead of a NAR hash. */ + return input.getRev().has_value() && input.getNarHash().has_value(); } std::optional experimentalFeature() const override