Merge pull request #10024 from edolstra/remove-locked-flag

Input: Replace 'locked' bool by isLocked() method

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
+            if overrides ? ${key}
-            then rootSrc
+            then
-            else fetchTree (node.info or {} // removeAttrs node.locked ["dir"]);
+              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 =

src/libexpr/flake/flake.cc

@@ -365,6 +365,7 @@ LockedFlake lockFlake(
         std::map<InputPath, FlakeInput> overrides;
         std::set<InputPath> explicitCliOverrides;
         std::set<InputPath> overridesUsed, updatesUsed;
+        std::map<ref<Node>, StorePath> nodePaths;
 
         for (auto & i : lockFlags.inputOverrides) {
             overrides.insert_or_assign(i.first, FlakeInput { .ref = i.second });
@@ -535,11 +536,13 @@ LockedFlake lockFlake(
                             }
                         }
 
-                        computeLocks(
+                        if (mustRefetch) {
-                            mustRefetch
+                            auto inputFlake = getFlake(state, oldLock->lockedRef, false, flakeCache, inputPath);
-                            ? getFlake(state, oldLock->lockedRef, false, flakeCache, inputPath).inputs
+                            nodePaths.emplace(childNode, inputFlake.storePath);
-                            : fakeInputs,
+                            computeLocks(inputFlake.inputs, childNode, inputPath, oldLock, lockRootPath, parentPath, false);
-                            childNode, inputPath, oldLock, lockRootPath, parentPath, !mustRefetch);
+                        } 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<LockedNode>(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();
+    experimentalFeatureSettings.require(Xp::Flakes);
-    auto vRootSrc = state.allocValue();
-    auto vRootSubdir = state.allocValue();
-    auto vTmp1 = state.allocValue();
-    auto vTmp2 = state.allocValue();
 
-    vLocks->mkString(lockedFlake.lockFile.to_string());
+    auto [lockFileStr, keyMap] = lockedFlake.lockFile.to_string();
 
-    emitTreeAttrs(
+    auto overrides = state.buildBindings(lockedFlake.nodePaths.size());
-        state,
-        lockedFlake.flake.storePath,
-        lockedFlake.flake.lockedRef.input,
-        *vRootSrc,
-        false,
-        lockedFlake.flake.forceDirty);
 
-    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<const LockedNode>();
+
+        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)

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<ref<Node>, StorePath> nodePaths;
+
     Fingerprint getFingerprint() const;
 };
 

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<nlohmann::json, LockFile::KeyMap> LockFile::toJSON() const
 {
     nlohmann::json nodes;
-    std::unordered_map<std::shared_ptr<const Node>, std::string> nodeKeys;
+    KeyMap nodeKeys;
     std::unordered_set<std::string> keys;
 
     std::function<std::string(const std::string & key, ref<const Node> 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<std::string, LockFile::KeyMap> 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<FlakeRef> 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

src/libexpr/flake/lockfile.hh

@@ -59,14 +59,15 @@ struct LockFile
 
     typedef std::map<ref<const Node>, std::string> KeyMap;
 
-    nlohmann::json toJSON() const;
+    std::pair<nlohmann::json, KeyMap> toJSON() const;
 
-    std::string to_string() const;
+    std::pair<std::string, KeyMap> 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<FlakeRef> isUnlocked() const;
 

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<EvalError>(
-            "in pure evaluation mode, %s requires a locked input",
+            "in pure evaluation mode, '%s' will not fetch unlocked input '%s'",
-            fetcher
+            fetcher, input.to_string()
         ).atPos(pos).debugThrow();
     }
 

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<StorePath, Input> Input::fetch(ref<Store> store) const
                 input.to_string(), *prevRevCount);
     }
 
-    input.locked = true;
-
     return {std::move(storePath), input};
 }
 

src/libfetchers/fetchers.hh

@@ -29,7 +29,6 @@ struct Input
 
     std::shared_ptr<InputScheme> 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<std::string> getFingerprint(ref<Store> 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<std::string> getFingerprint(ref<Store> 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<InputScheme> && fetcher);

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<GitInputScheme>()); });

src/libfetchers/github.cc

@@ -280,6 +280,15 @@ struct GitArchiveInputScheme : InputScheme
         return {accessor, input};
     }
 
+    bool isLocked(const Input & input) const override
+    {
+        /* 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> experimentalFeature() const override
     {
         return Xp::Flakes;

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<std::string> getFingerprint(ref<Store> store, const Input & input) const override
     {
         if (auto rev = input.getRev())

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");

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

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);

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(

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")