From 8a0a55fe12cd3598729400fa4e9314b4b0490fb9 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 2 Jun 2022 13:48:55 +0200 Subject: [PATCH] Fix subflake handling Relative 'path:' flake inputs now use the containing flake's InputAccessor. This has the following implications: * They're no longer locked in the lock file. * They don't cause an additional copy to the store. * They can reference the containing directory (i.e. a subflake can have an input '../foo', so long as it doesn't go outside the top-level containing flake). Note: this is not a complete fix for subflake handling, since the lock file currently makes it ambiguous what the containing flake is. We'll probably need to add another field to the lock file for that. Fixes #6352. --- src/libexpr/flake/flake.cc | 74 ++++++++++++++++++----------------- src/libexpr/flake/lockfile.cc | 9 +++-- src/libfetchers/fetchers.cc | 5 +++ src/libfetchers/fetchers.hh | 5 +++ src/libfetchers/path.cc | 5 +++ tests/flakes.sh | 14 ++++--- 6 files changed, 69 insertions(+), 43 deletions(-) diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 81a1d3090..1023e0071 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -91,7 +91,6 @@ static std::map parseFlakeInputs( EvalState & state, Value * value, const PosIdx pos, - const std::optional & baseDir, const InputPath & lockRootPath); static FlakeInput parseFlakeInput( @@ -99,7 +98,6 @@ static FlakeInput parseFlakeInput( const std::string & inputName, Value * value, const PosIdx pos, - const std::optional & baseDir, const InputPath & lockRootPath) { expectType(state, nAttrs, *value, pos); @@ -124,7 +122,7 @@ static FlakeInput parseFlakeInput( expectType(state, nBool, *attr.value, attr.pos); input.isFlake = attr.value->boolean; } else if (attr.name == sInputs) { - input.overrides = parseFlakeInputs(state, attr.value, attr.pos, baseDir, lockRootPath); + input.overrides = parseFlakeInputs(state, attr.value, attr.pos, lockRootPath); } else if (attr.name == sFollows) { expectType(state, nString, *attr.value, attr.pos); auto follows(parseInputPath(attr.value->string.s)); @@ -166,7 +164,7 @@ static FlakeInput parseFlakeInput( if (!attrs.empty()) throw Error("unexpected flake input attribute '%s', at %s", attrs.begin()->first, state.positions[pos]); if (url) - input.ref = parseFlakeRef(*url, baseDir, true, input.isFlake); + input.ref = parseFlakeRef(*url, {}, true, input.isFlake); } if (!input.follows && !input.ref) @@ -179,7 +177,6 @@ static std::map parseFlakeInputs( EvalState & state, Value * value, const PosIdx pos, - const std::optional & baseDir, const InputPath & lockRootPath) { std::map inputs; @@ -192,7 +189,6 @@ static std::map parseFlakeInputs( state.symbols[inputAttr.name], inputAttr.value, inputAttr.pos, - baseDir, lockRootPath)); } @@ -204,11 +200,11 @@ static Flake readFlake( const FlakeRef & originalRef, const FlakeRef & resolvedRef, const FlakeRef & lockedRef, - InputAccessor & accessor, + const SourcePath & rootDir, const InputPath & lockRootPath) { CanonPath flakeDir(resolvedRef.subdir); - SourcePath flakePath{accessor, flakeDir + CanonPath("flake.nix")}; + auto flakePath = rootDir + flakeDir + "flake.nix"; if (!flakePath.pathExists()) throw Error("source tree referenced by '%s' does not contain a file named '%s'", resolvedRef, flakePath.path); @@ -233,7 +229,7 @@ static Flake readFlake( auto sInputs = state.symbols.create("inputs"); if (auto inputs = vInfo.attrs->get(sInputs)) - flake.inputs = parseFlakeInputs(state, inputs->value, inputs->pos, flakeDir.abs(), lockRootPath); + flake.inputs = parseFlakeInputs(state, inputs->value, inputs->pos, lockRootPath); auto sOutputs = state.symbols.create("outputs"); @@ -322,7 +318,9 @@ static Flake getFlake( auto [accessor, lockedRef] = resolvedRef.lazyFetch(state.store); - return readFlake(state, originalRef, resolvedRef, lockedRef, state.registerAccessor(accessor), lockRootPath); + ; + + return readFlake(state, originalRef, resolvedRef, lockedRef, SourcePath {state.registerAccessor(accessor), CanonPath::root}, lockRootPath); } Flake getFlake(EvalState & state, const FlakeRef & originalRef, bool allowLookup, FlakeCache & flakeCache) @@ -450,6 +448,22 @@ LockedFlake lockFlake( assert(input.ref); + /* Get the input flake, resolve 'path:./...' + flakerefs relative to the parent flake. */ + auto getInputFlake = [&]() + { + if (input.ref->input.isRelative()) { + SourcePath inputSourcePath { + parentPath.accessor, + CanonPath(*input.ref->input.getSourcePath(), *parentPath.path.parent()) + }; + // FIXME: we need to record in the lock + // file what the parent flake is. + return readFlake(state, *input.ref, *input.ref, *input.ref, inputSourcePath, inputPath); + } else + return getFlake(state, *input.ref, useRegistries, flakeCache, inputPath); + }; + /* Do we have an entry in the existing lock file? And we don't have a --update-input flag for this input? */ std::shared_ptr oldLock; @@ -523,39 +537,27 @@ LockedFlake lockFlake( } } - auto localPath(parentPath); - #if 0 - // If this input is a path, recurse it down. - // This allows us to resolve path inputs relative to the current flake. - if ((*input.ref).input.getType() == "path") - localPath = absPath(*input.ref->input.getSourcePath(), parentPath); - #endif - computeLocks( - mustRefetch - ? getFlake(state, oldLock->lockedRef, false, flakeCache, inputPath).inputs - : fakeInputs, - childNode, inputPath, oldLock, lockRootPath, parentPath, !mustRefetch); + if (mustRefetch) { + auto inputFlake = getInputFlake(); + computeLocks(inputFlake.inputs, childNode, inputPath, oldLock, lockRootPath, inputFlake.path, !mustRefetch); + } else { + // FIXME: parentPath is wrong here, we + // should pass a lambda that lazily + // fetches the parent flake if needed + // (i.e. getInputFlake()). + computeLocks(fakeInputs, childNode, inputPath, oldLock, lockRootPath, parentPath, !mustRefetch); + } } else { /* We need to create a new lock file entry. So fetch this input. */ debug("creating new input '%s'", inputPathS); - if (!lockFlags.allowUnlocked && !input.ref->input.isLocked()) + if (!lockFlags.allowUnlocked && !input.ref->input.isLocked() && !input.ref->input.isRelative()) throw Error("cannot update unlocked flake input '%s' in pure mode", inputPathS); if (input.isFlake) { - auto localPath(parentPath); - FlakeRef localRef = *input.ref; - - #if 0 - // If this input is a path, recurse it down. - // This allows us to resolve path inputs relative to the current flake. - if (localRef.input.getType() == "path") - localPath = absPath(*input.ref->input.getSourcePath(), parentPath); - #endif - - auto inputFlake = getFlake(state, localRef, useRegistries, flakeCache, inputPath); + auto inputFlake = getInputFlake(); /* Note: in case of an --override-input, we use the *original* ref (input2.ref) for the @@ -585,7 +587,9 @@ LockedFlake lockFlake( oldLock ? std::dynamic_pointer_cast(oldLock) : readLockFile(inputFlake).root, - oldLock ? lockRootPath : inputPath, localPath, false); + oldLock ? lockRootPath : inputPath, + inputFlake.path, + false); } else { diff --git a/src/libexpr/flake/lockfile.cc b/src/libexpr/flake/lockfile.cc index f9a0c528f..ae67f8e61 100644 --- a/src/libexpr/flake/lockfile.cc +++ b/src/libexpr/flake/lockfile.cc @@ -35,7 +35,7 @@ LockedNode::LockedNode(const nlohmann::json & json) , originalRef(getFlakeRef(json, "original", nullptr)) , isFlake(json.find("flake") != json.end() ? (bool) json["flake"] : true) { - if (!lockedRef.input.isLocked()) + if (!lockedRef.input.isLocked() && !lockedRef.input.isRelative()) throw Error("lock file contains unlocked input '%s'", fetchers::attrsToJSON(lockedRef.input.toAttrs())); } @@ -215,8 +215,11 @@ bool LockFile::isLocked() const for (auto & i : nodes) { if (i == root) continue; - auto lockedNode = std::dynamic_pointer_cast(i); - if (lockedNode && !lockedNode->lockedRef.input.isLocked()) return false; + auto node = std::dynamic_pointer_cast(i); + if (node + && !node->lockedRef.input.isLocked() + && !node->lockedRef.input.isRelative()) + return false; } return true; diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index cce4193ea..9f9a9e728 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -87,6 +87,11 @@ Attrs Input::toAttrs() const return attrs; } +bool Input::isRelative() const +{ + return scheme->isRelative(*this); +} + bool Input::hasAllInfo() const { return getNarHash() && scheme && scheme->hasAllInfo(*this); diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index cc53e76b8..e8c897d4f 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -63,6 +63,8 @@ public: one that contains a commit hash or content hash. */ bool isLocked() const { return locked; } + bool isRelative() const; + bool hasAllInfo() const; bool operator ==(const Input & other) const; @@ -151,6 +153,9 @@ struct InputScheme { throw UnimplementedError("getAccessor"); } + + virtual bool isRelative(const Input & input) const + { return false; } }; void registerInputScheme(std::shared_ptr && fetcher); diff --git a/src/libfetchers/path.cc b/src/libfetchers/path.cc index d51a7f362..2706e8e91 100644 --- a/src/libfetchers/path.cc +++ b/src/libfetchers/path.cc @@ -66,6 +66,11 @@ struct PathInputScheme : InputScheme }; } + bool isRelative(const Input & input) const override + { + return !hasPrefix(*input.getSourcePath(), "/"); + } + bool hasAllInfo(const Input & input) override { return true; diff --git a/tests/flakes.sh b/tests/flakes.sh index 2a1b73e13..3aec64c14 100644 --- a/tests/flakes.sh +++ b/tests/flakes.sh @@ -733,7 +733,9 @@ cat > $flakeFollowsA/flake.nix < $flakeFollowsB/flake.nix < $flakeFollowsC/flake.nix < $flakeFollowsA/flake.nix <&1 | grep 'points outside' +nix flake lock $flakeFollowsA # Test flake in store does not evaluate rm -rf $badFlakeDir