From d9ad3723d59d9df2fb3c89335b5d9239f1860ec9 Mon Sep 17 00:00:00 2001 From: Nick Van den Broeck Date: Wed, 1 May 2019 11:38:48 +0200 Subject: [PATCH 1/3] Fixed issue 65 lockfile updating --- src/libexpr/primops/flake.cc | 91 +++++++++++++++++++++--------------- src/libexpr/primops/flake.hh | 6 +-- src/nix/command.hh | 4 +- src/nix/flake.cc | 9 ++-- src/nix/installables.cc | 16 +++---- tests/flakes.sh | 39 ++++++++-------- 6 files changed, 90 insertions(+), 75 deletions(-) diff --git a/src/libexpr/primops/flake.cc b/src/libexpr/primops/flake.cc index c73487585..88eadff55 100644 --- a/src/libexpr/primops/flake.cc +++ b/src/libexpr/primops/flake.cc @@ -289,8 +289,6 @@ Flake getFlake(EvalState & state, const FlakeRef & flakeRef, bool impureIsAllowe FlakeRef resolvedRef = sourceInfo.resolvedRef; - resolvedRef = sourceInfo.resolvedRef; // `resolvedRef` is now immutable - state.store->assertStorePath(sourceInfo.storePath); if (state.allowedPaths) @@ -368,35 +366,59 @@ NonFlake getNonFlake(EvalState & state, const FlakeRef & flakeRef, FlakeAlias al return nonFlake; } -/* Given a flake reference, recursively fetch it and its - dependencies. - FIXME: this should return a graph of flakes. -*/ -ResolvedFlake resolveFlake(EvalState & state, const FlakeRef & topRef, - RegistryAccess registryAccess, bool isTopFlake) +LockFile entryToLockFile(const LockFile::FlakeEntry & entry) +{ + LockFile lockFile; + lockFile.flakeEntries = entry.flakeEntries; + lockFile.nonFlakeEntries = entry.nonFlakeEntries; + return lockFile; +} + +ResolvedFlake resolveFlakeFromLockFile(EvalState & state, const FlakeRef & flakeRef, RegistryAccess registryAccess, + LockFile lockFile, bool isTopFlake = false) { bool allowRegistries = registryAccess == AllowRegistry || (registryAccess == AllowRegistryAtTop && isTopFlake); - Flake flake = getFlake(state, topRef, allowRegistries); - LockFile lockFile; - - if (isTopFlake) - lockFile = readLockFile(flake.storePath + flake.resolvedRef.subdir + "/flake.lock"); // FIXME: symlink attack + Flake flake = getFlake(state, flakeRef, allowRegistries); ResolvedFlake deps(flake); - for (auto & nonFlakeInfo : flake.nonFlakeRequires) - deps.nonFlakeDeps.push_back(getNonFlake(state, nonFlakeInfo.second, nonFlakeInfo.first)); + for (auto & nonFlakeInfo : flake.nonFlakeRequires) { + FlakeRef ref = nonFlakeInfo.second; + auto i = lockFile.nonFlakeEntries.find(nonFlakeInfo.first); + if (i != lockFile.nonFlakeEntries.end()) ref = i->second.ref; + deps.nonFlakeDeps.push_back(getNonFlake(state, ref, nonFlakeInfo.first)); + } for (auto newFlakeRef : flake.requires) { + FlakeRef ref = newFlakeRef; + LockFile newLockFile; auto i = lockFile.flakeEntries.find(newFlakeRef); - if (i != lockFile.flakeEntries.end()) newFlakeRef = i->second.ref; - // FIXME: propagate lockFile downwards - deps.flakeDeps.push_back(resolveFlake(state, newFlakeRef, registryAccess, false)); + if (i != lockFile.flakeEntries.end()) { // Propagate lockFile downwards if possible + ref = i->second.ref; + newLockFile = entryToLockFile(i->second); + } + deps.flakeDeps.push_back(resolveFlakeFromLockFile(state, ref, registryAccess, newLockFile)); } return deps; } +/* Given a flake reference, recursively fetch it and its dependencies. + FIXME: this should return a graph of flakes. +*/ +ResolvedFlake resolveFlake(EvalState & state, const FlakeRef & topRef, RegistryAccess registryAccess, + bool recreateLockFile) +{ + bool allowRegistries = registryAccess == AllowRegistry || registryAccess == AllowRegistryAtTop; + Flake flake = getFlake(state, topRef, allowRegistries); + LockFile lockFile; + + if (!recreateLockFile) // If recreateLockFile, start with an empty lockfile + lockFile = readLockFile(flake.storePath + "/flake.lock"); // FIXME: symlink attack + + return resolveFlakeFromLockFile(state, topRef, registryAccess, lockFile, true); +} + LockFile::FlakeEntry dependenciesToFlakeEntry(const ResolvedFlake & resolvedFlake) { LockFile::FlakeEntry entry(resolvedFlake.flake.resolvedRef, resolvedFlake.flake.hash); @@ -410,31 +432,25 @@ LockFile::FlakeEntry dependenciesToFlakeEntry(const ResolvedFlake & resolvedFlak return entry; } -static LockFile makeLockFile(EvalState & evalState, FlakeRef & flakeRef) +static LockFile makeLockFile(EvalState & evalState, FlakeRef & flakeRef, bool recreateLockFile) { - ResolvedFlake resFlake = resolveFlake(evalState, flakeRef, AllowRegistry); - LockFile::FlakeEntry entry = dependenciesToFlakeEntry(resFlake); - LockFile lockFile; - lockFile.flakeEntries = entry.flakeEntries; - lockFile.nonFlakeEntries = entry.nonFlakeEntries; - return lockFile; + ResolvedFlake resFlake = resolveFlake(evalState, flakeRef, AllowRegistry, recreateLockFile); + return entryToLockFile(dependenciesToFlakeEntry(resFlake)); } -void updateLockFile(EvalState & state, const FlakeUri & flakeUri) +void updateLockFile(EvalState & state, const FlakeUri & uri, bool recreateLockFile) { - // FIXME: We are writing the lockfile to the store here! Very bad practice! - FlakeRef flakeRef = FlakeRef(flakeUri); + FlakeRef flakeRef = FlakeRef(uri); + auto lockFile = makeLockFile(state, flakeRef, recreateLockFile); if (auto refData = std::get_if(&flakeRef.data)) { - auto lockFile = makeLockFile(state, flakeRef); - writeLockFile(lockFile, refData->path + "/" + flakeRef.subdir + "/flake.lock"); + writeLockFile(lockFile, refData->path + (flakeRef.subdir == "" ? "" : "/" + flakeRef.subdir) + "/flake.lock"); // Hack: Make sure that flake.lock is visible to Git. Otherwise, // exportGit will fail to copy it to the Nix store. - runProgram("git", true, - { "-C", refData->path, "add", "--intent-to-add", - (flakeRef.subdir == "" ? "" : flakeRef.subdir + "/") + "flake.lock" }); + runProgram("git", true, { "-C", refData->path, "add", + (flakeRef.subdir == "" ? "" : flakeRef.subdir + "/") + "flake.lock" }); } else - throw Error("flakeUri %s can't be updated because it is not a path", flakeUri); + throw Error("flakeUri %s can't be updated because it is not a path", uri); } void callFlake(EvalState & state, const ResolvedFlake & resFlake, Value & v) @@ -485,16 +501,17 @@ void callFlake(EvalState & state, const ResolvedFlake & resFlake, Value & v) // Return the `provides` of the top flake, while assigning to `v` the provides // of the dependencies as well. -void makeFlakeValue(EvalState & state, const FlakeRef & flakeRef, RegistryAccess registryAccess, Value & v) +void makeFlakeValue(EvalState & state, const FlakeRef & flakeRef, RegistryAccess registryAccess, Value & v, bool recreateLockFile) { - callFlake(state, resolveFlake(state, flakeRef, registryAccess), v); + callFlake(state, resolveFlake(state, flakeRef, registryAccess, recreateLockFile), v); } // This function is exposed to be used in nix files. static void prim_getFlake(EvalState & state, const Pos & pos, Value * * args, Value & v) { makeFlakeValue(state, state.forceStringNoCtx(*args[0], pos), - evalSettings.pureEval ? DisallowRegistry : AllowRegistryAtTop, v); + evalSettings.pureEval ? DisallowRegistry : AllowRegistryAtTop, v, false); + // `recreateLockFile == false` because this is the evaluation stage, which should be pure, and hence not recreate lockfiles. } static RegisterPrimOp r2("getFlake", 1, prim_getFlake); diff --git a/src/libexpr/primops/flake.hh b/src/libexpr/primops/flake.hh index 691f68b0a..e3481e99e 100644 --- a/src/libexpr/primops/flake.hh +++ b/src/libexpr/primops/flake.hh @@ -45,7 +45,7 @@ Path getUserRegistryPath(); enum RegistryAccess { DisallowRegistry, AllowRegistry, AllowRegistryAtTop }; -void makeFlakeValue(EvalState & state, const FlakeRef & flakeRef, RegistryAccess registryAccess, Value & v); +void makeFlakeValue(EvalState & state, const FlakeRef & flakeRef, RegistryAccess registryAccess, Value & v, bool recreateLockFile); std::shared_ptr readRegistry(const Path &); @@ -103,9 +103,9 @@ struct ResolvedFlake ResolvedFlake(const Flake & flake) : flake(flake) {} }; -ResolvedFlake resolveFlake(EvalState &, const FlakeRef &, RegistryAccess registryAccess, bool isTopFlake = true); +ResolvedFlake resolveFlake(EvalState &, const FlakeRef &, RegistryAccess, bool recreateLockFile); -void updateLockFile(EvalState &, const FlakeUri &); +void updateLockFile(EvalState &, const FlakeUri &, bool recreateLockFile); void gitCloneFlake (std::string flakeUri, EvalState &, Registries, Path); } diff --git a/src/nix/command.hh b/src/nix/command.hh index 640c6cd16..32a5047a8 100644 --- a/src/nix/command.hh +++ b/src/nix/command.hh @@ -76,10 +76,10 @@ struct SourceExprCommand : virtual Args, StoreCommand, MixEvalArgs { std::optional file; - bool updateLockFile = true; - SourceExprCommand(); + bool recreateLockFile = false; + ref getEvalState(); std::vector> parseInstallables( diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 0af368570..d2cdf6fc9 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -114,7 +114,8 @@ struct CmdFlakeDeps : FlakeCommand, MixJSON, StoreCommand, MixEvalArgs FlakeRef flakeRef(flakeUri); - ResolvedFlake resFlake = resolveFlake(*evalState, flakeRef, AllowRegistryAtTop); + bool recreateLockFile = false; + ResolvedFlake resFlake = resolveFlake(*evalState, flakeRef, AllowRegistryAtTop, recreateLockFile); std::queue todo; todo.push(resFlake); @@ -132,7 +133,7 @@ struct CmdFlakeDeps : FlakeCommand, MixJSON, StoreCommand, MixEvalArgs } }; -struct CmdFlakeUpdate : StoreCommand, GitRepoCommand, MixEvalArgs +struct CmdFlakeUpdate : StoreCommand, FlakeCommand, MixEvalArgs { std::string name() override { @@ -148,8 +149,8 @@ struct CmdFlakeUpdate : StoreCommand, GitRepoCommand, MixEvalArgs { auto evalState = std::make_shared(searchPath, store); - if (gitPath == "") gitPath = absPath("."); - updateLockFile(*evalState, gitPath); + bool recreateLockFile = true; + updateLockFile(*evalState, flakeUri, recreateLockFile); } }; diff --git a/src/nix/installables.cc b/src/nix/installables.cc index db67952e1..6d784002a 100644 --- a/src/nix/installables.cc +++ b/src/nix/installables.cc @@ -23,9 +23,9 @@ SourceExprCommand::SourceExprCommand() .dest(&file); mkFlag() - .longName("no-update") - .description("don't create/update flake lock files") - .set(&updateLockFile, false); + .longName("recreate-lock-file") + .description("recreate lock file from scratch") + .set(&recreateLockFile, true); } ref SourceExprCommand::getEvalState() @@ -157,13 +157,11 @@ struct InstallableFlake : InstallableValue Value * toValue(EvalState & state) override { - auto path = std::get_if(&flakeRef.data); - if (cmd.updateLockFile && path) { - updateLockFile(state, path->path); - } - auto vFlake = state.allocValue(); - makeFlakeValue(state, flakeRef, AllowRegistryAtTop, *vFlake); + if (std::get_if(&flakeRef.data)) + updateLockFile(state, flakeRef.to_string(), cmd.recreateLockFile); + + makeFlakeValue(state, flakeRef, AllowRegistryAtTop, *vFlake, cmd.recreateLockFile); auto vProvides = (*vFlake->attrs->get(state.symbols.create("provides")))->value; diff --git a/tests/flakes.sh b/tests/flakes.sh index 40ba42715..8b68aea65 100644 --- a/tests/flakes.sh +++ b/tests/flakes.sh @@ -9,11 +9,11 @@ clearStore registry=$TEST_ROOT/registry.json -flake1=$TEST_ROOT/flake1 -flake2=$TEST_ROOT/flake2 -flake3=$TEST_ROOT/flake3 +flake1Dir=$TEST_ROOT/flake1 +flake2Dir=$TEST_ROOT/flake2 +flake3Dir=$TEST_ROOT/flake3 -for repo in $flake1 $flake2 $flake3; do +for repo in $flake1Dir $flake2Dir $flake3Dir; do rm -rf $repo mkdir $repo git -C $repo init @@ -21,7 +21,7 @@ for repo in $flake1 $flake2 $flake3; do git -C $repo config user.name "Foobar" done -cat > $flake1/flake.nix < $flake1Dir/flake.nix < $flake1/flake.nix < $flake2/flake.nix < $flake2Dir/flake.nix < $flake2/flake.nix < $flake3/flake.nix < $registry < Date: Wed, 1 May 2019 11:38:48 +0200 Subject: [PATCH 2/3] Give errors in resolveFlake If DontUpdate but the lockfile isn't correct --- src/libexpr/primops/flake.cc | 70 ++++++++++++++++++++---------------- src/libexpr/primops/flake.hh | 8 ++--- src/nix/flake.cc | 3 +- src/nix/installables.cc | 2 +- tests/config.nix | 20 +++++++++++ tests/flakes.sh | 8 ++--- 6 files changed, 70 insertions(+), 41 deletions(-) create mode 100644 tests/config.nix diff --git a/src/libexpr/primops/flake.cc b/src/libexpr/primops/flake.cc index 88eadff55..c576a8b3e 100644 --- a/src/libexpr/primops/flake.cc +++ b/src/libexpr/primops/flake.cc @@ -50,8 +50,7 @@ LockFile::FlakeEntry readFlakeEntry(nlohmann::json json) if (!flakeRef.isImmutable()) throw Error("cannot use mutable flake '%s' in pure mode", flakeRef); - Hash hash = Hash((std::string) json["contentHash"]); - LockFile::FlakeEntry entry(flakeRef, hash); + LockFile::FlakeEntry entry(flakeRef, Hash((std::string) json["contentHash"])); auto nonFlakeRequires = json["nonFlakeRequires"]; @@ -59,9 +58,8 @@ LockFile::FlakeEntry readFlakeEntry(nlohmann::json json) FlakeRef flakeRef(i->value("uri", "")); if (!flakeRef.isImmutable()) throw Error("requested to fetch FlakeRef '%s' purely, which is mutable", flakeRef); - Hash hash = Hash((std::string) i->value("contentHash", "")); - LockFile::NonFlakeEntry newEntry(flakeRef, hash); - entry.nonFlakeEntries.insert_or_assign(i.key(), newEntry); + LockFile::NonFlakeEntry nonEntry(flakeRef, Hash(i->value("contentHash", ""))); + entry.nonFlakeEntries.insert_or_assign(i.key(), nonEntry); } auto requires = json["requires"]; @@ -89,10 +87,10 @@ LockFile readLockFile(const Path & path) for (auto i = nonFlakeRequires.begin(); i != nonFlakeRequires.end(); ++i) { FlakeRef flakeRef(i->value("uri", "")); - LockFile::NonFlakeEntry entry(flakeRef, Hash((std::string) json["contentHash"])); + LockFile::NonFlakeEntry nonEntry(flakeRef, Hash(i->value("contentHash", ""))); if (!flakeRef.isImmutable()) - throw Error("requested to fetch FlakeRef '%s' purely, which is mutable", flakeRef); - lockFile.nonFlakeEntries.insert_or_assign(i.key(), entry); + throw Error("found mutable FlakeRef '%s' in lockfile at path %s", flakeRef, path); + lockFile.nonFlakeEntries.insert_or_assign(i.key(), nonEntry); } auto requires = json["requires"]; @@ -374,19 +372,25 @@ LockFile entryToLockFile(const LockFile::FlakeEntry & entry) return lockFile; } -ResolvedFlake resolveFlakeFromLockFile(EvalState & state, const FlakeRef & flakeRef, RegistryAccess registryAccess, - LockFile lockFile, bool isTopFlake = false) +ResolvedFlake resolveFlakeFromLockFile(EvalState & state, const FlakeRef & flakeRef, + ShouldUpdateLockFile update, LockFile lockFile = {}) { - bool allowRegistries = registryAccess == AllowRegistry || (registryAccess == AllowRegistryAtTop && isTopFlake); - Flake flake = getFlake(state, flakeRef, allowRegistries); + Flake flake = getFlake(state, flakeRef, update != DontUpdate); ResolvedFlake deps(flake); for (auto & nonFlakeInfo : flake.nonFlakeRequires) { FlakeRef ref = nonFlakeInfo.second; auto i = lockFile.nonFlakeEntries.find(nonFlakeInfo.first); - if (i != lockFile.nonFlakeEntries.end()) ref = i->second.ref; - deps.nonFlakeDeps.push_back(getNonFlake(state, ref, nonFlakeInfo.first)); + if (i != lockFile.nonFlakeEntries.end()) { + NonFlake nonFlake = getNonFlake(state, i->second.ref, nonFlakeInfo.first); + if (nonFlake.hash != i->second.contentHash) + throw Error("the content hash of flakeref %s doesn't match", i->second.ref.to_string()); + deps.nonFlakeDeps.push_back(nonFlake); + } else { + if (update == DontUpdate) throw Error("the lockfile requires updating nonflake dependency %s in DontUpdate mode", nonFlakeInfo.first); + deps.nonFlakeDeps.push_back(getNonFlake(state, nonFlakeInfo.second, nonFlakeInfo.first)); + } } for (auto newFlakeRef : flake.requires) { @@ -394,10 +398,14 @@ ResolvedFlake resolveFlakeFromLockFile(EvalState & state, const FlakeRef & flake LockFile newLockFile; auto i = lockFile.flakeEntries.find(newFlakeRef); if (i != lockFile.flakeEntries.end()) { // Propagate lockFile downwards if possible - ref = i->second.ref; - newLockFile = entryToLockFile(i->second); + ResolvedFlake newResFlake = resolveFlakeFromLockFile(state, i->second.ref, update, entryToLockFile(i->second)); + if (newResFlake.flake.hash != i->second.contentHash) + throw Error("the content hash of flakeref %s doesn't match", i->second.ref.to_string()); + deps.flakeDeps.push_back(newResFlake); + } else { + if (update == DontUpdate) throw Error("the lockfile requires updating flake dependency %s in DontUpdate mode", newFlakeRef.to_string()); + deps.flakeDeps.push_back(resolveFlakeFromLockFile(state, newFlakeRef, update)); } - deps.flakeDeps.push_back(resolveFlakeFromLockFile(state, ref, registryAccess, newLockFile)); } return deps; @@ -406,17 +414,18 @@ ResolvedFlake resolveFlakeFromLockFile(EvalState & state, const FlakeRef & flake /* Given a flake reference, recursively fetch it and its dependencies. FIXME: this should return a graph of flakes. */ -ResolvedFlake resolveFlake(EvalState & state, const FlakeRef & topRef, RegistryAccess registryAccess, - bool recreateLockFile) +ResolvedFlake resolveFlake(EvalState & state, const FlakeRef & topRef, ShouldUpdateLockFile update) { - bool allowRegistries = registryAccess == AllowRegistry || registryAccess == AllowRegistryAtTop; - Flake flake = getFlake(state, topRef, allowRegistries); + if (!std::get_if(&topRef.data)) update = DontUpdate; + Flake flake = getFlake(state, topRef, update != DontUpdate); LockFile lockFile; - if (!recreateLockFile) // If recreateLockFile, start with an empty lockfile + if (update != RecreateLockFile) { + // If recreateLockFile, start with an empty lockfile lockFile = readLockFile(flake.storePath + "/flake.lock"); // FIXME: symlink attack + } - return resolveFlakeFromLockFile(state, topRef, registryAccess, lockFile, true); + return resolveFlakeFromLockFile(state, topRef, update, lockFile); } LockFile::FlakeEntry dependenciesToFlakeEntry(const ResolvedFlake & resolvedFlake) @@ -426,15 +435,17 @@ LockFile::FlakeEntry dependenciesToFlakeEntry(const ResolvedFlake & resolvedFlak for (auto & newResFlake : resolvedFlake.flakeDeps) entry.flakeEntries.insert_or_assign(newResFlake.flake.originalRef, dependenciesToFlakeEntry(newResFlake)); - for (auto & nonFlake : resolvedFlake.nonFlakeDeps) - entry.nonFlakeEntries.insert_or_assign(nonFlake.alias, LockFile::NonFlakeEntry(nonFlake.resolvedRef, nonFlake.hash)); + for (auto & nonFlake : resolvedFlake.nonFlakeDeps) { + LockFile::NonFlakeEntry nonEntry(nonFlake.resolvedRef, nonFlake.hash); + entry.nonFlakeEntries.insert_or_assign(nonFlake.alias, nonEntry); + } return entry; } static LockFile makeLockFile(EvalState & evalState, FlakeRef & flakeRef, bool recreateLockFile) { - ResolvedFlake resFlake = resolveFlake(evalState, flakeRef, AllowRegistry, recreateLockFile); + ResolvedFlake resFlake = resolveFlake(evalState, flakeRef, recreateLockFile ? RecreateLockFile : UpdateLockFile); return entryToLockFile(dependenciesToFlakeEntry(resFlake)); } @@ -501,17 +512,16 @@ void callFlake(EvalState & state, const ResolvedFlake & resFlake, Value & v) // Return the `provides` of the top flake, while assigning to `v` the provides // of the dependencies as well. -void makeFlakeValue(EvalState & state, const FlakeRef & flakeRef, RegistryAccess registryAccess, Value & v, bool recreateLockFile) +void makeFlakeValue(EvalState & state, const FlakeRef & flakeRef, ShouldUpdateLockFile update, Value & v) { - callFlake(state, resolveFlake(state, flakeRef, registryAccess, recreateLockFile), v); + callFlake(state, resolveFlake(state, flakeRef, update), v); } // This function is exposed to be used in nix files. static void prim_getFlake(EvalState & state, const Pos & pos, Value * * args, Value & v) { makeFlakeValue(state, state.forceStringNoCtx(*args[0], pos), - evalSettings.pureEval ? DisallowRegistry : AllowRegistryAtTop, v, false); - // `recreateLockFile == false` because this is the evaluation stage, which should be pure, and hence not recreate lockfiles. + evalSettings.pureEval ? DontUpdate : UpdateLockFile, v); } static RegisterPrimOp r2("getFlake", 1, prim_getFlake); diff --git a/src/libexpr/primops/flake.hh b/src/libexpr/primops/flake.hh index e3481e99e..132439b93 100644 --- a/src/libexpr/primops/flake.hh +++ b/src/libexpr/primops/flake.hh @@ -43,9 +43,9 @@ typedef std::vector> Registries; Path getUserRegistryPath(); -enum RegistryAccess { DisallowRegistry, AllowRegistry, AllowRegistryAtTop }; +enum ShouldUpdateLockFile { DontUpdate, UpdateLockFile, RecreateLockFile}; -void makeFlakeValue(EvalState & state, const FlakeRef & flakeRef, RegistryAccess registryAccess, Value & v, bool recreateLockFile); +void makeFlakeValue(EvalState &, const FlakeRef &, ShouldUpdateLockFile, Value &); std::shared_ptr readRegistry(const Path &); @@ -84,8 +84,8 @@ struct NonFlake FlakeRef originalRef; FlakeRef resolvedRef; std::optional revCount; + Hash hash; Path storePath; - Hash hash; // content hash // date NonFlake(const FlakeRef & origRef, const SourceInfo & sourceInfo) : originalRef(origRef), resolvedRef(sourceInfo.resolvedRef), revCount(sourceInfo.revCount), storePath(sourceInfo.storePath) {}; @@ -103,7 +103,7 @@ struct ResolvedFlake ResolvedFlake(const Flake & flake) : flake(flake) {} }; -ResolvedFlake resolveFlake(EvalState &, const FlakeRef &, RegistryAccess, bool recreateLockFile); +ResolvedFlake resolveFlake(EvalState &, const FlakeRef &, ShouldUpdateLockFile); void updateLockFile(EvalState &, const FlakeUri &, bool recreateLockFile); diff --git a/src/nix/flake.cc b/src/nix/flake.cc index d2cdf6fc9..fc0fc76b4 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -114,8 +114,7 @@ struct CmdFlakeDeps : FlakeCommand, MixJSON, StoreCommand, MixEvalArgs FlakeRef flakeRef(flakeUri); - bool recreateLockFile = false; - ResolvedFlake resFlake = resolveFlake(*evalState, flakeRef, AllowRegistryAtTop, recreateLockFile); + ResolvedFlake resFlake = resolveFlake(*evalState, flakeRef, UpdateLockFile); std::queue todo; todo.push(resFlake); diff --git a/src/nix/installables.cc b/src/nix/installables.cc index 6d784002a..25f3f4f9d 100644 --- a/src/nix/installables.cc +++ b/src/nix/installables.cc @@ -161,7 +161,7 @@ struct InstallableFlake : InstallableValue if (std::get_if(&flakeRef.data)) updateLockFile(state, flakeRef.to_string(), cmd.recreateLockFile); - makeFlakeValue(state, flakeRef, AllowRegistryAtTop, *vFlake, cmd.recreateLockFile); + makeFlakeValue(state, flakeRef, cmd.recreateLockFile ? RecreateLockFile : UpdateLockFile, *vFlake); auto vProvides = (*vFlake->attrs->get(state.symbols.create("provides")))->value; diff --git a/tests/config.nix b/tests/config.nix new file mode 100644 index 000000000..03810d57a --- /dev/null +++ b/tests/config.nix @@ -0,0 +1,20 @@ +with import ; + +rec { + inherit shell; + + path = coreutils; + + system = "x86_64-linux"; + + shared = builtins.getEnv "_NIX_TEST_SHARED"; + + mkDerivation = args: + derivation ({ + inherit system; + builder = shell; + args = ["-e" args.builder or (builtins.toFile "builder.sh" "if [ -e .attrs.sh ]; then source .attrs.sh; fi; eval \"$buildCommand\"")]; + PATH = path; + } // removeAttrs args ["builder" "meta"]) + // { meta = args.meta or {}; }; +} diff --git a/tests/flakes.sh b/tests/flakes.sh index 8b68aea65..d720eaf23 100644 --- a/tests/flakes.sh +++ b/tests/flakes.sh @@ -59,7 +59,7 @@ EOF git -C $flake2Dir add flake.nix git -C $flake2Dir commit -m 'Initial' -cat > $flake3/flake.nix < $flake3Dir/flake.nix < $flake3/flake.nix < $registry < Date: Tue, 14 May 2019 11:34:45 +0200 Subject: [PATCH 3/3] Lockfile handling in `resolveFlake` is fixed --- src/libexpr/primops/flake.cc | 136 +++++++++++++++++++++-------------- src/libexpr/primops/flake.hh | 17 +++-- src/nix/command.hh | 4 ++ src/nix/flake.cc | 4 +- src/nix/installables.cc | 18 ++++- tests/config.nix | 20 ------ tests/flakes.sh | 35 ++++++++- 7 files changed, 149 insertions(+), 85 deletions(-) delete mode 100644 tests/config.nix diff --git a/src/libexpr/primops/flake.cc b/src/libexpr/primops/flake.cc index c576a8b3e..3cbb0c1ef 100644 --- a/src/libexpr/primops/flake.cc +++ b/src/libexpr/primops/flake.cc @@ -372,10 +372,58 @@ LockFile entryToLockFile(const LockFile::FlakeEntry & entry) return lockFile; } -ResolvedFlake resolveFlakeFromLockFile(EvalState & state, const FlakeRef & flakeRef, - ShouldUpdateLockFile update, LockFile lockFile = {}) +LockFile::FlakeEntry dependenciesToFlakeEntry(const ResolvedFlake & resolvedFlake) { - Flake flake = getFlake(state, flakeRef, update != DontUpdate); + LockFile::FlakeEntry entry(resolvedFlake.flake.resolvedRef, resolvedFlake.flake.hash); + + for (auto & info : resolvedFlake.flakeDeps) + entry.flakeEntries.insert_or_assign(info.first.to_string(), dependenciesToFlakeEntry(info.second)); + + for (auto & nonFlake : resolvedFlake.nonFlakeDeps) { + LockFile::NonFlakeEntry nonEntry(nonFlake.resolvedRef, nonFlake.hash); + entry.nonFlakeEntries.insert_or_assign(nonFlake.alias, nonEntry); + } + + return entry; +} + +bool allowedToWrite (HandleLockFile handle) +{ + if (handle == AllPure) return false; + else if (handle == TopRefUsesRegistries) return false; + else if (handle == UpdateLockFile) return true; + else if (handle == UseUpdatedLockFile) return false; + else if (handle == RecreateLockFile) return true; + else if (handle == UseNewLockFile) return false; + else assert(false); +} + +bool recreateLockFile (HandleLockFile handle) +{ + if (handle == AllPure) return false; + else if (handle == TopRefUsesRegistries) return false; + else if (handle == UpdateLockFile) return false; + else if (handle == UseUpdatedLockFile) return false; + else if (handle == RecreateLockFile) return true; + else if (handle == UseNewLockFile) return true; + else assert(false); +} + +bool allowedToUseRegistries (HandleLockFile handle, bool isTopRef) +{ + if (handle == AllPure) return false; + else if (handle == TopRefUsesRegistries) return isTopRef; + else if (handle == UpdateLockFile) return true; + else if (handle == UseUpdatedLockFile) return true; + else if (handle == RecreateLockFile) return true; + else if (handle == UseNewLockFile) return true; + else assert(false); +} + +ResolvedFlake resolveFlakeFromLockFile(EvalState & state, const FlakeRef & flakeRef, + HandleLockFile handleLockFile, LockFile lockFile = {}, bool topRef = false) +{ + Flake flake = getFlake(state, flakeRef, allowedToUseRegistries(handleLockFile, topRef)); ResolvedFlake deps(flake); @@ -388,23 +436,23 @@ ResolvedFlake resolveFlakeFromLockFile(EvalState & state, const FlakeRef & flake throw Error("the content hash of flakeref %s doesn't match", i->second.ref.to_string()); deps.nonFlakeDeps.push_back(nonFlake); } else { - if (update == DontUpdate) throw Error("the lockfile requires updating nonflake dependency %s in DontUpdate mode", nonFlakeInfo.first); + if (handleLockFile == AllPure || handleLockFile == TopRefUsesRegistries) + throw Error("the lockfile requires updating nonflake dependency %s in AllPure mode", nonFlakeInfo.first); deps.nonFlakeDeps.push_back(getNonFlake(state, nonFlakeInfo.second, nonFlakeInfo.first)); } } for (auto newFlakeRef : flake.requires) { - FlakeRef ref = newFlakeRef; - LockFile newLockFile; auto i = lockFile.flakeEntries.find(newFlakeRef); if (i != lockFile.flakeEntries.end()) { // Propagate lockFile downwards if possible - ResolvedFlake newResFlake = resolveFlakeFromLockFile(state, i->second.ref, update, entryToLockFile(i->second)); + ResolvedFlake newResFlake = resolveFlakeFromLockFile(state, i->second.ref, handleLockFile, entryToLockFile(i->second)); if (newResFlake.flake.hash != i->second.contentHash) throw Error("the content hash of flakeref %s doesn't match", i->second.ref.to_string()); - deps.flakeDeps.push_back(newResFlake); + deps.flakeDeps.insert_or_assign(newFlakeRef, newResFlake); } else { - if (update == DontUpdate) throw Error("the lockfile requires updating flake dependency %s in DontUpdate mode", newFlakeRef.to_string()); - deps.flakeDeps.push_back(resolveFlakeFromLockFile(state, newFlakeRef, update)); + if (handleLockFile == AllPure || handleLockFile == TopRefUsesRegistries) + throw Error("the lockfile requires updating flake dependency %s in AllPure mode", newFlakeRef.to_string()); + deps.flakeDeps.insert_or_assign(newFlakeRef, resolveFlakeFromLockFile(state, newFlakeRef, handleLockFile)); } } @@ -414,54 +462,37 @@ ResolvedFlake resolveFlakeFromLockFile(EvalState & state, const FlakeRef & flake /* Given a flake reference, recursively fetch it and its dependencies. FIXME: this should return a graph of flakes. */ -ResolvedFlake resolveFlake(EvalState & state, const FlakeRef & topRef, ShouldUpdateLockFile update) +ResolvedFlake resolveFlake(EvalState & state, const FlakeRef & topRef, HandleLockFile handleLockFile) { - if (!std::get_if(&topRef.data)) update = DontUpdate; - Flake flake = getFlake(state, topRef, update != DontUpdate); + Flake flake = getFlake(state, topRef, allowedToUseRegistries(handleLockFile, true)); LockFile lockFile; - if (update != RecreateLockFile) { + if (!recreateLockFile (handleLockFile)) { // If recreateLockFile, start with an empty lockfile lockFile = readLockFile(flake.storePath + "/flake.lock"); // FIXME: symlink attack } - return resolveFlakeFromLockFile(state, topRef, update, lockFile); + ResolvedFlake resFlake = resolveFlakeFromLockFile(state, topRef, handleLockFile, lockFile, true); + lockFile = entryToLockFile(dependenciesToFlakeEntry(resFlake)); + + if (allowedToWrite(handleLockFile)) { + if (auto refData = std::get_if(&topRef.data)) { + writeLockFile(lockFile, refData->path + (topRef.subdir == "" ? "" : "/" + topRef.subdir) + "/flake.lock"); + + // Hack: Make sure that flake.lock is visible to Git, so it ends up in the Nix store. + runProgram("git", true, { "-C", refData->path, "add", + (topRef.subdir == "" ? "" : topRef.subdir + "/") + "flake.lock" }); + } else std::cout << "Cannot write lockfile because the FlakeRef isn't of the form IsPath." << std::endl; + } else if (handleLockFile != AllPure && handleLockFile != TopRefUsesRegistries) + std::cout << "Using updating lockfile without writing it to file" << std::endl; + + return resFlake; } -LockFile::FlakeEntry dependenciesToFlakeEntry(const ResolvedFlake & resolvedFlake) +void updateLockFile (EvalState & state, const FlakeUri & flakeUri, bool recreateLockFile) { - LockFile::FlakeEntry entry(resolvedFlake.flake.resolvedRef, resolvedFlake.flake.hash); - - for (auto & newResFlake : resolvedFlake.flakeDeps) - entry.flakeEntries.insert_or_assign(newResFlake.flake.originalRef, dependenciesToFlakeEntry(newResFlake)); - - for (auto & nonFlake : resolvedFlake.nonFlakeDeps) { - LockFile::NonFlakeEntry nonEntry(nonFlake.resolvedRef, nonFlake.hash); - entry.nonFlakeEntries.insert_or_assign(nonFlake.alias, nonEntry); - } - - return entry; -} - -static LockFile makeLockFile(EvalState & evalState, FlakeRef & flakeRef, bool recreateLockFile) -{ - ResolvedFlake resFlake = resolveFlake(evalState, flakeRef, recreateLockFile ? RecreateLockFile : UpdateLockFile); - return entryToLockFile(dependenciesToFlakeEntry(resFlake)); -} - -void updateLockFile(EvalState & state, const FlakeUri & uri, bool recreateLockFile) -{ - FlakeRef flakeRef = FlakeRef(uri); - auto lockFile = makeLockFile(state, flakeRef, recreateLockFile); - if (auto refData = std::get_if(&flakeRef.data)) { - writeLockFile(lockFile, refData->path + (flakeRef.subdir == "" ? "" : "/" + flakeRef.subdir) + "/flake.lock"); - - // Hack: Make sure that flake.lock is visible to Git. Otherwise, - // exportGit will fail to copy it to the Nix store. - runProgram("git", true, { "-C", refData->path, "add", - (flakeRef.subdir == "" ? "" : flakeRef.subdir + "/") + "flake.lock" }); - } else - throw Error("flakeUri %s can't be updated because it is not a path", uri); + FlakeRef flakeRef(flakeUri); + resolveFlake(state, flakeRef, recreateLockFile ? RecreateLockFile : UpdateLockFile); } void callFlake(EvalState & state, const ResolvedFlake & resFlake, Value & v) @@ -471,7 +502,8 @@ void callFlake(EvalState & state, const ResolvedFlake & resFlake, Value & v) state.mkAttrs(v, resFlake.flakeDeps.size() + resFlake.nonFlakeDeps.size() + 8); - for (const ResolvedFlake newResFlake : resFlake.flakeDeps) { + for (auto info : resFlake.flakeDeps) { + const ResolvedFlake newResFlake = info.second; auto vFlake = state.allocAttr(v, newResFlake.flake.id); callFlake(state, newResFlake, *vFlake); } @@ -512,16 +544,16 @@ void callFlake(EvalState & state, const ResolvedFlake & resFlake, Value & v) // Return the `provides` of the top flake, while assigning to `v` the provides // of the dependencies as well. -void makeFlakeValue(EvalState & state, const FlakeRef & flakeRef, ShouldUpdateLockFile update, Value & v) +void makeFlakeValue(EvalState & state, const FlakeRef & flakeRef, HandleLockFile handle, Value & v) { - callFlake(state, resolveFlake(state, flakeRef, update), v); + callFlake(state, resolveFlake(state, flakeRef, handle), v); } // This function is exposed to be used in nix files. static void prim_getFlake(EvalState & state, const Pos & pos, Value * * args, Value & v) { makeFlakeValue(state, state.forceStringNoCtx(*args[0], pos), - evalSettings.pureEval ? DontUpdate : UpdateLockFile, v); + evalSettings.pureEval ? AllPure : UseUpdatedLockFile, v); } static RegisterPrimOp r2("getFlake", 1, prim_getFlake); diff --git a/src/libexpr/primops/flake.hh b/src/libexpr/primops/flake.hh index 132439b93..6f91686a6 100644 --- a/src/libexpr/primops/flake.hh +++ b/src/libexpr/primops/flake.hh @@ -36,16 +36,23 @@ struct LockFile }; std::map flakeEntries; - std::map nonFlakeEntries; + std::map nonFlakeEntries; }; typedef std::vector> Registries; Path getUserRegistryPath(); -enum ShouldUpdateLockFile { DontUpdate, UpdateLockFile, RecreateLockFile}; +enum HandleLockFile + { AllPure // Everything is handled 100% purely + , TopRefUsesRegistries // The top FlakeRef uses the registries, apart from that, everything happens 100% purely + , UpdateLockFile // Update the existing lockfile and write it to file + , UseUpdatedLockFile // `UpdateLockFile` without writing to file + , RecreateLockFile // Recreate the lockfile from scratch and write it to file + , UseNewLockFile // `RecreateLockFile` without writing to file + }; -void makeFlakeValue(EvalState &, const FlakeRef &, ShouldUpdateLockFile, Value &); +void makeFlakeValue(EvalState &, const FlakeRef &, HandleLockFile, Value &); std::shared_ptr readRegistry(const Path &); @@ -98,12 +105,12 @@ Flake getFlake(EvalState &, const FlakeRef &, bool impureIsAllowed); struct ResolvedFlake { Flake flake; - std::vector flakeDeps; // The flake dependencies + std::map flakeDeps; // The key in this map, is the originalRef as written in flake.nix std::vector nonFlakeDeps; ResolvedFlake(const Flake & flake) : flake(flake) {} }; -ResolvedFlake resolveFlake(EvalState &, const FlakeRef &, ShouldUpdateLockFile); +ResolvedFlake resolveFlake(EvalState &, const FlakeRef &, HandleLockFile); void updateLockFile(EvalState &, const FlakeUri &, bool recreateLockFile); diff --git a/src/nix/command.hh b/src/nix/command.hh index 32a5047a8..30d869b19 100644 --- a/src/nix/command.hh +++ b/src/nix/command.hh @@ -80,6 +80,10 @@ struct SourceExprCommand : virtual Args, StoreCommand, MixEvalArgs bool recreateLockFile = false; + bool saveLockFile = true; + + bool noRegistries = false; + ref getEvalState(); std::vector> parseInstallables( diff --git a/src/nix/flake.cc b/src/nix/flake.cc index fc0fc76b4..bfb3178ad 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -126,8 +126,8 @@ struct CmdFlakeDeps : FlakeCommand, MixJSON, StoreCommand, MixEvalArgs for (NonFlake & nonFlake : resFlake.nonFlakeDeps) printNonFlakeInfo(nonFlake, json); - for (ResolvedFlake & newResFlake : resFlake.flakeDeps) - todo.push(newResFlake); + for (auto info : resFlake.flakeDeps) + todo.push(info.second); } } }; diff --git a/src/nix/installables.cc b/src/nix/installables.cc index 25f3f4f9d..a2a55d949 100644 --- a/src/nix/installables.cc +++ b/src/nix/installables.cc @@ -26,6 +26,16 @@ SourceExprCommand::SourceExprCommand() .longName("recreate-lock-file") .description("recreate lock file from scratch") .set(&recreateLockFile, true); + + mkFlag() + .longName("dont-save-lock-file") + .description("save the newly generated lock file") + .set(&saveLockFile, false); + + mkFlag() + .longName("no-registries") + .description("don't use flake registries") + .set(&noRegistries, true); } ref SourceExprCommand::getEvalState() @@ -158,10 +168,12 @@ struct InstallableFlake : InstallableValue Value * toValue(EvalState & state) override { auto vFlake = state.allocValue(); - if (std::get_if(&flakeRef.data)) - updateLockFile(state, flakeRef.to_string(), cmd.recreateLockFile); - makeFlakeValue(state, flakeRef, cmd.recreateLockFile ? RecreateLockFile : UpdateLockFile, *vFlake); + HandleLockFile handle = cmd.noRegistries ? AllPure : + cmd.recreateLockFile ? + (cmd.saveLockFile ? RecreateLockFile : UseNewLockFile) + : (cmd.saveLockFile ? UpdateLockFile : UseUpdatedLockFile); + makeFlakeValue(state, flakeRef, handle, *vFlake); auto vProvides = (*vFlake->attrs->get(state.symbols.create("provides")))->value; diff --git a/tests/config.nix b/tests/config.nix deleted file mode 100644 index 03810d57a..000000000 --- a/tests/config.nix +++ /dev/null @@ -1,20 +0,0 @@ -with import ; - -rec { - inherit shell; - - path = coreutils; - - system = "x86_64-linux"; - - shared = builtins.getEnv "_NIX_TEST_SHARED"; - - mkDerivation = args: - derivation ({ - inherit system; - builder = shell; - args = ["-e" args.builder or (builtins.toFile "builder.sh" "if [ -e .attrs.sh ]; then source .attrs.sh; fi; eval \"$buildCommand\"")]; - PATH = path; - } // removeAttrs args ["builder" "meta"]) - // { meta = args.meta or {}; }; -} diff --git a/tests/flakes.sh b/tests/flakes.sh index d720eaf23..45c5f2048 100644 --- a/tests/flakes.sh +++ b/tests/flakes.sh @@ -118,7 +118,7 @@ nix build -o $TEST_ROOT/result --flake-registry $registry flake1: [[ -e $TEST_ROOT/result/hello ]] # Building a flake with an unlocked dependency should fail in pure mode. -(! nix build -o $TEST_ROOT/result --flake-registry $registry flake2:bar) +(! nix eval "(builtins.getFlake "$flake2Dir")") # But should succeed in impure mode. nix build -o $TEST_ROOT/result --flake-registry $registry flake2:bar --impure @@ -129,8 +129,8 @@ nix build -o $TEST_ROOT/result --flake-registry $registry $flake2Dir:bar git -C $flake2Dir commit flake.lock -m 'Add flake.lock' # Rerunning the build should not change the lockfile. -#nix build -o $TEST_ROOT/result --flake-registry $registry $flake2:bar -#[[ -z $(git -C $flake2 diff) ]] +nix build -o $TEST_ROOT/result --flake-registry $registry $flake2Dir:bar +[[ -z $(git -C $flake2Dir diff master) ]] # Now we should be able to build the flake in pure mode. nix build -o $TEST_ROOT/result --flake-registry $registry flake2:bar @@ -140,3 +140,32 @@ nix build -o $TEST_ROOT/result file://$flake2Dir:bar # Test whether indirect dependencies work. nix build -o $TEST_ROOT/result --flake-registry $registry $flake3Dir:xyzzy + +# Add dependency to flake3 +rm $flake3Dir/flake.nix + +cat > $flake3Dir/flake.nix <