From 3d144e778e4193c948fa6e0a23f9f3de194bc62f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Tue, 16 May 2023 11:31:28 +0200 Subject: [PATCH 1/2] Rename `ProfileElement.describe()` to `.identifier` This method isn't used to describe what the element is, but to return a unique identifier for it whithin the current profile --- src/nix/profile.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/nix/profile.cc b/src/nix/profile.cc index fd63b3519..ce3b6fbbc 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -42,7 +42,7 @@ struct ProfileElement bool active = true; int priority = defaultPriority; - std::string describe() const + std::string identifier() const { if (source) return fmt("%s#%s%s", source->originalRef, source->attrPath, source->outputs.to_string()); @@ -62,7 +62,7 @@ struct ProfileElement bool operator < (const ProfileElement & other) const { - return std::tuple(describe(), storePaths) < std::tuple(other.describe(), other.storePaths); + return std::tuple(identifier(), storePaths) < std::tuple(other.identifier(), other.storePaths); } void updateStorePaths( @@ -237,13 +237,13 @@ struct ProfileManifest bool changes = false; while (i != prevElems.end() || j != curElems.end()) { - if (j != curElems.end() && (i == prevElems.end() || i->describe() > j->describe())) { - logger->cout("%s%s: ∅ -> %s", indent, j->describe(), j->versions()); + if (j != curElems.end() && (i == prevElems.end() || i->identifier() > j->identifier())) { + logger->cout("%s%s: ∅ -> %s", indent, j->identifier(), j->versions()); changes = true; ++j; } - else if (i != prevElems.end() && (j == curElems.end() || i->describe() < j->describe())) { - logger->cout("%s%s: %s -> ∅", indent, i->describe(), i->versions()); + else if (i != prevElems.end() && (j == curElems.end() || i->identifier() < j->identifier())) { + logger->cout("%s%s: %s -> ∅", indent, i->identifier(), i->versions()); changes = true; ++i; } @@ -251,7 +251,7 @@ struct ProfileManifest auto v1 = i->versions(); auto v2 = j->versions(); if (v1 != v2) { - logger->cout("%s%s: %s -> %s", indent, i->describe(), v1, v2); + logger->cout("%s%s: %s -> %s", indent, i->identifier(), v1, v2); changes = true; } ++i; @@ -491,7 +491,7 @@ struct CmdProfileRemove : virtual EvalCommand, MixDefaultProfile, MixProfileElem if (!matches(*store, element, i, matchers)) { newManifest.elements.push_back(std::move(element)); } else { - notice("removing '%s'", element.describe()); + notice("removing '%s'", element.identifier()); } } From e997512523d8c5d22005b6cd5e22271284273ee6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Tue, 16 May 2023 11:37:45 +0200 Subject: [PATCH 2/2] Fix the printing of the installables on `nix profile install` conflict - If the element comes from a flake, print the full flakeref (with the fragment part) and not just the reference to the flake itself - If the element doesn't come from a flake, print its store path(s) This is a bit too verbose, but has the advantages of being correct (and not crashing), so it's strictly better than the previous situation Fix https://github.com/NixOS/nix/issues/8284 --- src/nix/profile.cc | 33 ++++++++++++++++++++++++++------- tests/nix-profile.sh | 13 ++++++++++--- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/src/nix/profile.cc b/src/nix/profile.cc index ce3b6fbbc..7cea616d2 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -31,6 +31,11 @@ struct ProfileElementSource std::tuple(originalRef.to_string(), attrPath, outputs) < std::tuple(other.originalRef.to_string(), other.attrPath, other.outputs); } + + std::string to_string() const + { + return fmt("%s#%s%s", originalRef, attrPath, outputs.to_string()); + } }; const int defaultPriority = 5; @@ -45,13 +50,27 @@ struct ProfileElement std::string identifier() const { if (source) - return fmt("%s#%s%s", source->originalRef, source->attrPath, source->outputs.to_string()); + return source->to_string(); StringSet names; for (auto & path : storePaths) names.insert(DrvName(path.name()).name); return concatStringsSep(", ", names); } + /** + * Return a string representing an installable corresponding to the current + * element, either a flakeref or a plain store path + */ + std::set toInstallables(Store & store) + { + if (source) + return {source->to_string()}; + StringSet rawPaths; + for (auto & path : storePaths) + rawPaths.insert(store.printStorePath(path)); + return rawPaths; + } + std::string versions() const { StringSet versions; @@ -363,10 +382,10 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile auto profileElement = *it; for (auto & storePath : profileElement.storePaths) { if (conflictError.fileA.starts_with(store->printStorePath(storePath))) { - return std::pair(conflictError.fileA, profileElement.source->originalRef); + return std::pair(conflictError.fileA, profileElement.toInstallables(*store)); } if (conflictError.fileB.starts_with(store->printStorePath(storePath))) { - return std::pair(conflictError.fileB, profileElement.source->originalRef); + return std::pair(conflictError.fileB, profileElement.toInstallables(*store)); } } } @@ -375,9 +394,9 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile // There are 2 conflicting files. We need to find out which one is from the already installed package and // which one is the package that is the new package that is being installed. // The first matching package is the one that was already installed (original). - auto [originalConflictingFilePath, originalConflictingRef] = findRefByFilePath(manifest.elements.begin(), manifest.elements.end()); + auto [originalConflictingFilePath, originalConflictingRefs] = findRefByFilePath(manifest.elements.begin(), manifest.elements.end()); // The last matching package is the one that was going to be installed (new). - auto [newConflictingFilePath, newConflictingRef] = findRefByFilePath(manifest.elements.rbegin(), manifest.elements.rend()); + auto [newConflictingFilePath, newConflictingRefs] = findRefByFilePath(manifest.elements.rbegin(), manifest.elements.rend()); throw Error( "An existing package already provides the following file:\n" @@ -403,8 +422,8 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile " nix profile install %4% --priority %7%\n", originalConflictingFilePath, newConflictingFilePath, - originalConflictingRef.to_string(), - newConflictingRef.to_string(), + concatStringsSep(" ", originalConflictingRefs), + concatStringsSep(" ", newConflictingRefs), conflictError.priority, conflictError.priority - 1, conflictError.priority + 1 diff --git a/tests/nix-profile.sh b/tests/nix-profile.sh index 4ef5b484a..9da3f802b 100644 --- a/tests/nix-profile.sh +++ b/tests/nix-profile.sh @@ -157,17 +157,17 @@ error: An existing package already provides the following file: To remove the existing package: - nix profile remove path:${flake1Dir} + nix profile remove path:${flake1Dir}#packages.${system}.default The new package can also be installed next to the existing one by assigning a different priority. The conflicting packages have a priority of 5. To prioritise the new package: - nix profile install path:${flake2Dir} --priority 4 + nix profile install path:${flake2Dir}#packages.${system}.default --priority 4 To prioritise the existing package: - nix profile install path:${flake2Dir} --priority 6 + nix profile install path:${flake2Dir}#packages.${system}.default --priority 6 EOF ) [[ $($TEST_HOME/.nix-profile/bin/hello) = "Hello World" ]] @@ -177,3 +177,10 @@ nix profile install $flake2Dir --priority 0 [[ $($TEST_HOME/.nix-profile/bin/hello) = "Hello World2" ]] # nix profile install $flake1Dir --priority 100 # [[ $($TEST_HOME/.nix-profile/bin/hello) = "Hello World" ]] + +# Ensure that conflicts are handled properly even when the installables aren't +# flake references. +# Regression test for https://github.com/NixOS/nix/issues/8284 +clearProfiles +nix profile install $(nix build $flake1Dir --no-link --print-out-paths) +expect 1 nix profile install --impure --expr "(builtins.getFlake ''$flake2Dir'').packages.$system.default"