From 71e0114708d406fdc0d9ca34d4b67cb190881439 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 8 Mar 2024 13:56:43 +0100 Subject: [PATCH] remove getDerivations deduplication deduplication does not currently work fully, showing derivations multiple times if they have different underlying values. this can happen by selecting the same derivation twice for two different attributes of a set, using inherit-from (which reduces to the previous), importing nixpkgs twice, or any other number of things. since users already have to deal with duplicates for this reason it won't hurt to add *more* duplicates. the alternative would be to deduplicate fully, which would drop derivations that are currently returned and those pose a regression risk. Change-Id: I64b397351237e10375d270f1bddecb71f62aa131 --- flake.nix | 2 +- src/libexpr/get-drvs.cc | 28 +++++++++++++++------------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/flake.nix b/flake.nix index d9e16881f..3e5023cb6 100644 --- a/flake.nix +++ b/flake.nix @@ -552,7 +552,7 @@ type -p nix-env # Note: we're filtering out nixos-install-tools because https://github.com/NixOS/nixpkgs/pull/153594#issuecomment-1020530593. time nix-env --store dummy:// -f ${nixpkgs-regression} -qaP --drv-path | sort | grep -v nixos-install-tools > packages - [[ $(sha1sum < packages | cut -c1-40) = ff451c521e61e4fe72bdbe2d0ca5d1809affa733 ]] + [[ $(sha1sum < packages | cut -c1-40) = 402242fca90874112b34718b8199d844e8b03d12 ]] mkdir $out ''; diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index 495407c39..e686ffe8c 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -296,22 +296,16 @@ void DrvInfo::setMeta(const std::string & name, Value * v) typedef std::set Done; -/* Evaluate value `v'. If it evaluates to a set of type `derivation', - then put information about it in `drvs' (unless it's already in `done'). - The result boolean indicates whether it makes sense +/* The result boolean indicates whether it makes sense for the caller to recursively search for derivations in `v'. */ static bool getDerivation(EvalState & state, Value & v, - const std::string & attrPath, DrvInfos & drvs, Done & done, + const std::string & attrPath, DrvInfos & drvs, bool ignoreAssertionFailures) { try { state.forceValue(v, v.determinePos(noPos)); if (!state.isDerivation(v)) return true; - /* Remove spurious duplicates (e.g., a set like `rec { x = - derivation {...}; y = x;}'. */ - if (!done.insert(v.attrs).second) return false; - DrvInfo drv(state, attrPath, v.attrs); drv.queryName(); @@ -330,9 +324,8 @@ static bool getDerivation(EvalState & state, Value & v, std::optional getDerivation(EvalState & state, Value & v, bool ignoreAssertionFailures) { - Done done; DrvInfos drvs; - getDerivation(state, v, "", drvs, done, ignoreAssertionFailures); + getDerivation(state, v, "", drvs, ignoreAssertionFailures); if (drvs.size() != 1) return {}; return std::move(drvs.front()); } @@ -347,6 +340,9 @@ static std::string addToPath(const std::string & s1, const std::string & s2) static std::regex attrRegex("[A-Za-z_][A-Za-z0-9-_+]*"); +/* Evaluate value `v'. If it evaluates to a set of type `derivation', + then put information about it in `drvs'. If it evaluates to a different + kind of set recurse (unless it's already in `done'). */ static void getDerivations(EvalState & state, Value & vIn, const std::string & pathPrefix, Bindings & autoArgs, DrvInfos & drvs, Done & done, @@ -356,10 +352,14 @@ static void getDerivations(EvalState & state, Value & vIn, state.autoCallFunction(autoArgs, vIn, v); /* Process the expression. */ - if (!getDerivation(state, v, pathPrefix, drvs, done, ignoreAssertionFailures)) ; + if (!getDerivation(state, v, pathPrefix, drvs, ignoreAssertionFailures)) ; else if (v.type() == nAttrs) { + /* Dont consider sets we've already seen, e.g. y in + `rec { x.d = derivation {...}; y = x; }`. */ + if (!done.insert(v.attrs).second) return; + /* !!! undocumented hackery to support combining channels in nix-env.cc. */ bool combineChannels = v.attrs->find(state.symbols.create("_combineChannels")) != v.attrs->end(); @@ -376,7 +376,7 @@ static void getDerivations(EvalState & state, Value & vIn, std::string pathPrefix2 = addToPath(pathPrefix, state.symbols[i->name]); if (combineChannels) getDerivations(state, *i->value, pathPrefix2, autoArgs, drvs, done, ignoreAssertionFailures); - else if (getDerivation(state, *i->value, pathPrefix2, drvs, done, ignoreAssertionFailures)) { + else if (getDerivation(state, *i->value, pathPrefix2, drvs, ignoreAssertionFailures)) { /* If the value of this attribute is itself a set, should we recurse into it? => Only if it has a `recurseForDerivations = true' attribute. */ @@ -390,9 +390,11 @@ static void getDerivations(EvalState & state, Value & vIn, } else if (v.type() == nList) { + // NOTE we can't really deduplicate here because small lists don't have stable addresses + // and can cause spurious duplicate detections due to v being on the stack. for (auto [n, elem] : enumerate(v.listItems())) { std::string pathPrefix2 = addToPath(pathPrefix, fmt("%d", n)); - if (getDerivation(state, *elem, pathPrefix2, drvs, done, ignoreAssertionFailures)) + if (getDerivation(state, *elem, pathPrefix2, drvs, ignoreAssertionFailures)) getDerivations(state, *elem, pathPrefix2, autoArgs, drvs, done, ignoreAssertionFailures); } }