nix3-profile: make element names stable

Based off of commit 6268a45 Upstream-PR: NixOS/nix#9656 Co-authored-by: Eelco Dolstra <[email protected]> Change-Id: I0fcf069a8537c61ad6fc4eee1f3c193a708ea1c4
lix-project · May 2, 2024 · 61ead41 · 61ead41
1 parent 5cec872
commit 61ead41
Show file tree

Hide file tree

Showing 6 changed files with 103 additions and 82 deletions.
diff --git a/doc/manual/rl-next/nix-profile-names.md b/doc/manual/rl-next/nix-profile-names.md
@@ -1,7 +1,9 @@
 ---
 synopsis: "`nix profile` now allows referring to elements by human-readable name, and no longer accepts indices"
 prs: 8678
-cls: 978
+cls: [978, 980]
 ---
 
-[`nix profile`](@docroot@/command-ref/new-cli/nix3-profile.md) now uses names to refer to installed packages when running [`list`](@docroot@/command-ref/new-cli/nix3-profile-list.md), [`remove`](@docroot@/command-ref/new-cli/nix3-profile-remove.md) or [`upgrade`](@docroot@/command-ref/new-cli/nix3-profile-upgrade.md) as opposed to indices. Indices have been removed.
+[`nix profile`](@docroot@/command-ref/new-cli/nix3-profile.md) now uses names to refer to installed packages when running [`list`](@docroot@/command-ref/new-cli/nix3-profile-list.md), [`remove`](@docroot@/command-ref/new-cli/nix3-profile-remove.md) or [`upgrade`](@docroot@/command-ref/new-cli/nix3-profile-upgrade.md) as opposed to indices. Indices have been removed. Profile element names are generated when a package is installed and remain the same until the package is removed.
+
+**Warning**: The `manifest.nix` file used to record the contents of profiles has changed. Nix will automatically upgrade profiles to the new version when you modify the profile. After that, the profile can no longer be used by older versions of Nix.
diff --git a/src/libcmd/cmd-profiles.cc b/src/libcmd/cmd-profiles.cc
@@ -6,6 +6,7 @@
 #include "logging.hh"
 #include "names.hh"
 #include "store-api.hh"
+#include "url-name.hh"
 
 namespace nix
 {
@@ -109,8 +110,6 @@ ProfileManifest::ProfileManifest(EvalState & state, const Path & profile)
 
     if (pathExists(manifestPath)) {
         auto json = nlohmann::json::parse(readFile(manifestPath));
-        // Keep track of already found names so we can prevent duplicates.
-        std::set<std::string> foundNames;
 
         auto version = json.value("version", 0);
         std::string sUrl;
@@ -121,14 +120,19 @@ ProfileManifest::ProfileManifest(EvalState & state, const Path & profile)
             sOriginalUrl = "originalUri";
             break;
         case 2:
+            [[fallthrough]];
+        case 3:
             sUrl = "url";
             sOriginalUrl = "originalUrl";
             break;
         default:
             throw Error("profile manifest '%s' has unsupported version %d", manifestPath, version);
         }
 
-        for (auto & e : json["elements"]) {
+        auto elems = json["elements"];
+
+        for (auto & elem : elems.items()) {
+            auto & e = elem.value();
             ProfileElement element;
             for (auto & p : e["storePaths"]) {
                 element.storePaths.insert(state.store->parseStorePath((std::string) p));
@@ -145,26 +149,17 @@ ProfileManifest::ProfileManifest(EvalState & state, const Path & profile)
                     e["outputs"].get<ExtendedOutputsSpec>()};
             }
 
-            std::string nameCandidate(element.identifier());
-
-            if (e.contains("name")) {
-                nameCandidate = e["name"];
-            } else if (element.source) {
-                auto const url = parseURL(element.source->to_string());
-                auto const name = getNameFromURL(url);
-                if (name) {
-                    nameCandidate = *name;
-                }
-            }
-
-            auto finalName = nameCandidate;
-            for (unsigned appendedIndex = 1; foundNames.contains(finalName); ++appendedIndex) {
-                finalName = nameCandidate + std::to_string(appendedIndex);
-            }
-            element.name = finalName;
-            foundNames.insert(element.name);
-
-            elements.emplace_back(std::move(element));
+            // TODO(Qyriad): holy crap this chain of ternaries needs cleanup.
+            std::string name =
+                elems.is_object()
+                ? elem.key()
+                : e.contains("name")
+                ? static_cast<std::string>(e["name"])
+                : element.source
+                ? getNameFromURL(parseURL(element.source->to_string())).value_or(element.identifier())
+                : element.identifier();
+
+            addElement(name, std::move(element));
         }
     } else if (pathExists(profile + "/manifest.nix")) {
         // FIXME: needed because of pure mode; ugly.
@@ -176,16 +171,37 @@ ProfileManifest::ProfileManifest(EvalState & state, const Path & profile)
         for (auto & drvInfo : drvInfos) {
             ProfileElement element;
             element.storePaths = {drvInfo.queryOutPath()};
-            element.name = element.identifier();
-            elements.emplace_back(std::move(element));
+            addElement(std::move(element));
         }
     }
 }
 
+void ProfileManifest::addElement(std::string_view nameCandidate, ProfileElement element)
+{
+    std::string finalName(nameCandidate);
+
+    for (unsigned i = 1; elements.contains(finalName); ++i) {
+        finalName = nameCandidate + "-" + std::to_string(i);
+    }
+
+    elements.insert_or_assign(finalName, std::move(element));
+}
+
+void ProfileManifest::addElement(ProfileElement element)
+{
+    auto name =
+        element.source
+        ? getNameFromURL(parseURL(element.source->to_string()))
+        : std::nullopt;
+
+    auto finalName = name.value_or(element.identifier());
+    addElement(finalName, std::move(element));
+}
+
 nlohmann::json ProfileManifest::toJSON(Store & store) const
 {
-    auto array = nlohmann::json::array();
-    for (auto & element : elements) {
+    auto es = nlohmann::json::object();
+    for (auto & [name, element] : elements) {
         auto paths = nlohmann::json::array();
         for (auto & path : element.storePaths) {
             paths.push_back(store.printStorePath(path));
@@ -200,11 +216,11 @@ nlohmann::json ProfileManifest::toJSON(Store & store) const
             obj["attrPath"] = element.source->attrPath;
             obj["outputs"] = element.source->outputs;
         }
-        array.push_back(obj);
+        es[name] = obj;
     }
     nlohmann::json json;
-    json["version"] = 2;
-    json["elements"] = array;
+    json["version"] = 3;
+    json["elements"] = es;
     return json;
 }
 
@@ -215,7 +231,7 @@ StorePath ProfileManifest::build(ref<Store> store)
     StorePathSet references;
 
     Packages pkgs;
-    for (auto & element : elements) {
+    for (auto & [name, element] : elements) {
         for (auto & path : element.storePaths) {
             if (element.active) {
                 pkgs.emplace_back(store->printStorePath(path), true, element.priority);
@@ -261,35 +277,29 @@ void ProfileManifest::printDiff(
     const ProfileManifest & prev, const ProfileManifest & cur, std::string_view indent
 )
 {
-    auto prevElems = prev.elements;
-    std::sort(prevElems.begin(), prevElems.end());
-
-    auto curElems = cur.elements;
-    std::sort(curElems.begin(), curElems.end());
-
-    auto i = prevElems.begin();
-    auto j = curElems.begin();
+    auto prevElemIt = prev.elements.begin();
+    auto curElemIt = cur.elements.begin();
 
     bool changes = false;
 
-    while (i != prevElems.end() || j != curElems.end()) {
-        if (j != curElems.end() && (i == prevElems.end() || i->identifier() > j->identifier())) {
-            logger->cout("%s%s: ∅ -> %s", indent, j->identifier(), j->versions());
+    while (prevElemIt != prev.elements.end() || curElemIt != cur.elements.end()) {
+        if (curElemIt != cur.elements.end() && (prevElemIt == prev.elements.end() || prevElemIt->first > curElemIt->first)) {
+            logger->cout("%s%s: ∅ -> %s", indent, curElemIt->second.identifier(), curElemIt->second.versions());
             changes = true;
-            ++j;
-        } else if (i != prevElems.end() && (j == curElems.end() || i->identifier() < j->identifier())) {
-            logger->cout("%s%s: %s -> ∅", indent, i->identifier(), i->versions());
+            ++curElemIt;
+        } else if (prevElemIt != prev.elements.end() && (curElemIt == cur.elements.end() || prevElemIt->first < curElemIt->first)) {
+            logger->cout("%s%s: %s -> ∅", indent, prevElemIt->second.identifier(), prevElemIt->second.versions());
             changes = true;
-            ++i;
+            ++prevElemIt;
         } else {
-            auto v1 = i->versions();
-            auto v2 = j->versions();
+            auto v1 = prevElemIt->second.versions();
+            auto v2 = curElemIt->second.versions();
             if (v1 != v2) {
-                logger->cout("%s%s: %s -> %s", indent, i->identifier(), v1, v2);
+                logger->cout("%s%s: %s -> %s", indent, prevElemIt->second.identifier(), v1, v2);
                 changes = true;
             }
-            ++i;
-            ++j;
+            ++prevElemIt;
+            ++curElemIt;
         }
     }
 

diff --git a/src/libcmd/cmd-profiles.hh b/src/libcmd/cmd-profiles.hh
@@ -35,7 +35,6 @@ constexpr int DEFAULT_PRIORITY = 5;
 struct ProfileElement
 {
     StorePathSet storePaths;
-    std::string name;
     std::optional<ProfileElementSource> source;
     bool active = true;
     int priority = DEFAULT_PRIORITY;
@@ -57,7 +56,7 @@ struct ProfileElement
 
 struct ProfileManifest
 {
-    std::vector<ProfileElement> elements;
+    std::map<std::string, ProfileElement> elements;
 
     ProfileManifest() { }
 
@@ -67,6 +66,9 @@ struct ProfileManifest
 
     StorePath build(ref<Store> store);
 
+    void addElement(std::string_view nameCandidate, ProfileElement element);
+    void addElement(ProfileElement element);
+
     static void printDiff(const ProfileManifest & prev, const ProfileManifest & cur, std::string_view indent);
 };
 

diff --git a/src/nix/profile.cc b/src/nix/profile.cc
@@ -105,7 +105,7 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile
 
             element.updateStorePaths(getEvalStore(), store, res);
 
-            manifest.elements.push_back(std::move(element));
+            manifest.addElement(std::move(element));
         }
 
         try {
@@ -115,7 +115,7 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile
             //       See https://github.com/NixOS/nix/compare/3efa476c5439f8f6c1968a6ba20a31d1239c2f04..1fe5d172ece51a619e879c4b86f603d9495cc102
             auto findRefByFilePath = [&]<typename Iterator>(Iterator begin, Iterator end) {
                 for (auto it = begin; it != end; it++) {
-                    auto profileElement = *it;
+                    auto & profileElement = it->second;
                     for (auto & storePath : profileElement.storePaths) {
                         if (conflictError.fileA.starts_with(store->printStorePath(storePath))) {
                             return std::pair(conflictError.fileA, profileElement.toInstallables(*store));
@@ -202,13 +202,19 @@ class MixProfileElementMatchers : virtual Args
         return res;
     }
 
-    bool matches(const Store & store, const ProfileElement & element, const std::vector<Matcher> & matchers)
+    bool matches(
+        Store const & store,
+        // regex_match doesn't take a string_view lol
+        std::string const & name,
+        ProfileElement const & element,
+        std::vector<Matcher> const & matchers
+    )
     {
         for (auto & matcher : matchers) {
             if (auto path = std::get_if<Path>(&matcher)) {
                 if (element.storePaths.count(store.parseStorePath(*path))) return true;
             } else if (auto regex = std::get_if<RegexPattern>(&matcher)) {
-                if (std::regex_match(element.name, regex->reg)) {
+                if (std::regex_match(name, regex->reg)) {
                     return true;
                 }
             }
@@ -240,10 +246,9 @@ struct CmdProfileRemove : virtual EvalCommand, MixDefaultProfile, MixProfileElem
 
         ProfileManifest newManifest;
 
-        for (size_t i = 0; i < oldManifest.elements.size(); ++i) {
-            auto & element(oldManifest.elements[i]);
-            if (!matches(*store, element, matchers)) {
-                newManifest.elements.push_back(std::move(element));
+        for (auto & [name, element] : oldManifest.elements) {
+            if (!matches(*store, name, element, matchers)) {
+                newManifest.elements.insert_or_assign(name, std::move(element));
             } else {
                 notice("removing '%s'", element.identifier());
             }
@@ -289,14 +294,13 @@ struct CmdProfileUpgrade : virtual SourceExprCommand, MixDefaultProfile, MixProf
         auto matchers = getMatchers(store);
 
         Installables installables;
-        std::vector<size_t> indices;
+        std::vector<ProfileElement *> elems;
 
         auto matchedCount = 0;
         auto upgradedCount = 0;
 
-        for (size_t i = 0; i < manifest.elements.size(); ++i) {
-            auto & element(manifest.elements[i]);
-            if (!matches(*store, element, matchers)) {
+        for (auto & [name, element] : manifest.elements) {
+            if (!matches(*store, name, element, matchers)) {
                 continue;
             }
 
@@ -368,7 +372,7 @@ struct CmdProfileUpgrade : virtual SourceExprCommand, MixDefaultProfile, MixProf
             };
 
             installables.push_back(installable);
-            indices.push_back(i);
+            elems.push_back(&element);
 
         }
 
@@ -393,7 +397,7 @@ struct CmdProfileUpgrade : virtual SourceExprCommand, MixDefaultProfile, MixProf
 
         for (size_t i = 0; i < installables.size(); ++i) {
             auto & installable = installables.at(i);
-            auto & element = manifest.elements[indices.at(i)];
+            auto & element = *elems.at(i);
             element.updateStorePaths(
                 getEvalStore(),
                 store,
@@ -425,14 +429,14 @@ struct CmdProfileList : virtual EvalCommand, virtual StoreCommand, MixDefaultPro
         if (json) {
             std::cout << manifest.toJSON(*store).dump() << "\n";
         } else {
-            for (size_t i = 0; i < manifest.elements.size(); ++i) {
-                auto & element(manifest.elements[i]);
+            for (auto const & [i, nameElemPair] : enumerate(manifest.elements)) {
+                auto & [name, element] = nameElemPair;
                 if (i) {
                     logger->cout("");
                 }
                 logger->cout(
                     "Name:               " ANSI_BOLD "%s" ANSI_NORMAL "%s",
-                    element.name,
+                    name,
                     element.active ? "" : " " ANSI_RED "(inactive)" ANSI_NORMAL
                 );
                 if (element.source) {

diff --git a/src/nix/upgrade-nix.cc b/src/nix/upgrade-nix.cc
@@ -208,7 +208,8 @@ struct CmdUpgradeNix : MixDryRun, EvalCommand
         // Find which profile element has Nix in it.
         // It should be impossible to *not* have Nix, since we grabbed this
         // store path by looking for things with bin/nix-env in them anyway.
-        auto findNix = [&](ProfileElement const & elem) -> bool {
+        auto findNix = [&](std::pair<std::string, ProfileElement> const & nameElemPair) -> bool {
+            auto const & [name, elem] = nameElemPair;
             for (auto const & ePath : elem.storePaths) {
                 auto const nixEnv = store->printStorePath(ePath) + "/bin/nix-env";
                 if (pathExists(nixEnv)) {
@@ -226,14 +227,15 @@ struct CmdUpgradeNix : MixDryRun, EvalCommand
         // *Should* be impossible...
         assert(elemWithNix != std::end(manifest.elements));
 
+        auto const nixElemName = elemWithNix->first;
+
         // Now create a new profile element for the new Nix version...
         ProfileElement elemForNewNix = {
             .storePaths = {newNix},
         };
 
         // ...and splork it into the manifest where the old profile element was.
-        // (Remember, elemWithNix is an iterator)
-        *elemWithNix = elemForNewNix;
+        manifest.elements.at(nixElemName) = elemForNewNix;
 
         // Build the new profile, and switch to it.
         StorePath const newProfile = manifest.build(store);