Merge pull request #6328 from edolstra/fix-nix-profile-install

nix profile install: Don't use queryDerivationOutputMap()

src/libcmd/installables.cc

@@ -756,15 +756,50 @@ std::shared_ptr<Installable> SourceExprCommand::parseInstallable(
     return installables.front();
 }
 
-BuiltPaths getBuiltPaths(ref<Store> evalStore, ref<Store> store, const DerivedPaths & hopefullyBuiltPaths)
+BuiltPaths Installable::build(
+    ref<Store> evalStore,
+    ref<Store> store,
+    Realise mode,
+    const std::vector<std::shared_ptr<Installable>> & installables,
+    BuildMode bMode)
 {
     BuiltPaths res;
-    for (const auto & b : hopefullyBuiltPaths)
+    for (auto & [_, builtPath] : build2(evalStore, store, mode, installables, bMode))
-        std::visit(
+        res.push_back(builtPath);
-            overloaded{
+    return res;
-                [&](const DerivedPath::Opaque & bo) {
+}
-                    res.push_back(BuiltPath::Opaque{bo.path});
+
-                },
+std::vector<std::pair<std::shared_ptr<Installable>, BuiltPath>> Installable::build2(
+    ref<Store> evalStore,
+    ref<Store> store,
+    Realise mode,
+    const std::vector<std::shared_ptr<Installable>> & installables,
+    BuildMode bMode)
+{
+    if (mode == Realise::Nothing)
+        settings.readOnlyMode = true;
+
+    std::vector<DerivedPath> pathsToBuild;
+    std::map<DerivedPath, std::vector<std::shared_ptr<Installable>>> backmap;
+
+    for (auto & i : installables) {
+        for (auto b : i->toDerivedPaths()) {
+            pathsToBuild.push_back(b);
+            backmap[b].push_back(i);
+        }
+    }
+
+    std::vector<std::pair<std::shared_ptr<Installable>, BuiltPath>> res;
+
+    switch (mode) {
+
+    case Realise::Nothing:
+    case Realise::Derivation:
+        printMissing(store, pathsToBuild, lvlError);
+
+        for (auto & path : pathsToBuild) {
+            for (auto & installable : backmap[path]) {
+                std::visit(overloaded {
                     [&](const DerivedPath::Built & bfd) {
                         OutputPathMap outputs;
                         auto drv = evalStore->readDerivation(bfd.drvPath);
@@ -776,17 +811,14 @@ BuiltPaths getBuiltPaths(ref<Store> evalStore, ref<Store> store, const DerivedPa
                                     "the derivation '%s' doesn't have an output named '%s'",
                                     store->printStorePath(bfd.drvPath), output);
                             if (settings.isExperimentalFeatureEnabled(Xp::CaDerivations)) {
-                            auto outputId =
+                                DrvOutput outputId { outputHashes.at(output), output };
-                                DrvOutput{outputHashes.at(output), output};
+                                auto realisation = store->queryRealisation(outputId);
-                            auto realisation =
-                                store->queryRealisation(outputId);
                                 if (!realisation)
                                     throw Error(
                                         "cannot operate on an output of unbuilt "
                                         "content-addressed derivation '%s'",
                                         outputId.to_string());
-                            outputs.insert_or_assign(
+                                outputs.insert_or_assign(output, realisation->outPath);
-                                output, realisation->outPath);
                             } else {
                                 // If ca-derivations isn't enabled, assume that
                                 // the output path is statically known.
@@ -796,58 +828,45 @@ BuiltPaths getBuiltPaths(ref<Store> evalStore, ref<Store> store, const DerivedPa
                                     output, *drvOutputs.at(output).second);
                             }
                         }
-                    res.push_back(BuiltPath::Built{bfd.drvPath, outputs});
+                        res.push_back({installable, BuiltPath::Built { bfd.drvPath, outputs }});
                     },
+                    [&](const DerivedPath::Opaque & bo) {
+                        res.push_back({installable, BuiltPath::Opaque { bo.path }});
                     },
-            b.raw());
+                }, path.raw());
-
+            }
-    return res;
         }
 
-BuiltPaths Installable::build(
+        break;
-    ref<Store> evalStore,
-    ref<Store> store,
-    Realise mode,
-    const std::vector<std::shared_ptr<Installable>> & installables,
-    BuildMode bMode)
-{
-    if (mode == Realise::Nothing)
-        settings.readOnlyMode = true;
 
-    std::vector<DerivedPath> pathsToBuild;
-
-    for (auto & i : installables) {
-        auto b = i->toDerivedPaths();
-        pathsToBuild.insert(pathsToBuild.end(), b.begin(), b.end());
-    }
-
-    switch (mode) {
-    case Realise::Nothing:
-    case Realise::Derivation:
-        printMissing(store, pathsToBuild, lvlError);
-        return getBuiltPaths(evalStore, store, pathsToBuild);
     case Realise::Outputs: {
-        BuiltPaths res;
         for (auto & buildResult : store->buildPathsWithResults(pathsToBuild, bMode, evalStore)) {
             if (!buildResult.success())
                 buildResult.rethrow();
+
+            for (auto & installable : backmap[buildResult.path]) {
                 std::visit(overloaded {
                     [&](const DerivedPath::Built & bfd) {
                         std::map<std::string, StorePath> outputs;
                         for (auto & path : buildResult.builtOutputs)
                             outputs.emplace(path.first.outputName, path.second.outPath);
-                    res.push_back(BuiltPath::Built { bfd.drvPath, outputs });
+                        res.push_back({installable, BuiltPath::Built { bfd.drvPath, outputs }});
                     },
                     [&](const DerivedPath::Opaque & bo) {
-                    res.push_back(BuiltPath::Opaque { bo.path });
+                        res.push_back({installable, BuiltPath::Opaque { bo.path }});
                     },
                 }, buildResult.path.raw());
             }
-        return res;
         }
+
+        break;
+    }
+
     default:
         assert(false);
     }
+
+    return res;
 }
 
 BuiltPaths Installable::toBuiltPaths(

src/libcmd/installables.hh

@@ -98,6 +98,13 @@ struct Installable
         const std::vector<std::shared_ptr<Installable>> & installables,
         BuildMode bMode = bmNormal);
 
+    static std::vector<std::pair<std::shared_ptr<Installable>, BuiltPath>> build2(
+        ref<Store> evalStore,
+        ref<Store> store,
+        Realise mode,
+        const std::vector<std::shared_ptr<Installable>> & installables,
+        BuildMode bMode = bmNormal);
+
     static std::set<StorePath> toStorePaths(
         ref<Store> evalStore,
         ref<Store> store,
@@ -185,9 +192,4 @@ ref<eval_cache::EvalCache> openEvalCache(
     EvalState & state,
     std::shared_ptr<flake::LockedFlake> lockedFlake);
 
-BuiltPaths getBuiltPaths(
-    ref<Store> evalStore,
-    ref<Store> store,
-    const DerivedPaths & hopefullyBuiltPaths);
-
 }

src/libstore/derived-path.hh

@@ -25,6 +25,9 @@ struct DerivedPathOpaque {
     nlohmann::json toJSON(ref<Store> store) const;
     std::string to_string(const Store & store) const;
     static DerivedPathOpaque parse(const Store & store, std::string_view);
+
+    bool operator < (const DerivedPathOpaque & b) const
+    { return path < b.path; }
 };
 
 /**
@@ -46,6 +49,9 @@ struct DerivedPathBuilt {
     std::string to_string(const Store & store) const;
     static DerivedPathBuilt parse(const Store & store, std::string_view);
     nlohmann::json toJSON(ref<Store> store) const;
+
+    bool operator < (const DerivedPathBuilt & b) const
+    { return std::make_pair(drvPath, outputs) < std::make_pair(b.drvPath, b.outputs); }
 };
 
 using _DerivedPathRaw = std::variant<

src/nix/profile.cc

@@ -62,22 +62,21 @@ struct ProfileElement
         return std::tuple(describe(), storePaths) < std::tuple(other.describe(), other.storePaths);
     }
 
-    void updateStorePaths(ref<Store> evalStore, ref<Store> store, Installable & installable)
+    void updateStorePaths(
+        ref<Store> evalStore,
+        ref<Store> store,
+        const BuiltPaths & builtPaths)
     {
         // FIXME: respect meta.outputsToInstall
         storePaths.clear();
-        for (auto & buildable : getBuiltPaths(evalStore, store, installable.toDerivedPaths())) {
+        for (auto & buildable : builtPaths) {
             std::visit(overloaded {
                 [&](const BuiltPath::Opaque & bo) {
                     storePaths.insert(bo.path);
                 },
                 [&](const BuiltPath::Built & bfd) {
-                    // TODO: Why are we querying if we know the output
+                    for (auto & output : bfd.outputs)
-                    // names already? Is it just to figure out what the
-                    // default one is?
-                    for (auto & output : store->queryDerivationOutputMap(bfd.drvPath)) {
                         storePaths.insert(output.second);
-                    }
                 },
             }, buildable.raw());
         }
@@ -236,6 +235,16 @@ struct ProfileManifest
     }
 };
 
+static std::map<Installable *, BuiltPaths>
+builtPathsPerInstallable(
+    const std::vector<std::pair<std::shared_ptr<Installable>, BuiltPath>> & builtPaths)
+{
+    std::map<Installable *, BuiltPaths> res;
+    for (auto & [installable, builtPath] : builtPaths)
+        res[installable.get()].push_back(builtPath);
+    return res;
+}
+
 struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile
 {
     std::string description() override
@@ -254,7 +263,9 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile
     {
         ProfileManifest manifest(*getEvalState(), *profile);
 
-        auto builtPaths = Installable::build(getEvalStore(), store, Realise::Outputs, installables, bmNormal);
+        auto builtPaths = builtPathsPerInstallable(
+            Installable::build2(
+                getEvalStore(), store, Realise::Outputs, installables, bmNormal));
 
         for (auto & installable : installables) {
             ProfileElement element;
@@ -269,7 +280,7 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile
                 };
             }
 
-            element.updateStorePaths(getEvalStore(), store, *installable);
+            element.updateStorePaths(getEvalStore(), store, builtPaths[installable.get()]);
 
             manifest.elements.push_back(std::move(element));
         }
@@ -457,12 +468,14 @@ struct CmdProfileUpgrade : virtual SourceExprCommand, MixDefaultProfile, MixProf
             warn ("Use 'nix profile list' to see the current profile.");
         }
 
-        auto builtPaths = Installable::build(getEvalStore(), store, Realise::Outputs, installables, bmNormal);
+        auto builtPaths = builtPathsPerInstallable(
+            Installable::build2(
+                getEvalStore(), store, Realise::Outputs, installables, bmNormal));
 
         for (size_t i = 0; i < installables.size(); ++i) {
             auto & installable = installables.at(i);
             auto & element = manifest.elements[indices.at(i)];
-            element.updateStorePaths(getEvalStore(), store, *installable);
+            element.updateStorePaths(getEvalStore(), store, builtPaths[installable.get()]);
         }
 
         updateProfile(manifest.build(store));