From 04b113f6cb5356d44992f98928a595cdf93d561a Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 8 Mar 2022 06:02:25 +0100 Subject: [PATCH 01/41] Fix `nix log` with CA derivations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix #6209 When trying to run `nix log `, try first to resolve the derivation pointed to by `` as it is the resolved one that holds the build log. This has a couple of shortcomings: 1. It’s expensive as it requires re-reading the derivation 2. It’s brittle because if the derivation doesn’t exist anymore or can’t be resolved (which is the case if any one of its build inputs is missing), then we can’t access the log anymore However, I don’t think we can do better (at least not right now). The alternatives I see are: 1. Copy the build log for the un-resolved derivation. But that means a lot of duplication 2. Store the results of the resolving in the db. Which might be the best long-term solution, but leads to a whole new class of potential issues. --- src/libstore/binary-cache-store.cc | 16 +++------ src/libstore/local-fs-store.cc | 15 +++----- src/libstore/store-api.cc | 28 +++++++++++++++ src/libstore/store-api.hh | 7 ++++ tests/build-hook-ca-floating.nix | 55 +++--------------------------- tests/build-hook.nix | 11 ++++-- tests/build-remote.sh | 9 ++--- 7 files changed, 59 insertions(+), 82 deletions(-) diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 12d0c32fb..14584f0a2 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -504,18 +504,10 @@ void BinaryCacheStore::addSignatures(const StorePath & storePath, const StringSe std::optional BinaryCacheStore::getBuildLog(const StorePath & path) { - auto drvPath = path; - - if (!path.isDerivation()) { - try { - auto info = queryPathInfo(path); - // FIXME: add a "Log" field to .narinfo - if (!info->deriver) return std::nullopt; - drvPath = *info->deriver; - } catch (InvalidPath &) { - return std::nullopt; - } - } + auto maybePath = getBuildDerivationPath(path); + if (!maybePath) + return std::nullopt; + auto drvPath = maybePath.value(); auto logPath = "log/" + std::string(baseNameOf(printStorePath(drvPath))); diff --git a/src/libstore/local-fs-store.cc b/src/libstore/local-fs-store.cc index c5ae7536f..3a0585b15 100644 --- a/src/libstore/local-fs-store.cc +++ b/src/libstore/local-fs-store.cc @@ -89,17 +89,10 @@ const std::string LocalFSStore::drvsLogDir = "drvs"; std::optional LocalFSStore::getBuildLog(const StorePath & path_) { - auto path = path_; - - if (!path.isDerivation()) { - try { - auto info = queryPathInfo(path); - if (!info->deriver) return std::nullopt; - path = *info->deriver; - } catch (InvalidPath &) { - return std::nullopt; - } - } + auto maybePath = getBuildDerivationPath(path_); + if (!maybePath) + return std::nullopt; + auto path = maybePath.value(); auto baseName = path.to_string(); diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 8811ab578..ebd641aa4 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1300,6 +1300,34 @@ Derivation readDerivationCommon(Store& store, const StorePath& drvPath, bool req } } +std::optional Store::getBuildDerivationPath(const StorePath & path) +{ + + if (!path.isDerivation()) { + try { + auto info = queryPathInfo(path); + if (!info->deriver) return std::nullopt; + return *info->deriver; + } catch (InvalidPath &) { + return std::nullopt; + } + } + + if (!settings.isExperimentalFeatureEnabled(Xp::CaDerivations) || !isValidPath(path)) + return path; + + auto drv = readDerivation(path); + if (!derivationHasKnownOutputPaths(drv.type())) { + // The build log is actually attached to the corresponding + // resolved derivation, so we need to get it first + auto resolvedDrv = drv.tryResolve(*this); + if (resolvedDrv) + return writeDerivation(*this, *resolvedDrv, NoRepair, true); + } + + return path; +} + Derivation Store::readDerivation(const StorePath & drvPath) { return readDerivationCommon(*this, drvPath, true); } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 151ec10d6..88a11d953 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -618,6 +618,13 @@ public: */ StorePathSet exportReferences(const StorePathSet & storePaths, const StorePathSet & inputPaths); + /** + * Given a store path, return the realisation actually used in the realisation of this path: + * - If the path is a content-addressed derivation, try to resolve it + * - Otherwise, find one of its derivers + */ + std::optional getBuildDerivationPath(const StorePath &); + /* Hack to allow long-running processes like hydra-queue-runner to occasionally flush their path info cache. */ void clearPathInfoCache() diff --git a/tests/build-hook-ca-floating.nix b/tests/build-hook-ca-floating.nix index 67295985f..dfdbb82da 100644 --- a/tests/build-hook-ca-floating.nix +++ b/tests/build-hook-ca-floating.nix @@ -1,53 +1,6 @@ { busybox }: -with import ./config.nix; - -let - - mkDerivation = args: - derivation ({ - inherit system; - builder = busybox; - args = ["sh" "-e" args.builder or (builtins.toFile "builder-${args.name}.sh" "if [ -e .attrs.sh ]; then source .attrs.sh; fi; eval \"$buildCommand\"")]; - outputHashMode = "recursive"; - outputHashAlgo = "sha256"; - __contentAddressed = true; - } // removeAttrs args ["builder" "meta"]) - // { meta = args.meta or {}; }; - - input1 = mkDerivation { - shell = busybox; - name = "build-remote-input-1"; - buildCommand = "echo FOO > $out"; - requiredSystemFeatures = ["foo"]; - }; - - input2 = mkDerivation { - shell = busybox; - name = "build-remote-input-2"; - buildCommand = "echo BAR > $out"; - requiredSystemFeatures = ["bar"]; - }; - - input3 = mkDerivation { - shell = busybox; - name = "build-remote-input-3"; - buildCommand = '' - read x < ${input2} - echo $x BAZ > $out - ''; - requiredSystemFeatures = ["baz"]; - }; - -in - - mkDerivation { - shell = busybox; - name = "build-remote"; - buildCommand = - '' - read x < ${input1} - read y < ${input3} - echo "$x $y" > $out - ''; - } +import ./build-hook.nix { + inherit busybox; + contentAddressed = true; +} diff --git a/tests/build-hook.nix b/tests/build-hook.nix index 643334caa..7effd7903 100644 --- a/tests/build-hook.nix +++ b/tests/build-hook.nix @@ -1,15 +1,22 @@ -{ busybox }: +{ busybox, contentAddressed ? false }: with import ./config.nix; let + caArgs = if contentAddressed then { + outputHashMode = "recursive"; + outputHashAlgo = "sha256"; + __contentAddressed = true; + } else {}; + mkDerivation = args: derivation ({ inherit system; builder = busybox; args = ["sh" "-e" args.builder or (builtins.toFile "builder-${args.name}.sh" "if [ -e .attrs.sh ]; then source .attrs.sh; fi; eval \"$buildCommand\"")]; - } // removeAttrs args ["builder" "meta" "passthru"]) + } // removeAttrs args ["builder" "meta" "passthru"] + // caArgs) // { meta = args.meta or {}; passthru = args.passthru or {}; }; input1 = mkDerivation { diff --git a/tests/build-remote.sh b/tests/build-remote.sh index e73c37ea4..25a482003 100644 --- a/tests/build-remote.sh +++ b/tests/build-remote.sh @@ -63,12 +63,9 @@ nix path-info --store $TEST_ROOT/machine3 --all \ | grep builder-build-remote-input-3.sh -# Temporarily disabled because of https://github.com/NixOS/nix/issues/6209 -if [[ -z "$CONTENT_ADDRESSED" ]]; then - for i in input1 input3; do - nix log --store $TEST_ROOT/machine0 --file "$file" --arg busybox $busybox passthru."$i" | grep hi-$i - done -fi +for i in input1 input3; do +nix log --store $TEST_ROOT/machine0 --file "$file" --arg busybox $busybox passthru."$i" | grep hi-$i +done # Behavior of keep-failed out="$(nix-build 2>&1 failing.nix \ From 3b27181ee54176aba2cdff98028586048e08e74d Mon Sep 17 00:00:00 2001 From: Taeer Bar-Yam Date: Thu, 8 Dec 2022 16:59:21 -0500 Subject: [PATCH 02/41] fix missing function after rebase --- src/libstore/store-api.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index ebd641aa4..6c0261446 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1317,7 +1317,7 @@ std::optional Store::getBuildDerivationPath(const StorePath & path) return path; auto drv = readDerivation(path); - if (!derivationHasKnownOutputPaths(drv.type())) { + if (!drv.type().hasKnownOutputPaths()) { // The build log is actually attached to the corresponding // resolved derivation, so we need to get it first auto resolvedDrv = drv.tryResolve(*this); From e5eb05c5990d837e0bbc1529e3b5b167f7015be0 Mon Sep 17 00:00:00 2001 From: Taeer Bar-Yam Date: Thu, 15 Dec 2022 15:58:54 -0500 Subject: [PATCH 03/41] getBuildLog: factor out resolving derivations --- src/libstore/binary-cache-store.cc | 9 ++------- src/libstore/binary-cache-store.hh | 2 +- src/libstore/build/local-derivation-goal.cc | 2 +- src/libstore/local-fs-store.cc | 7 +------ src/libstore/local-fs-store.hh | 2 +- src/libstore/log-store.hh | 9 ++++++++- src/libstore/ssh-store.cc | 4 ++-- 7 files changed, 16 insertions(+), 19 deletions(-) diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 14584f0a2..0fef2d23a 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -502,14 +502,9 @@ void BinaryCacheStore::addSignatures(const StorePath & storePath, const StringSe writeNarInfo(narInfo); } -std::optional BinaryCacheStore::getBuildLog(const StorePath & path) +std::optional BinaryCacheStore::getBuildLogExact(const StorePath & path) { - auto maybePath = getBuildDerivationPath(path); - if (!maybePath) - return std::nullopt; - auto drvPath = maybePath.value(); - - auto logPath = "log/" + std::string(baseNameOf(printStorePath(drvPath))); + auto logPath = "log/" + std::string(baseNameOf(printStorePath(path))); debug("fetching build log from binary cache '%s/%s'", getUri(), logPath); diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh index 8c82e2387..abd92a83c 100644 --- a/src/libstore/binary-cache-store.hh +++ b/src/libstore/binary-cache-store.hh @@ -129,7 +129,7 @@ public: void addSignatures(const StorePath & storePath, const StringSet & sigs) override; - std::optional getBuildLog(const StorePath & path) override; + std::optional getBuildLogExact(const StorePath & path) override; void addBuildLog(const StorePath & drvPath, std::string_view log) override; diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 6fe3bc49c..c867c0f1d 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1460,7 +1460,7 @@ struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual Lo unknown, downloadSize, narSize); } - virtual std::optional getBuildLog(const StorePath & path) override + virtual std::optional getBuildLogExact(const StorePath & path) override { return std::nullopt; } virtual void addBuildLog(const StorePath & path, std::string_view log) override diff --git a/src/libstore/local-fs-store.cc b/src/libstore/local-fs-store.cc index 3a0585b15..b224fc3e9 100644 --- a/src/libstore/local-fs-store.cc +++ b/src/libstore/local-fs-store.cc @@ -87,13 +87,8 @@ void LocalFSStore::narFromPath(const StorePath & path, Sink & sink) const std::string LocalFSStore::drvsLogDir = "drvs"; -std::optional LocalFSStore::getBuildLog(const StorePath & path_) +std::optional LocalFSStore::getBuildLogExact(const StorePath & path) { - auto maybePath = getBuildDerivationPath(path_); - if (!maybePath) - return std::nullopt; - auto path = maybePath.value(); - auto baseName = path.to_string(); for (int j = 0; j < 2; j++) { diff --git a/src/libstore/local-fs-store.hh b/src/libstore/local-fs-store.hh index e6fb3201a..947707341 100644 --- a/src/libstore/local-fs-store.hh +++ b/src/libstore/local-fs-store.hh @@ -50,7 +50,7 @@ public: return getRealStoreDir() + "/" + std::string(storePath, storeDir.size() + 1); } - std::optional getBuildLog(const StorePath & path) override; + std::optional getBuildLogExact(const StorePath & path) override; }; diff --git a/src/libstore/log-store.hh b/src/libstore/log-store.hh index ff1b92e17..b807e3e71 100644 --- a/src/libstore/log-store.hh +++ b/src/libstore/log-store.hh @@ -11,7 +11,14 @@ struct LogStore : public virtual Store /* Return the build log of the specified store path, if available, or null otherwise. */ - virtual std::optional getBuildLog(const StorePath & path) = 0; + std::optional getBuildLog(const StorePath & path) { + auto maybePath = getBuildDerivationPath(path); + if (!maybePath) + return std::nullopt; + return getBuildLogExact(maybePath.value()); + } + + virtual std::optional getBuildLogExact(const StorePath & path) = 0; virtual void addBuildLog(const StorePath & path, std::string_view log) = 0; diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index 62daa838c..a1d4daafd 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -53,8 +53,8 @@ public: { return false; } // FIXME extend daemon protocol, move implementation to RemoteStore - std::optional getBuildLog(const StorePath & path) override - { unsupported("getBuildLog"); } + std::optional getBuildLogExact(const StorePath & path) override + { unsupported("getBuildLogExact"); } private: From 845fc3f605be6dd1140ab81eefb969da1cc5346b Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 15 Dec 2022 22:09:32 +0100 Subject: [PATCH 04/41] Merge toDerivations() into toDerivedPaths() toDerivedPaths() now returns DerivedPathWithInfo, which is DerivedPath with some attributes needed by 'nix profile' etc. Preparation for #7417. --- src/libcmd/installables.cc | 175 +++++++++++++++++-------------------- src/libcmd/installables.hh | 47 +++++----- src/nix/app.cc | 5 +- src/nix/build.cc | 10 ++- src/nix/log.cc | 2 +- src/nix/profile.cc | 67 ++++++++------ src/nix/store-copy-log.cc | 8 +- src/nix/why-depends.cc | 2 +- 8 files changed, 154 insertions(+), 162 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 5cdd3e12c..97bad5c45 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -358,7 +358,7 @@ void completeFlakeRef(ref store, std::string_view prefix) } } -DerivedPath Installable::toDerivedPath() +DerivedPathWithInfo Installable::toDerivedPath() { auto buildables = toDerivedPaths(); if (buildables.size() != 1) @@ -422,21 +422,9 @@ struct InstallableStorePath : Installable return req.to_string(*store); } - DerivedPaths toDerivedPaths() override + DerivedPathsWithInfo toDerivedPaths() override { - return { req }; - } - - StorePathSet toDrvPaths(ref store) override - { - return std::visit(overloaded { - [&](const DerivedPath::Built & bfd) -> StorePathSet { - return { bfd.drvPath }; - }, - [&](const DerivedPath::Opaque & bo) -> StorePathSet { - return { getDeriver(store, *this, bo.path) }; - }, - }, req.raw()); + return {{req}}; } std::optional getStorePath() override @@ -452,34 +440,6 @@ struct InstallableStorePath : Installable } }; -DerivedPaths InstallableValue::toDerivedPaths() -{ - DerivedPaths res; - - std::map> drvsToOutputs; - RealisedPath::Set drvsToCopy; - - // Group by derivation, helps with .all in particular - for (auto & drv : toDerivations()) { - for (auto & outputName : drv.outputsToInstall) - drvsToOutputs[drv.drvPath].insert(outputName); - drvsToCopy.insert(drv.drvPath); - } - - for (auto & i : drvsToOutputs) - res.push_back(DerivedPath::Built { i.first, i.second }); - - return res; -} - -StorePathSet InstallableValue::toDrvPaths(ref store) -{ - StorePathSet res; - for (auto & drv : toDerivations()) - res.insert(drv.drvPath); - return res; -} - struct InstallableAttrPath : InstallableValue { SourceExprCommand & cmd; @@ -509,40 +469,52 @@ struct InstallableAttrPath : InstallableValue return {vRes, pos}; } - virtual std::vector toDerivations() override; -}; + DerivedPathsWithInfo toDerivedPaths() override + { + auto v = toValue(*state).first; -std::vector InstallableAttrPath::toDerivations() -{ - auto v = toValue(*state).first; + Bindings & autoArgs = *cmd.getAutoArgs(*state); - Bindings & autoArgs = *cmd.getAutoArgs(*state); + DrvInfos drvInfos; + getDerivations(*state, *v, "", autoArgs, drvInfos, false); - DrvInfos drvInfos; - getDerivations(*state, *v, "", autoArgs, drvInfos, false); + DerivedPathsWithInfo res; - std::vector res; - for (auto & drvInfo : drvInfos) { - auto drvPath = drvInfo.queryDrvPath(); - if (!drvPath) - throw Error("'%s' is not a derivation", what()); + // Backward compatibility hack: group results by drvPath. This + // helps keep .all output together. + std::map byDrvPath; - std::set outputsToInstall; + for (auto & drvInfo : drvInfos) { + auto drvPath = drvInfo.queryDrvPath(); + if (!drvPath) + throw Error("'%s' is not a derivation", what()); - if (auto outputNames = std::get_if(&outputsSpec)) - outputsToInstall = *outputNames; - else - for (auto & output : drvInfo.queryOutputs(false, std::get_if(&outputsSpec))) - outputsToInstall.insert(output.first); + std::set outputsToInstall; - res.push_back(DerivationInfo { - .drvPath = *drvPath, - .outputsToInstall = std::move(outputsToInstall) - }); + if (auto outputNames = std::get_if(&outputsSpec)) + outputsToInstall = *outputNames; + else + for (auto & output : drvInfo.queryOutputs(false, std::get_if(&outputsSpec))) + outputsToInstall.insert(output.first); + + auto i = byDrvPath.find(*drvPath); + if (i == byDrvPath.end()) { + byDrvPath[*drvPath] = res.size(); + res.push_back({ + .path = DerivedPath::Built { + .drvPath = std::move(*drvPath), + .outputs = std::move(outputsToInstall), + } + }); + } else { + for (auto & output : outputsToInstall) + std::get(res[i->second].path).outputs.insert(output); + } + } + + return res; } - - return res; -} +}; std::vector InstallableFlake::getActualAttrPaths() { @@ -630,7 +602,7 @@ InstallableFlake::InstallableFlake( throw UsageError("'--arg' and '--argstr' are incompatible with flakes"); } -std::tuple InstallableFlake::toDerivation() +DerivedPathsWithInfo InstallableFlake::toDerivedPaths() { Activity act(*logger, lvlTalkative, actUnknown, fmt("evaluating derivation '%s'", what())); @@ -674,20 +646,19 @@ std::tuple InstallableF if (auto outputNames = std::get_if(&outputsSpec)) outputsToInstall = *outputNames; - auto drvInfo = DerivationInfo { - .drvPath = std::move(drvPath), - .outputsToInstall = std::move(outputsToInstall), - .priority = priority, - }; - - return {attrPath, getLockedFlake()->flake.lockedRef, std::move(drvInfo)}; -} - -std::vector InstallableFlake::toDerivations() -{ - std::vector res; - res.push_back(std::get<2>(toDerivation())); - return res; + return {{ + .path = DerivedPath::Built { + .drvPath = std::move(drvPath), + .outputs = std::move(outputsToInstall), + }, + .info = { + .priority = priority, + .originalRef = flakeRef, + .resolvedRef = getLockedFlake()->flake.lockedRef, + .attrPath = attrPath, + .outputsSpec = outputsSpec, + } + }}; } std::pair InstallableFlake::toValue(EvalState & state) @@ -895,13 +866,19 @@ std::vector, BuiltPathWithResult>> Instal if (mode == Realise::Nothing) settings.readOnlyMode = true; + struct Aux + { + ExtraInfo info; + std::shared_ptr installable; + }; + std::vector pathsToBuild; - std::map>> backmap; + std::map> backmap; for (auto & i : installables) { for (auto b : i->toDerivedPaths()) { - pathsToBuild.push_back(b); - backmap[b].push_back(i); + pathsToBuild.push_back(b.path); + backmap[b.path].push_back({.info = b.info, .installable = i}); } } @@ -914,7 +891,7 @@ std::vector, BuiltPathWithResult>> Instal printMissing(store, pathsToBuild, lvlError); for (auto & path : pathsToBuild) { - for (auto & installable : backmap[path]) { + for (auto & aux : backmap[path]) { std::visit(overloaded { [&](const DerivedPath::Built & bfd) { OutputPathMap outputs; @@ -946,10 +923,14 @@ std::vector, BuiltPathWithResult>> Instal output, *drvOutput->second); } } - res.push_back({installable, {.path = BuiltPath::Built { bfd.drvPath, outputs }}}); + res.push_back({aux.installable, { + .path = BuiltPath::Built { bfd.drvPath, outputs }, + .info = aux.info}}); }, [&](const DerivedPath::Opaque & bo) { - res.push_back({installable, {.path = BuiltPath::Opaque { bo.path }}}); + res.push_back({aux.installable, { + .path = BuiltPath::Opaque { bo.path }, + .info = aux.info}}); }, }, path.raw()); } @@ -965,16 +946,22 @@ std::vector, BuiltPathWithResult>> Instal if (!buildResult.success()) buildResult.rethrow(); - for (auto & installable : backmap[buildResult.path]) { + for (auto & aux : backmap[buildResult.path]) { std::visit(overloaded { [&](const DerivedPath::Built & bfd) { std::map outputs; for (auto & path : buildResult.builtOutputs) outputs.emplace(path.first.outputName, path.second.outPath); - res.push_back({installable, {.path = BuiltPath::Built { bfd.drvPath, outputs }, .result = buildResult}}); + res.push_back({aux.installable, { + .path = BuiltPath::Built { bfd.drvPath, outputs }, + .info = aux.info, + .result = buildResult}}); }, [&](const DerivedPath::Opaque & bo) { - res.push_back({installable, {.path = BuiltPath::Opaque { bo.path }, .result = buildResult}}); + res.push_back({aux.installable, { + .path = BuiltPath::Opaque { bo.path }, + .info = aux.info, + .result = buildResult}}); }, }, buildResult.path.raw()); } @@ -1059,7 +1046,7 @@ StorePathSet Installable::toDerivations( [&](const DerivedPath::Built & bfd) { drvPaths.insert(bfd.drvPath); }, - }, b.raw()); + }, b.path.raw()); return drvPaths; } diff --git a/src/libcmd/installables.hh b/src/libcmd/installables.hh index 02ea351d3..95ab4a40e 100644 --- a/src/libcmd/installables.hh +++ b/src/libcmd/installables.hh @@ -52,26 +52,42 @@ enum class OperateOn { Derivation }; +struct ExtraInfo +{ + std::optional priority; + std::optional originalRef; + std::optional resolvedRef; + std::optional attrPath; + // FIXME: merge with DerivedPath's 'outputs' field? + std::optional outputsSpec; +}; + +/* A derived path with any additional info that commands might + need from the derivation. */ +struct DerivedPathWithInfo +{ + DerivedPath path; + ExtraInfo info; +}; + struct BuiltPathWithResult { BuiltPath path; + ExtraInfo info; std::optional result; }; +typedef std::vector DerivedPathsWithInfo; + struct Installable { virtual ~Installable() { } virtual std::string what() const = 0; - virtual DerivedPaths toDerivedPaths() = 0; + virtual DerivedPathsWithInfo toDerivedPaths() = 0; - virtual StorePathSet toDrvPaths(ref store) - { - throw Error("'%s' cannot be converted to a derivation path", what()); - } - - DerivedPath toDerivedPath(); + DerivedPathWithInfo toDerivedPath(); UnresolvedApp toApp(EvalState & state); @@ -146,19 +162,6 @@ struct InstallableValue : Installable ref state; InstallableValue(ref state) : state(state) {} - - struct DerivationInfo - { - StorePath drvPath; - std::set outputsToInstall; - std::optional priority; - }; - - virtual std::vector toDerivations() = 0; - - DerivedPaths toDerivedPaths() override; - - StorePathSet toDrvPaths(ref store) override; }; struct InstallableFlake : InstallableValue @@ -186,9 +189,7 @@ struct InstallableFlake : InstallableValue Value * getFlakeOutputs(EvalState & state, const flake::LockedFlake & lockedFlake); - std::tuple toDerivation(); - - std::vector toDerivations() override; + DerivedPathsWithInfo toDerivedPaths() override; std::pair toValue(EvalState & state) override; diff --git a/src/nix/app.cc b/src/nix/app.cc index 5658f2a52..a8d7e115b 100644 --- a/src/nix/app.cc +++ b/src/nix/app.cc @@ -19,12 +19,11 @@ struct InstallableDerivedPath : Installable { } - std::string what() const override { return derivedPath.to_string(*store); } - DerivedPaths toDerivedPaths() override + DerivedPathsWithInfo toDerivedPaths() override { - return {derivedPath}; + return {{derivedPath}}; } std::optional getStorePath() override diff --git a/src/nix/build.cc b/src/nix/build.cc index 94b169167..12b22d999 100644 --- a/src/nix/build.cc +++ b/src/nix/build.cc @@ -94,13 +94,15 @@ struct CmdBuild : InstallablesCommand, MixDryRun, MixJSON, MixProfile if (dryRun) { std::vector pathsToBuild; - for (auto & i : installables) { - auto b = i->toDerivedPaths(); - pathsToBuild.insert(pathsToBuild.end(), b.begin(), b.end()); - } + for (auto & i : installables) + for (auto & b : i->toDerivedPaths()) + pathsToBuild.push_back(b.path); + printMissing(store, pathsToBuild, lvlError); + if (json) logger->cout("%s", derivedPathsToJSON(pathsToBuild, store).dump()); + return; } diff --git a/src/nix/log.cc b/src/nix/log.cc index 72d02ef11..a0598ca13 100644 --- a/src/nix/log.cc +++ b/src/nix/log.cc @@ -49,7 +49,7 @@ struct CmdLog : InstallableCommand [&](const DerivedPath::Built & bfd) { return logSub.getBuildLog(bfd.drvPath); }, - }, b.raw()); + }, b.path.raw()); if (!log) continue; stopProgressBar(); printInfo("got build log for '%s' from '%s'", installable->what(), logSub.getUri()); diff --git a/src/nix/profile.cc b/src/nix/profile.cc index 11910523d..db702db1b 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -32,12 +32,14 @@ struct ProfileElementSource } }; +const int defaultPriority = 5; + struct ProfileElement { StorePathSet storePaths; std::optional source; bool active = true; - int priority = 5; + int priority = defaultPriority; std::string describe() const { @@ -251,13 +253,19 @@ struct ProfileManifest } }; -static std::map +static std::map> builtPathsPerInstallable( const std::vector, BuiltPathWithResult>> & builtPaths) { - std::map res; - for (auto & [installable, builtPath] : builtPaths) - res[installable.get()].push_back(builtPath.path); + std::map> res; + for (auto & [installable, builtPath] : builtPaths) { + auto & r = res[installable.get()]; + /* Note that there could be conflicting info + (e.g. meta.priority fields) if the installable returned + multiple derivations. So pick one arbitrarily. */ + r.first.push_back(builtPath.path); + r.second = builtPath.info; + } return res; } @@ -297,28 +305,25 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile for (auto & installable : installables) { ProfileElement element; + auto & [res, info] = builtPaths[installable.get()]; - - if (auto installable2 = std::dynamic_pointer_cast(installable)) { - // FIXME: make build() return this? - auto [attrPath, resolvedRef, drv] = installable2->toDerivation(); + if (info.originalRef && info.resolvedRef && info.attrPath && info.outputsSpec) { element.source = ProfileElementSource { - installable2->flakeRef, - resolvedRef, - attrPath, - installable2->outputsSpec + .originalRef = *info.originalRef, + .resolvedRef = *info.resolvedRef, + .attrPath = *info.attrPath, + .outputs = *info.outputsSpec, }; - - if(drv.priority) { - element.priority = *drv.priority; - } } - if(priority) { // if --priority was specified we want to override the priority of the installable - element.priority = *priority; - }; + // If --priority was specified we want to override the + // priority of the installable. + element.priority = + priority + ? *priority + : info.priority.value_or(defaultPriority); - element.updateStorePaths(getEvalStore(), store, builtPaths[installable.get()]); + element.updateStorePaths(getEvalStore(), store, res); manifest.elements.push_back(std::move(element)); } @@ -476,18 +481,22 @@ struct CmdProfileUpgrade : virtual SourceExprCommand, MixDefaultProfile, MixProf Strings{}, lockFlags); - auto [attrPath, resolvedRef, drv] = installable->toDerivation(); + auto derivedPaths = installable->toDerivedPaths(); + if (derivedPaths.empty()) continue; + auto & info = derivedPaths[0].info; - if (element.source->resolvedRef == resolvedRef) continue; + assert(info.resolvedRef && info.attrPath); + + if (element.source->resolvedRef == info.resolvedRef) continue; printInfo("upgrading '%s' from flake '%s' to '%s'", - element.source->attrPath, element.source->resolvedRef, resolvedRef); + element.source->attrPath, element.source->resolvedRef, *info.resolvedRef); element.source = ProfileElementSource { - installable->flakeRef, - resolvedRef, - attrPath, - installable->outputsSpec + .originalRef = installable->flakeRef, + .resolvedRef = *info.resolvedRef, + .attrPath = *info.attrPath, + .outputs = installable->outputsSpec, }; installables.push_back(installable); @@ -515,7 +524,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)]; - element.updateStorePaths(getEvalStore(), store, builtPaths[installable.get()]); + element.updateStorePaths(getEvalStore(), store, builtPaths[installable.get()].first); } updateProfile(manifest.build(store)); diff --git a/src/nix/store-copy-log.cc b/src/nix/store-copy-log.cc index 2e288f743..d5fab5f2f 100644 --- a/src/nix/store-copy-log.cc +++ b/src/nix/store-copy-log.cc @@ -33,13 +33,7 @@ struct CmdCopyLog : virtual CopyCommand, virtual InstallablesCommand auto dstStore = getDstStore(); auto & dstLogStore = require(*dstStore); - StorePathSet drvPaths; - - for (auto & i : installables) - for (auto & drvPath : i->toDrvPaths(getEvalStore())) - drvPaths.insert(drvPath); - - for (auto & drvPath : drvPaths) { + for (auto & drvPath : Installable::toDerivations(getEvalStore(), installables, true)) { if (auto log = srcLogStore.getBuildLog(drvPath)) dstLogStore.addBuildLog(drvPath, *log); else diff --git a/src/nix/why-depends.cc b/src/nix/why-depends.cc index 723017497..661df965e 100644 --- a/src/nix/why-depends.cc +++ b/src/nix/why-depends.cc @@ -111,7 +111,7 @@ struct CmdWhyDepends : SourceExprCommand } return maybePath->second; }, - }, derivedDependency.raw()); + }, derivedDependency.path.raw()); StorePathSet closure; store->computeFSClosure({packagePath}, closure, false, false); From bda879170fbf8ff8c5397948ccc0e1695d23871a Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 20 Dec 2022 14:58:39 +0100 Subject: [PATCH 05/41] EvalState::copyPathToStore(): Return a StorePath --- src/libexpr/eval.cc | 28 ++++++++++++++-------------- src/libexpr/eval.hh | 2 +- src/libexpr/value-to-json.cc | 3 ++- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 084ccbee2..29d109ac4 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -2250,7 +2250,7 @@ BackedStringView EvalState::coerceToString(const PosIdx pos, Value & v, PathSet if (canonicalizePath) path = canonPath(*path); if (copyToStore) - path = copyPathToStore(context, std::move(path).toOwned()); + path = store->printStorePath(copyPathToStore(context, std::move(path).toOwned())); return path; } @@ -2293,26 +2293,26 @@ BackedStringView EvalState::coerceToString(const PosIdx pos, Value & v, PathSet } -std::string EvalState::copyPathToStore(PathSet & context, const Path & path) +StorePath EvalState::copyPathToStore(PathSet & context, const Path & path) { if (nix::isDerivation(path)) throwEvalError("file names are not allowed to end in '%1%'", drvExtension); - Path dstPath; - auto i = srcToStore.find(path); - if (i != srcToStore.end()) - dstPath = store->printStorePath(i->second); - else { - auto p = settings.readOnlyMode + auto dstPath = [&]() -> StorePath + { + auto i = srcToStore.find(path); + if (i != srcToStore.end()) return i->second; + + auto dstPath = settings.readOnlyMode ? store->computeStorePathForPath(std::string(baseNameOf(path)), checkSourcePath(path)).first : store->addToStore(std::string(baseNameOf(path)), checkSourcePath(path), FileIngestionMethod::Recursive, htSHA256, defaultPathFilter, repair); - dstPath = store->printStorePath(p); - allowPath(p); - srcToStore.insert_or_assign(path, std::move(p)); - printMsg(lvlChatty, "copied source '%1%' -> '%2%'", path, dstPath); - } + allowPath(dstPath); + srcToStore.insert_or_assign(path, dstPath); + printMsg(lvlChatty, "copied source '%1%' -> '%2%'", path, store->printStorePath(dstPath)); + return dstPath; + }(); - context.insert(dstPath); + context.insert(store->printStorePath(dstPath)); return dstPath; } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 21666339b..52b1736fe 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -400,7 +400,7 @@ public: bool coerceMore = false, bool copyToStore = true, bool canonicalizePath = true); - std::string copyPathToStore(PathSet & context, const Path & path); + StorePath copyPathToStore(PathSet & context, const Path & path); /* Path coercion. Converts strings, paths and derivations to a path. The result is guaranteed to be a canonicalised, absolute diff --git a/src/libexpr/value-to-json.cc b/src/libexpr/value-to-json.cc index 5dc453b2e..c35c876e3 100644 --- a/src/libexpr/value-to-json.cc +++ b/src/libexpr/value-to-json.cc @@ -1,6 +1,7 @@ #include "value-to-json.hh" #include "eval-inline.hh" #include "util.hh" +#include "store-api.hh" #include #include @@ -35,7 +36,7 @@ json printValueAsJSON(EvalState & state, bool strict, case nPath: if (copyToStore) - out = state.copyPathToStore(context, v.path); + out = state.store->printStorePath(state.copyPathToStore(context, v.path)); else out = v.path; break; From 5c97b5a3988c7dd28e617734c2eba669ee0c1288 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 15 Dec 2022 23:01:15 +0100 Subject: [PATCH 06/41] InstallableFlake::toDerivedPaths(): Support paths and store paths This makes 'nix build' work on paths (which will be copied to the store) and store paths (returned as is). E.g. the following flake output attributes can be built using 'nix build .#foo': foo = ./src; foo = self.outPath; foo = builtins.fetchTarball { ... }; foo = (builtins.fetchTree { .. }).outPath; foo = builtins.fetchTree { .. } + "/README.md"; foo = builtins.storePath /nix/store/...; Note that this is potentially risky, e.g. foo = /.; will cause Nix to try to copy the entire file system to the store. What doesn't work yet: foo = self; foo = builtins.fetchTree { .. }; because we don't handle attrsets with an outPath attribute in it yet, and foo = builtins.storePath /nix/store/.../README.md; since result symlinks have to point to a store path currently (rather than a file inside a store path). Fixes #7417. --- src/libcmd/installables.cc | 34 +++++++++++++++++-- tests/flakes/build-paths.sh | 66 +++++++++++++++++++++++++++++++++++++ tests/local.mk | 1 + 3 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 tests/flakes/build-paths.sh diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 97bad5c45..f4486bc2f 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -610,8 +610,38 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() auto attrPath = attr->getAttrPathStr(); - if (!attr->isDerivation()) - throw Error("flake output attribute '%s' is not a derivation", attrPath); + if (!attr->isDerivation()) { + + // FIXME: use eval cache? + auto v = attr->forceValue(); + + if (v.type() == nPath) { + PathSet context; + auto storePath = state->copyPathToStore(context, Path(v.path)); + return {{ + .path = DerivedPath::Opaque { + .path = std::move(storePath), + } + }}; + } + + else if (v.type() == nString) { + PathSet context; + auto s = state->forceString(v, context); + auto storePath = state->store->maybeParseStorePath(s); + if (storePath && context.count(std::string(s))) { + return {{ + .path = DerivedPath::Opaque { + .path = std::move(*storePath), + } + }}; + } else + throw Error("flake output attribute '%s' evaluates to a string that does not denote a store path", attrPath); + } + + else + throw Error("flake output attribute '%s' is not a derivation or path", attrPath); + } auto drvPath = attr->forceDerivation(); diff --git a/tests/flakes/build-paths.sh b/tests/flakes/build-paths.sh new file mode 100644 index 000000000..08b4d1763 --- /dev/null +++ b/tests/flakes/build-paths.sh @@ -0,0 +1,66 @@ +source ./common.sh + +flake1Dir=$TEST_ROOT/flake1 +flake2Dir=$TEST_ROOT/flake2 + +mkdir -p $flake1Dir $flake2Dir + +writeSimpleFlake $flake2Dir +tar cfz $TEST_ROOT/flake.tar.gz -C $TEST_ROOT flake2 +hash=$(nix hash path $flake2Dir) + +dep=$(nix store add-path ./common.sh) + +cat > $flake1Dir/flake.nix < $flake1Dir/foo + +nix build --json --out-link $TEST_ROOT/result $flake1Dir#a1 +[[ -e $TEST_ROOT/result/simple.nix ]] + +nix build --json --out-link $TEST_ROOT/result $flake1Dir#a2 +[[ $(cat $TEST_ROOT/result) = bar ]] + +nix build --json --out-link $TEST_ROOT/result $flake1Dir#a3 + +nix build --json --out-link $TEST_ROOT/result $flake1Dir#a4 + +nix build --json --out-link $TEST_ROOT/result $flake1Dir#a6 +[[ -e $TEST_ROOT/result/simple.nix ]] + +nix build --impure --json --out-link $TEST_ROOT/result $flake1Dir#a8 +diff common.sh $TEST_ROOT/result + +(! nix build --impure --json --out-link $TEST_ROOT/result $flake1Dir#a9) diff --git a/tests/local.mk b/tests/local.mk index 2f7f76261..55913e977 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -9,6 +9,7 @@ nix_tests = \ flakes/check.sh \ flakes/unlocked-override.sh \ flakes/absolute-paths.sh \ + flakes/build-paths.sh \ ca/gc.sh \ gc.sh \ remote-store.sh \ From 49e058f1cfbf315e9bcb664ef884e27eb48cacb5 Mon Sep 17 00:00:00 2001 From: Alexandre Thomas Date: Tue, 3 Jan 2023 21:06:47 +0100 Subject: [PATCH 07/41] Fix Nix installation on older versions of fish The `fish_add_path` function is only available for fish 3.2.0 or newer, and not on older versions. This commit adds an alternative way to update the PATH when `fish_add_path` does not exist. --- scripts/nix-profile-daemon.fish.in | 18 ++++++++++++++++-- scripts/nix-profile.fish.in | 16 +++++++++++++++- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/scripts/nix-profile-daemon.fish.in b/scripts/nix-profile-daemon.fish.in index 3d587dd7f..400696812 100644 --- a/scripts/nix-profile-daemon.fish.in +++ b/scripts/nix-profile-daemon.fish.in @@ -1,3 +1,15 @@ +function add_path --argument-names new_path + if type -q fish_add_path + # fish 3.2.0 or newer + fish_add_path --prepend --global $new_path + else + # older versions of fish + if not contains $new_path $fish_user_paths + set --global fish_user_paths $new_path $fish_user_paths + end + end +end + # Only execute this file once per shell. if test -n "$__ETC_PROFILE_NIX_SOURCED" exit @@ -31,5 +43,7 @@ else end end -fish_add_path --prepend --global "@localstatedir@/nix/profiles/default/bin" -fish_add_path --prepend --global "$HOME/.nix-profile/bin" +add_path "@localstatedir@/nix/profiles/default/bin" +add_path "$HOME/.nix-profile/bin" + +functions -e add_path diff --git a/scripts/nix-profile.fish.in b/scripts/nix-profile.fish.in index 8d783d7c0..731498c76 100644 --- a/scripts/nix-profile.fish.in +++ b/scripts/nix-profile.fish.in @@ -1,3 +1,15 @@ +function add_path --argument-names new_path + if type -q fish_add_path + # fish 3.2.0 or newer + fish_add_path --prepend --global $new_path + else + # older versions of fish + if not contains $new_path $fish_user_paths + set --global fish_user_paths $new_path $fish_user_paths + end + end +end + if test -n "$HOME" && test -n "$USER" # Set up the per-user profile. @@ -32,6 +44,8 @@ if test -n "$HOME" && test -n "$USER" set --export --prepend --path MANPATH "$NIX_LINK/share/man" end - fish_add_path --prepend --global "$NIX_LINK/bin" + add_path "$NIX_LINK/bin" set --erase NIX_LINK end + +functions -e add_path From 05b13aff3d33fe1fae38d092ac9b30976e26dd58 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 6 Jan 2023 23:04:43 -0600 Subject: [PATCH 08/41] Fix typo in example for builtin function map --- src/libexpr/primops.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 9356d307e..9ef91cbc5 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -2807,7 +2807,7 @@ static RegisterPrimOp primop_map({ example, ```nix - map (x"foo" + x) [ "bar" "bla" "abc" ] + map (x: "foo" + x) [ "bar" "bla" "abc" ] ``` evaluates to `[ "foobar" "foobla" "fooabc" ]`. From c83a8174fdfd7e7f7c04584a02a0160403fb047d Mon Sep 17 00:00:00 2001 From: Linus Heckemann Date: Sun, 8 Jan 2023 14:38:34 +0100 Subject: [PATCH 09/41] tests: fix for nixpkgs 22.11 runCommand now uses stdenvNoCC by default, so that needs to be included instead of the regular stdenv. --- tests/containers.nix | 16 ++++++++-------- tests/setuid.nix | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/containers.nix b/tests/containers.nix index f31f22cf6..a4856b2df 100644 --- a/tests/containers.nix +++ b/tests/containers.nix @@ -16,7 +16,7 @@ makeTest ({ { virtualisation.writableStore = true; virtualisation.diskSize = 2048; virtualisation.additionalPaths = - [ pkgs.stdenv + [ pkgs.stdenvNoCC (import ./systemd-nspawn.nix { inherit nixpkgs; }).toplevel ]; virtualisation.memorySize = 4096; @@ -38,30 +38,30 @@ makeTest ({ # Test that 'id' gives the expected result in various configurations. # Existing UIDs, sandbox. - host.succeed("nix build --no-auto-allocate-uids --sandbox -L --offline --impure --file ${./id-test.nix} --argstr name id-test-1") + host.succeed("nix build -v --no-auto-allocate-uids --sandbox -L --offline --impure --file ${./id-test.nix} --argstr name id-test-1") host.succeed("[[ $(cat ./result) = 'uid=1000(nixbld) gid=100(nixbld) groups=100(nixbld)' ]]") # Existing UIDs, no sandbox. - host.succeed("nix build --no-auto-allocate-uids --no-sandbox -L --offline --impure --file ${./id-test.nix} --argstr name id-test-2") + host.succeed("nix build -v --no-auto-allocate-uids --no-sandbox -L --offline --impure --file ${./id-test.nix} --argstr name id-test-2") host.succeed("[[ $(cat ./result) = 'uid=30001(nixbld1) gid=30000(nixbld) groups=30000(nixbld)' ]]") # Auto-allocated UIDs, sandbox. - host.succeed("nix build --auto-allocate-uids --sandbox -L --offline --impure --file ${./id-test.nix} --argstr name id-test-3") + host.succeed("nix build -v --auto-allocate-uids --sandbox -L --offline --impure --file ${./id-test.nix} --argstr name id-test-3") host.succeed("[[ $(cat ./result) = 'uid=1000(nixbld) gid=100(nixbld) groups=100(nixbld)' ]]") # Auto-allocated UIDs, no sandbox. - host.succeed("nix build --auto-allocate-uids --no-sandbox -L --offline --impure --file ${./id-test.nix} --argstr name id-test-4") + host.succeed("nix build -v --auto-allocate-uids --no-sandbox -L --offline --impure --file ${./id-test.nix} --argstr name id-test-4") host.succeed("[[ $(cat ./result) = 'uid=872415232 gid=30000(nixbld) groups=30000(nixbld)' ]]") # Auto-allocated UIDs, UID range, sandbox. - host.succeed("nix build --auto-allocate-uids --sandbox -L --offline --impure --file ${./id-test.nix} --argstr name id-test-5 --arg uidRange true") + host.succeed("nix build -v --auto-allocate-uids --sandbox -L --offline --impure --file ${./id-test.nix} --argstr name id-test-5 --arg uidRange true") host.succeed("[[ $(cat ./result) = 'uid=0(root) gid=0(root) groups=0(root)' ]]") # Auto-allocated UIDs, UID range, no sandbox. - host.fail("nix build --auto-allocate-uids --no-sandbox -L --offline --impure --file ${./id-test.nix} --argstr name id-test-6 --arg uidRange true") + host.fail("nix build -v --auto-allocate-uids --no-sandbox -L --offline --impure --file ${./id-test.nix} --argstr name id-test-6 --arg uidRange true") # Run systemd-nspawn in a Nix build. - host.succeed("nix build --auto-allocate-uids --sandbox -L --offline --impure --file ${./systemd-nspawn.nix} --argstr nixpkgs ${nixpkgs}") + host.succeed("nix build -v --auto-allocate-uids --sandbox -L --offline --impure --file ${./systemd-nspawn.nix} --argstr nixpkgs ${nixpkgs}") host.succeed("[[ $(cat ./result/msg) = 'Hello World' ]]") ''; diff --git a/tests/setuid.nix b/tests/setuid.nix index 82efd6d54..6784615e4 100644 --- a/tests/setuid.nix +++ b/tests/setuid.nix @@ -15,7 +15,7 @@ makeTest { { virtualisation.writableStore = true; nix.settings.substituters = lib.mkForce [ ]; nix.nixPath = [ "nixpkgs=${lib.cleanSource pkgs.path}" ]; - virtualisation.additionalPaths = [ pkgs.stdenv pkgs.pkgsi686Linux.stdenv ]; + virtualisation.additionalPaths = [ pkgs.stdenvNoCC pkgs.pkgsi686Linux.stdenvNoCC ]; }; testScript = { nodes }: '' From 89ef26664d3339a89afa8dfa762326cf196d1622 Mon Sep 17 00:00:00 2001 From: Jeremy Fleischman Date: Mon, 9 Jan 2023 00:49:46 -0800 Subject: [PATCH 10/41] Add a pointer from "realising" to `nix log`. (#4876) --- doc/manual/src/command-ref/nix-store.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/doc/manual/src/command-ref/nix-store.md b/doc/manual/src/command-ref/nix-store.md index acf29e4aa..6d0e02ca5 100644 --- a/doc/manual/src/command-ref/nix-store.md +++ b/doc/manual/src/command-ref/nix-store.md @@ -155,6 +155,12 @@ To test whether a previously-built derivation is deterministic: $ nix-build '' -A hello --check -K ``` +Use [`--read-log`](#operation---read-log) to show the stderr and stdout of a build: + +```console +$ nix-store --read-log $(nix-instantiate ./test.nix) +``` + # Operation `--serve` ## Synopsis From b80e4b57dae78490ed644a2e2a840dae39d1da4e Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 10 Jan 2023 14:52:49 +0100 Subject: [PATCH 11/41] ExtraInfo -> ExtraPathInfo --- src/libcmd/installables.cc | 2 +- src/libcmd/installables.hh | 6 +++--- src/nix/profile.cc | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index f5a436fbe..280212379 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -898,7 +898,7 @@ std::vector, BuiltPathWithResult>> Instal struct Aux { - ExtraInfo info; + ExtraPathInfo info; std::shared_ptr installable; }; diff --git a/src/libcmd/installables.hh b/src/libcmd/installables.hh index 95ab4a40e..250cf7e83 100644 --- a/src/libcmd/installables.hh +++ b/src/libcmd/installables.hh @@ -52,7 +52,7 @@ enum class OperateOn { Derivation }; -struct ExtraInfo +struct ExtraPathInfo { std::optional priority; std::optional originalRef; @@ -67,13 +67,13 @@ struct ExtraInfo struct DerivedPathWithInfo { DerivedPath path; - ExtraInfo info; + ExtraPathInfo info; }; struct BuiltPathWithResult { BuiltPath path; - ExtraInfo info; + ExtraPathInfo info; std::optional result; }; diff --git a/src/nix/profile.cc b/src/nix/profile.cc index db702db1b..3da47568c 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -253,11 +253,11 @@ struct ProfileManifest } }; -static std::map> +static std::map> builtPathsPerInstallable( const std::vector, BuiltPathWithResult>> & builtPaths) { - std::map> res; + std::map> res; for (auto & [installable, builtPath] : builtPaths) { auto & r = res[installable.get()]; /* Note that there could be conflicting info From b4dc68f0be07edc3511a4ef276de1cb4e13a676d Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 10 Jan 2023 14:56:03 +0100 Subject: [PATCH 12/41] Show string in error message --- src/libcmd/installables.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 280212379..d7fdbb13d 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -636,7 +636,7 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() } }}; } else - throw Error("flake output attribute '%s' evaluates to a string that does not denote a store path", attrPath); + throw Error("flake output attribute '%s' evaluates to the string '%s' which is not a store path", attrPath, s); } else From 1123c42f9016c5df7ade9d917d5e1900af6688e8 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 10 Jan 2023 14:57:35 +0100 Subject: [PATCH 13/41] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com> --- src/libcmd/installables.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index d7fdbb13d..409afd762 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -424,7 +424,7 @@ struct InstallableStorePath : Installable DerivedPathsWithInfo toDerivedPaths() override { - return {{req}}; + return {{.path = req, .info = {} }}; } std::optional getStorePath() override From 7f1af270ddffa1cd6d18c167bcebe56b93bfd127 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 10 Jan 2023 15:08:46 +0100 Subject: [PATCH 14/41] Clean up toDerivedPaths() logic --- src/libcmd/installables.cc | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 409afd762..c0db2a715 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -478,11 +478,9 @@ struct InstallableAttrPath : InstallableValue DrvInfos drvInfos; getDerivations(*state, *v, "", autoArgs, drvInfos, false); - DerivedPathsWithInfo res; - // Backward compatibility hack: group results by drvPath. This // helps keep .all output together. - std::map byDrvPath; + std::map byDrvPath; for (auto & drvInfo : drvInfos) { auto drvPath = drvInfo.queryDrvPath(); @@ -497,21 +495,16 @@ struct InstallableAttrPath : InstallableValue for (auto & output : drvInfo.queryOutputs(false, std::get_if(&outputsSpec))) outputsToInstall.insert(output.first); - auto i = byDrvPath.find(*drvPath); - if (i == byDrvPath.end()) { - byDrvPath[*drvPath] = res.size(); - res.push_back({ - .path = DerivedPath::Built { - .drvPath = std::move(*drvPath), - .outputs = std::move(outputsToInstall), - } - }); - } else { - for (auto & output : outputsToInstall) - std::get(res[i->second].path).outputs.insert(output); - } + auto derivedPath = byDrvPath.emplace(*drvPath, DerivedPath::Built { .drvPath = *drvPath }).first; + + for (auto & output : outputsToInstall) + derivedPath->second.outputs.insert(output); } + DerivedPathsWithInfo res; + for (auto & [_, info] : byDrvPath) + res.push_back({ .path = { info } }); + return res; } }; From 59cc920cc0751f93d4a3b717da72505a7bab2ee7 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 10 Jan 2023 15:20:30 +0100 Subject: [PATCH 15/41] Add a FIXME --- src/nix/profile.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/nix/profile.cc b/src/nix/profile.cc index 3da47568c..22ee51ab9 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -262,7 +262,8 @@ builtPathsPerInstallable( auto & r = res[installable.get()]; /* Note that there could be conflicting info (e.g. meta.priority fields) if the installable returned - multiple derivations. So pick one arbitrarily. */ + multiple derivations. So pick one arbitrarily. FIXME: + print a warning? */ r.first.push_back(builtPath.path); r.second = builtPath.info; } From da64f026dd7b12d72ffbc15752e8b95707fa1f9f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 10 Jan 2023 11:27:19 -0500 Subject: [PATCH 16/41] Make clear that `StorePathWithOutputs` is a deprecated type - Add a comment - Put `OutputsSpec` in a different header (First part of #6815) - Make a few stray uses of it in new code use `DerivedPath` instead. --- src/libcmd/installables.hh | 4 +- src/libexpr/flake/flakeref.hh | 2 +- src/libmain/shared.hh | 1 - src/libstore/outputs-spec.cc | 61 +++++++++++++++++++ src/libstore/outputs-spec.hh | 32 ++++++++++ src/libstore/path-with-outputs.cc | 54 ---------------- src/libstore/path-with-outputs.hh | 30 +++------ .../{path-with-outputs.cc => outputs-spec.cc} | 2 +- src/nix/app.cc | 23 +++++-- src/nix/develop.cc | 2 +- src/nix/flake.cc | 2 +- 11 files changed, 124 insertions(+), 89 deletions(-) create mode 100644 src/libstore/outputs-spec.cc create mode 100644 src/libstore/outputs-spec.hh rename src/libstore/tests/{path-with-outputs.cc => outputs-spec.cc} (97%) diff --git a/src/libcmd/installables.hh b/src/libcmd/installables.hh index 250cf7e83..9b92cc4be 100644 --- a/src/libcmd/installables.hh +++ b/src/libcmd/installables.hh @@ -2,7 +2,7 @@ #include "util.hh" #include "path.hh" -#include "path-with-outputs.hh" +#include "outputs-spec.hh" #include "derived-path.hh" #include "eval.hh" #include "store-api.hh" @@ -20,7 +20,7 @@ namespace eval_cache { class EvalCache; class AttrCursor; } struct App { - std::vector context; + std::vector context; Path program; // FIXME: add args, sandbox settings, metadata, ... }; diff --git a/src/libexpr/flake/flakeref.hh b/src/libexpr/flake/flakeref.hh index a36d852a8..4ec79fb73 100644 --- a/src/libexpr/flake/flakeref.hh +++ b/src/libexpr/flake/flakeref.hh @@ -3,7 +3,7 @@ #include "types.hh" #include "hash.hh" #include "fetchers.hh" -#include "path-with-outputs.hh" +#include "outputs-spec.hh" #include diff --git a/src/libmain/shared.hh b/src/libmain/shared.hh index 3c37fd627..1715374a6 100644 --- a/src/libmain/shared.hh +++ b/src/libmain/shared.hh @@ -39,7 +39,6 @@ void printVersion(const std::string & programName); void printGCWarning(); class Store; -struct StorePathWithOutputs; void printMissing( ref store, diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc new file mode 100644 index 000000000..76779d193 --- /dev/null +++ b/src/libstore/outputs-spec.cc @@ -0,0 +1,61 @@ +#include "outputs-spec.hh" +#include "nlohmann/json.hpp" + +#include + +namespace nix { + +std::pair parseOutputsSpec(const std::string & s) +{ + static std::regex regex(R"((.*)\^((\*)|([a-z]+(,[a-z]+)*)))"); + + std::smatch match; + if (!std::regex_match(s, match, regex)) + return {s, DefaultOutputs()}; + + if (match[3].matched) + return {match[1], AllOutputs()}; + + return {match[1], tokenizeString(match[4].str(), ",")}; +} + +std::string printOutputsSpec(const OutputsSpec & outputsSpec) +{ + if (std::get_if(&outputsSpec)) + return ""; + + if (std::get_if(&outputsSpec)) + return "^*"; + + if (auto outputNames = std::get_if(&outputsSpec)) + return "^" + concatStringsSep(",", *outputNames); + + assert(false); +} + +void to_json(nlohmann::json & json, const OutputsSpec & outputsSpec) +{ + if (std::get_if(&outputsSpec)) + json = nullptr; + + else if (std::get_if(&outputsSpec)) + json = std::vector({"*"}); + + else if (auto outputNames = std::get_if(&outputsSpec)) + json = *outputNames; +} + +void from_json(const nlohmann::json & json, OutputsSpec & outputsSpec) +{ + if (json.is_null()) + outputsSpec = DefaultOutputs(); + else { + auto names = json.get(); + if (names == OutputNames({"*"})) + outputsSpec = AllOutputs(); + else + outputsSpec = names; + } +} + +} diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh new file mode 100644 index 000000000..e2cf1d12b --- /dev/null +++ b/src/libstore/outputs-spec.hh @@ -0,0 +1,32 @@ +#pragma once + +#include + +#include "util.hh" + +#include "nlohmann/json_fwd.hpp" + +namespace nix { + +typedef std::set OutputNames; + +struct AllOutputs { + bool operator < (const AllOutputs & _) const { return false; } +}; + +struct DefaultOutputs { + bool operator < (const DefaultOutputs & _) const { return false; } +}; + +typedef std::variant OutputsSpec; + +/* Parse a string of the form 'prefix^output1,...outputN' or + 'prefix^*', returning the prefix and the outputs spec. */ +std::pair parseOutputsSpec(const std::string & s); + +std::string printOutputsSpec(const OutputsSpec & outputsSpec); + +void to_json(nlohmann::json &, const OutputsSpec &); +void from_json(const nlohmann::json &, OutputsSpec &); + +} diff --git a/src/libstore/path-with-outputs.cc b/src/libstore/path-with-outputs.cc index d6d67ea05..17230ffc4 100644 --- a/src/libstore/path-with-outputs.cc +++ b/src/libstore/path-with-outputs.cc @@ -1,6 +1,5 @@ #include "path-with-outputs.hh" #include "store-api.hh" -#include "nlohmann/json.hpp" #include @@ -71,57 +70,4 @@ StorePathWithOutputs followLinksToStorePathWithOutputs(const Store & store, std: return StorePathWithOutputs { store.followLinksToStorePath(path), std::move(outputs) }; } -std::pair parseOutputsSpec(const std::string & s) -{ - static std::regex regex(R"((.*)\^((\*)|([a-z]+(,[a-z]+)*)))"); - - std::smatch match; - if (!std::regex_match(s, match, regex)) - return {s, DefaultOutputs()}; - - if (match[3].matched) - return {match[1], AllOutputs()}; - - return {match[1], tokenizeString(match[4].str(), ",")}; -} - -std::string printOutputsSpec(const OutputsSpec & outputsSpec) -{ - if (std::get_if(&outputsSpec)) - return ""; - - if (std::get_if(&outputsSpec)) - return "^*"; - - if (auto outputNames = std::get_if(&outputsSpec)) - return "^" + concatStringsSep(",", *outputNames); - - assert(false); -} - -void to_json(nlohmann::json & json, const OutputsSpec & outputsSpec) -{ - if (std::get_if(&outputsSpec)) - json = nullptr; - - else if (std::get_if(&outputsSpec)) - json = std::vector({"*"}); - - else if (auto outputNames = std::get_if(&outputsSpec)) - json = *outputNames; -} - -void from_json(const nlohmann::json & json, OutputsSpec & outputsSpec) -{ - if (json.is_null()) - outputsSpec = DefaultOutputs(); - else { - auto names = json.get(); - if (names == OutputNames({"*"})) - outputsSpec = AllOutputs(); - else - outputsSpec = names; - } -} - } diff --git a/src/libstore/path-with-outputs.hh b/src/libstore/path-with-outputs.hh index 0cb5eb223..ed55cc333 100644 --- a/src/libstore/path-with-outputs.hh +++ b/src/libstore/path-with-outputs.hh @@ -1,13 +1,18 @@ #pragma once -#include - #include "path.hh" #include "derived-path.hh" #include "nlohmann/json_fwd.hpp" namespace nix { +/* This is a deprecated old type just for use by the old CLI, and older + versions of the RPC protocols. In new code don't use it; you want + `DerivedPath` instead. + + `DerivedPath` is better because it handles more cases, and does so more + explicitly without devious punning tricks. +*/ struct StorePathWithOutputs { StorePath path; @@ -33,25 +38,4 @@ StorePathWithOutputs parsePathWithOutputs(const Store & store, std::string_view StorePathWithOutputs followLinksToStorePathWithOutputs(const Store & store, std::string_view pathWithOutputs); -typedef std::set OutputNames; - -struct AllOutputs { - bool operator < (const AllOutputs & _) const { return false; } -}; - -struct DefaultOutputs { - bool operator < (const DefaultOutputs & _) const { return false; } -}; - -typedef std::variant OutputsSpec; - -/* Parse a string of the form 'prefix^output1,...outputN' or - 'prefix^*', returning the prefix and the outputs spec. */ -std::pair parseOutputsSpec(const std::string & s); - -std::string printOutputsSpec(const OutputsSpec & outputsSpec); - -void to_json(nlohmann::json &, const OutputsSpec &); -void from_json(const nlohmann::json &, OutputsSpec &); - } diff --git a/src/libstore/tests/path-with-outputs.cc b/src/libstore/tests/outputs-spec.cc similarity index 97% rename from src/libstore/tests/path-with-outputs.cc rename to src/libstore/tests/outputs-spec.cc index 350ea7ffd..d781a930e 100644 --- a/src/libstore/tests/path-with-outputs.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -1,4 +1,4 @@ -#include "path-with-outputs.hh" +#include "outputs-spec.hh" #include diff --git a/src/nix/app.cc b/src/nix/app.cc index a8d7e115b..fb149042c 100644 --- a/src/nix/app.cc +++ b/src/nix/app.cc @@ -79,9 +79,19 @@ UnresolvedApp Installable::toApp(EvalState & state) if (type == "app") { auto [program, context] = cursor->getAttr("program")->getStringWithContext(); - std::vector context2; - for (auto & [path, name] : context) - context2.push_back({path, {name}}); + std::vector context2; + for (auto & [path, name] : context) { + context2.push_back(name != "" || path.isDerivation() + ? (DerivedPath) DerivedPath::Built { + .drvPath = path, + .outputs = name != "" + ? StringSet { name } + : StringSet { }, + } + : (DerivedPath) DerivedPath::Opaque { + .path = path, + }); + } return UnresolvedApp{App { .context = std::move(context2), @@ -105,7 +115,10 @@ UnresolvedApp Installable::toApp(EvalState & state) : DrvName(name).name; auto program = outPath + "/bin/" + mainProgram; return UnresolvedApp { App { - .context = { { drvPath, {outputName} } }, + .context = { DerivedPath::Built { + .drvPath = drvPath, + .outputs = {outputName}, + } }, .program = program, }}; } @@ -123,7 +136,7 @@ App UnresolvedApp::resolve(ref evalStore, ref store) for (auto & ctxElt : unresolved.context) installableContext.push_back( - std::make_shared(store, ctxElt.toDerivedPath())); + std::make_shared(store, ctxElt)); auto builtContext = Installable::build(evalStore, store, Realise::Outputs, installableContext); res.program = resolveString(*store, unresolved.program, builtContext); diff --git a/src/nix/develop.cc b/src/nix/develop.cc index 1d90d1dac..6aa675386 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -3,7 +3,7 @@ #include "common-args.hh" #include "shared.hh" #include "store-api.hh" -#include "path-with-outputs.hh" +#include "outputs-spec.hh" #include "derivations.hh" #include "progress-bar.hh" #include "run.hh" diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 9b4cdf35a..bb020d51e 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -7,7 +7,7 @@ #include "get-drvs.hh" #include "store-api.hh" #include "derivations.hh" -#include "path-with-outputs.hh" +#include "outputs-spec.hh" #include "attr-path.hh" #include "fetchers.hh" #include "registry.hh" From 5576d5e987e907bf13ae6c7fe79ececce4e86e2d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 3 Jan 2023 11:44:59 -0500 Subject: [PATCH 17/41] Parse string context elements properly Prior to this change, we had a bunch of ad-hoc string manipulation code scattered around. This made it hard to figure out what data model for string contexts is. Now, we still store string contexts most of the time as encoded strings --- I was wary of the performance implications of changing that --- but whenever we parse them we do so only through the `NixStringContextElem::parse` method, which handles all cases. This creates a data type that is very similar to `DerivedPath` but: - Represents the funky `=` case as properly distinct from the others. - Only encodes a single output, no wildcards and no set, for the "built" case. (I would like to deprecate `=`, after which we are in spitting distance of `DerivedPath` and could maybe get away with fewer types, but that is another topic for another day.) --- src/libexpr/eval-cache.cc | 15 ++++- src/libexpr/eval.cc | 23 +------- src/libexpr/eval.hh | 4 -- src/libexpr/local.mk | 3 + src/libexpr/primops.cc | 90 +++++++++++++++++------------- src/libexpr/primops/context.cc | 51 ++++++++--------- src/libexpr/tests/local.mk | 4 +- src/libexpr/tests/value/context.cc | 72 ++++++++++++++++++++++++ src/libexpr/value.hh | 3 +- src/libexpr/value/context.cc | 67 ++++++++++++++++++++++ src/libexpr/value/context.hh | 90 ++++++++++++++++++++++++++++++ src/nix/app.cc | 32 +++++++---- src/nix/flake.cc | 2 +- tests/plugins/local.mk | 2 +- 14 files changed, 347 insertions(+), 111 deletions(-) create mode 100644 src/libexpr/tests/value/context.cc create mode 100644 src/libexpr/value/context.cc create mode 100644 src/libexpr/value/context.hh diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index afe575fee..1219b2471 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -300,7 +300,7 @@ struct AttrDb NixStringContext context; if (!queryAttribute.isNull(3)) for (auto & s : tokenizeString>(queryAttribute.getStr(3), ";")) - context.push_back(decodeContext(cfg, s)); + context.push_back(NixStringContextElem::parse(cfg, s)); return {{rowId, string_t{queryAttribute.getStr(2), context}}}; } case AttrType::Bool: @@ -592,7 +592,18 @@ string_t AttrCursor::getStringWithContext() if (auto s = std::get_if(&cachedValue->second)) { bool valid = true; for (auto & c : s->second) { - if (!root->state.store->isValidPath(c.first)) { + const StorePath & path = std::visit(overloaded { + [&](const NixStringContextElem::DrvDeep & d) -> const StorePath & { + return d.drvPath; + }, + [&](const NixStringContextElem::Built & b) -> const StorePath & { + return b.drvPath; + }, + [&](const NixStringContextElem::Opaque & o) -> const StorePath & { + return o.path; + }, + }, c.raw()); + if (!root->state.store->isValidPath(path)) { valid = false; break; } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 978b0f0e2..277cbb5f9 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -2068,27 +2068,6 @@ std::string_view EvalState::forceString(Value & v, const PosIdx pos, std::string } -/* Decode a context string ‘!!’ into a pair . */ -NixStringContextElem decodeContext(const Store & store, std::string_view s) -{ - if (s.at(0) == '!') { - size_t index = s.find("!", 1); - return { - store.parseStorePath(s.substr(index + 1)), - std::string(s.substr(1, index - 1)), - }; - } else - return { - store.parseStorePath( - s.at(0) == '/' - ? s - : s.substr(1)), - "", - }; -} - - void copyContext(const Value & v, PathSet & context) { if (v.string.context) @@ -2103,7 +2082,7 @@ NixStringContext Value::getContext(const Store & store) assert(internalType == tString); if (string.context) for (const char * * p = string.context; *p; ++p) - res.push_back(decodeContext(store, *p)); + res.push_back(NixStringContextElem::parse(store, *p)); return res; } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 4e0c4db95..46b8cbaa5 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -551,10 +551,6 @@ struct DebugTraceStacker { std::string_view showType(ValueType type); std::string showType(const Value & v); -/* Decode a context string ‘!!’ into a pair . */ -NixStringContextElem decodeContext(const Store & store, std::string_view s); - /* If `path' refers to a directory, then append "/default.nix". */ Path resolveExprPath(Path path); diff --git a/src/libexpr/local.mk b/src/libexpr/local.mk index 016631647..2171e769b 100644 --- a/src/libexpr/local.mk +++ b/src/libexpr/local.mk @@ -6,6 +6,7 @@ libexpr_DIR := $(d) libexpr_SOURCES := \ $(wildcard $(d)/*.cc) \ + $(wildcard $(d)/value/*.cc) \ $(wildcard $(d)/primops/*.cc) \ $(wildcard $(d)/flake/*.cc) \ $(d)/lexer-tab.cc \ @@ -37,6 +38,8 @@ clean-files += $(d)/parser-tab.cc $(d)/parser-tab.hh $(d)/lexer-tab.cc $(d)/lexe $(eval $(call install-file-in, $(d)/nix-expr.pc, $(libdir)/pkgconfig, 0644)) +$(foreach i, $(wildcard src/libexpr/value/*.hh), \ + $(eval $(call install-file-in, $(i), $(includedir)/nix/value, 0644))) $(foreach i, $(wildcard src/libexpr/flake/*.hh), \ $(eval $(call install-file-in, $(i), $(includedir)/nix/flake, 0644))) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 9ef91cbc5..a433e99f0 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -43,16 +43,32 @@ StringMap EvalState::realiseContext(const PathSet & context) std::vector drvs; StringMap res; - for (auto & i : context) { - auto [ctx, outputName] = decodeContext(*store, i); - auto ctxS = store->printStorePath(ctx); - if (!store->isValidPath(ctx)) - debugThrowLastTrace(InvalidPathError(store->printStorePath(ctx))); - if (!outputName.empty() && ctx.isDerivation()) { - drvs.push_back({ctx, {outputName}}); - } else { - res.insert_or_assign(ctxS, ctxS); - } + for (auto & c_ : context) { + auto ensureValid = [&](const StorePath & p) { + if (!store->isValidPath(p)) + debugThrowLastTrace(InvalidPathError(store->printStorePath(p))); + }; + auto c = NixStringContextElem::parse(*store, c_); + std::visit(overloaded { + [&](const NixStringContextElem::Built & b) { + drvs.push_back(DerivedPath::Built { + .drvPath = b.drvPath, + .outputs = std::set { b.output }, + }); + ensureValid(b.drvPath); + }, + [&](const NixStringContextElem::Opaque & o) { + auto ctxS = store->printStorePath(o.path); + res.insert_or_assign(ctxS, ctxS); + ensureValid(o.path); + }, + [&](const NixStringContextElem::DrvDeep & d) { + /* Treat same as Opaque */ + auto ctxS = store->printStorePath(d.drvPath); + res.insert_or_assign(ctxS, ctxS); + ensureValid(d.drvPath); + }, + }, c.raw()); } if (drvs.empty()) return {}; @@ -1179,35 +1195,31 @@ static void prim_derivationStrict(EvalState & state, const PosIdx pos, Value * * /* Everything in the context of the strings in the derivation attributes should be added as dependencies of the resulting derivation. */ - for (auto & path : context) { - - /* Paths marked with `=' denote that the path of a derivation - is explicitly passed to the builder. Since that allows the - builder to gain access to every path in the dependency - graph of the derivation (including all outputs), all paths - in the graph must be added to this derivation's list of - inputs to ensure that they are available when the builder - runs. */ - if (path.at(0) == '=') { - /* !!! This doesn't work if readOnlyMode is set. */ - StorePathSet refs; - state.store->computeFSClosure(state.store->parseStorePath(std::string_view(path).substr(1)), refs); - for (auto & j : refs) { - drv.inputSrcs.insert(j); - if (j.isDerivation()) - drv.inputDrvs[j] = state.store->readDerivation(j).outputNames(); - } - } - - /* Handle derivation outputs of the form ‘!!’. */ - else if (path.at(0) == '!') { - auto ctx = decodeContext(*state.store, path); - drv.inputDrvs[ctx.first].insert(ctx.second); - } - - /* Otherwise it's a source file. */ - else - drv.inputSrcs.insert(state.store->parseStorePath(path)); + for (auto & c_ : context) { + auto c = NixStringContextElem::parse(*state.store, c_); + std::visit(overloaded { + /* Since this allows the builder to gain access to every + path in the dependency graph of the derivation (including + all outputs), all paths in the graph must be added to + this derivation's list of inputs to ensure that they are + available when the builder runs. */ + [&](const NixStringContextElem::DrvDeep & d) { + /* !!! This doesn't work if readOnlyMode is set. */ + StorePathSet refs; + state.store->computeFSClosure(d.drvPath, refs); + for (auto & j : refs) { + drv.inputSrcs.insert(j); + if (j.isDerivation()) + drv.inputDrvs[j] = state.store->readDerivation(j).outputNames(); + } + }, + [&](const NixStringContextElem::Built & b) { + drv.inputDrvs[b.drvPath].insert(b.output); + }, + [&](const NixStringContextElem::Opaque & o) { + drv.inputSrcs.insert(o.path); + }, + }, c.raw()); } /* Do we have all required attributes? */ diff --git a/src/libexpr/primops/context.cc b/src/libexpr/primops/context.cc index 9fae0b14d..0c65a6b98 100644 --- a/src/libexpr/primops/context.cc +++ b/src/libexpr/primops/context.cc @@ -37,8 +37,15 @@ static void prim_unsafeDiscardOutputDependency(EvalState & state, const PosIdx p auto s = state.coerceToString(pos, *args[0], context, "while evaluating the argument passed to builtins.unsafeDiscardOutputDependency"); PathSet context2; - for (auto & p : context) - context2.insert(p.at(0) == '=' ? std::string(p, 1) : p); + for (auto && p : context) { + auto c = NixStringContextElem::parse(*state.store, p); + if (auto * ptr = std::get_if(&c)) { + context2.emplace(state.store->printStorePath(ptr->drvPath)); + } else { + /* Can reuse original item */ + context2.emplace(std::move(p)); + } + } v.mkString(*s, context2); } @@ -74,34 +81,22 @@ static void prim_getContext(EvalState & state, const PosIdx pos, Value * * args, }; PathSet context; state.forceString(*args[0], context, pos, "while evaluating the argument passed to builtins.getContext"); - auto contextInfos = std::map(); + auto contextInfos = std::map(); for (const auto & p : context) { Path drv; std::string output; - const Path * path = &p; - if (p.at(0) == '=') { - drv = std::string(p, 1); - path = &drv; - } else if (p.at(0) == '!') { - NixStringContextElem ctx = decodeContext(*state.store, p); - drv = state.store->printStorePath(ctx.first); - output = ctx.second; - path = &drv; - } - auto isPath = drv.empty(); - auto isAllOutputs = (!drv.empty()) && output.empty(); - - auto iter = contextInfos.find(*path); - if (iter == contextInfos.end()) { - contextInfos.emplace(*path, ContextInfo{isPath, isAllOutputs, output.empty() ? Strings{} : Strings{std::move(output)}}); - } else { - if (isPath) - iter->second.path = true; - else if (isAllOutputs) - iter->second.allOutputs = true; - else - iter->second.outputs.emplace_back(std::move(output)); - } + NixStringContextElem ctx = NixStringContextElem::parse(*state.store, p); + std::visit(overloaded { + [&](NixStringContextElem::DrvDeep & d) { + contextInfos[d.drvPath].allOutputs = true; + }, + [&](NixStringContextElem::Built & b) { + contextInfos[b.drvPath].outputs.emplace_back(std::move(output)); + }, + [&](NixStringContextElem::Opaque & o) { + contextInfos[o.path].path = true; + }, + }, ctx.raw()); } auto attrs = state.buildBindings(contextInfos.size()); @@ -120,7 +115,7 @@ static void prim_getContext(EvalState & state, const PosIdx pos, Value * * args, for (const auto & [i, output] : enumerate(info.second.outputs)) (outputsVal.listElems()[i] = state.allocValue())->mkString(output); } - attrs.alloc(info.first).mkAttrs(infoAttrs); + attrs.alloc(state.store->printStorePath(info.first)).mkAttrs(infoAttrs); } v.mkAttrs(attrs); diff --git a/src/libexpr/tests/local.mk b/src/libexpr/tests/local.mk index b95980cab..e483575a4 100644 --- a/src/libexpr/tests/local.mk +++ b/src/libexpr/tests/local.mk @@ -6,7 +6,9 @@ libexpr-tests_DIR := $(d) libexpr-tests_INSTALL_DIR := -libexpr-tests_SOURCES := $(wildcard $(d)/*.cc) +libexpr-tests_SOURCES := \ + $(wildcard $(d)/*.cc) \ + $(wildcard $(d)/value/*.cc) libexpr-tests_CXXFLAGS += -I src/libexpr -I src/libutil -I src/libstore -I src/libexpr/tests diff --git a/src/libexpr/tests/value/context.cc b/src/libexpr/tests/value/context.cc new file mode 100644 index 000000000..d5c9d3bce --- /dev/null +++ b/src/libexpr/tests/value/context.cc @@ -0,0 +1,72 @@ +#include "value/context.hh" + +#include "libexprtests.hh" + +namespace nix { + +// Testing of trivial expressions +struct NixStringContextElemTest : public LibExprTest { + const Store & store() const { + return *LibExprTest::store; + } +}; + +TEST_F(NixStringContextElemTest, empty_invalid) { + EXPECT_THROW( + NixStringContextElem::parse(store(), ""), + BadNixStringContextElem); +} + +TEST_F(NixStringContextElemTest, single_bang_invalid) { + EXPECT_THROW( + NixStringContextElem::parse(store(), "!"), + BadNixStringContextElem); +} + +TEST_F(NixStringContextElemTest, double_bang_invalid) { + EXPECT_THROW( + NixStringContextElem::parse(store(), "!!/"), + BadStorePath); +} + +TEST_F(NixStringContextElemTest, eq_slash_invalid) { + EXPECT_THROW( + NixStringContextElem::parse(store(), "=/"), + BadStorePath); +} + +TEST_F(NixStringContextElemTest, slash_invalid) { + EXPECT_THROW( + NixStringContextElem::parse(store(), "/"), + BadStorePath); +} + +TEST_F(NixStringContextElemTest, opaque) { + std::string_view opaque = "/nix/store/g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x"; + auto elem = NixStringContextElem::parse(store(), opaque); + auto * p = std::get_if(&elem); + ASSERT_TRUE(p); + ASSERT_EQ(p->path, store().parseStorePath(opaque)); + ASSERT_EQ(elem.to_string(store()), opaque); +} + +TEST_F(NixStringContextElemTest, drvDeep) { + std::string_view drvDeep = "=/nix/store/g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x.drv"; + auto elem = NixStringContextElem::parse(store(), drvDeep); + auto * p = std::get_if(&elem); + ASSERT_TRUE(p); + ASSERT_EQ(p->drvPath, store().parseStorePath(drvDeep.substr(1))); + ASSERT_EQ(elem.to_string(store()), drvDeep); +} + +TEST_F(NixStringContextElemTest, built) { + std::string_view built = "!foo!/nix/store/g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x.drv"; + auto elem = NixStringContextElem::parse(store(), built); + auto * p = std::get_if(&elem); + ASSERT_TRUE(p); + ASSERT_EQ(p->output, "foo"); + ASSERT_EQ(p->drvPath, store().parseStorePath(built.substr(5))); + ASSERT_EQ(elem.to_string(store()), built); +} + +} diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index f57597cff..7d3f6d700 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -3,6 +3,7 @@ #include #include "symbol-table.hh" +#include "value/context.hh" #if HAVE_BOEHMGC #include @@ -67,8 +68,6 @@ class XMLWriter; typedef int64_t NixInt; typedef double NixFloat; -typedef std::pair NixStringContextElem; -typedef std::vector NixStringContext; /* External values must descend from ExternalValueBase, so that * type-agnostic nix functions (e.g. showType) can be implemented diff --git a/src/libexpr/value/context.cc b/src/libexpr/value/context.cc new file mode 100644 index 000000000..61d9c53df --- /dev/null +++ b/src/libexpr/value/context.cc @@ -0,0 +1,67 @@ +#include "value/context.hh" +#include "store-api.hh" + +#include + +namespace nix { + +NixStringContextElem NixStringContextElem::parse(const Store & store, std::string_view s0) +{ + std::string_view s = s0; + + if (s.size() == 0) { + throw BadNixStringContextElem(s0, + "String context element should never be an empty string"); + } + switch (s.at(0)) { + case '!': { + s = s.substr(1); // advance string to parse after first ! + size_t index = s.find("!"); + // This makes index + 1 safe. Index can be the length (one after index + // of last character), so given any valid character index --- a + // successful find --- we can add one. + if (index == std::string_view::npos) { + throw BadNixStringContextElem(s0, + "String content element beginning with '!' should have a second '!'"); + } + return NixStringContextElem::Built { + .drvPath = store.parseStorePath(s.substr(index + 1)), + .output = std::string(s.substr(0, index)), + }; + } + case '=': { + return NixStringContextElem::DrvDeep { + .drvPath = store.parseStorePath(s.substr(1)), + }; + } + default: { + return NixStringContextElem::Opaque { + .path = store.parseStorePath(s), + }; + } + } +} + +std::string NixStringContextElem::to_string(const Store & store) const { + return std::visit(overloaded { + [&](const NixStringContextElem::Built & b) { + std::string res; + res += '!'; + res += b.output; + res += '!'; + res += store.printStorePath(b.drvPath); + return res; + }, + [&](const NixStringContextElem::DrvDeep & d) { + std::string res; + res += '='; + res += store.printStorePath(d.drvPath); + return res; + }, + [&](const NixStringContextElem::Opaque & o) { + return store.printStorePath(o.path); + }, + }, raw()); +} + +} diff --git a/src/libexpr/value/context.hh b/src/libexpr/value/context.hh new file mode 100644 index 000000000..d8008c436 --- /dev/null +++ b/src/libexpr/value/context.hh @@ -0,0 +1,90 @@ +#pragma once + +#include "util.hh" +#include "path.hh" + +#include + +#include + +namespace nix { + +class BadNixStringContextElem : public Error +{ +public: + std::string_view raw; + + template + BadNixStringContextElem(std::string_view raw_, const Args & ... args) + : Error("") + { + raw = raw_; + auto hf = hintfmt(args...); + err.msg = hintfmt("Bad String Context element: %1%: %2%", normaltxt(hf.str()), raw); + } +}; + +class Store; + +/* Plain opaque path to some store object. + + Encoded as just the path: ‘’. +*/ +struct NixStringContextElem_Opaque { + StorePath path; +}; + +/* Path to a derivation and its entire build closure. + + The path doesn't just refer to derivation itself and its closure, but + also all outputs of all derivations in that closure (including the + root derivation). + + Encoded in the form ‘=’. +*/ +struct NixStringContextElem_DrvDeep { + StorePath drvPath; +}; + +/* Derivation output. + + Encoded in the form ‘!!’. +*/ +struct NixStringContextElem_Built { + StorePath drvPath; + std::string output; +}; + +using _NixStringContextElem_Raw = std::variant< + NixStringContextElem_Opaque, + NixStringContextElem_DrvDeep, + NixStringContextElem_Built +>; + +struct NixStringContextElem : _NixStringContextElem_Raw { + using Raw = _NixStringContextElem_Raw; + using Raw::Raw; + + using Opaque = NixStringContextElem_Opaque; + using DrvDeep = NixStringContextElem_DrvDeep; + using Built = NixStringContextElem_Built; + + inline const Raw & raw() const { + return static_cast(*this); + } + inline Raw & raw() { + return static_cast(*this); + } + + /* Decode a context string, one of: + - ‘’ + - ‘=’ + - ‘!!’ + */ + static NixStringContextElem parse(const Store & store, std::string_view s); + std::string to_string(const Store & store) const; +}; + +typedef std::vector NixStringContext; + +} diff --git a/src/nix/app.cc b/src/nix/app.cc index fb149042c..c9637dcf5 100644 --- a/src/nix/app.cc +++ b/src/nix/app.cc @@ -80,17 +80,27 @@ UnresolvedApp Installable::toApp(EvalState & state) auto [program, context] = cursor->getAttr("program")->getStringWithContext(); std::vector context2; - for (auto & [path, name] : context) { - context2.push_back(name != "" || path.isDerivation() - ? (DerivedPath) DerivedPath::Built { - .drvPath = path, - .outputs = name != "" - ? StringSet { name } - : StringSet { }, - } - : (DerivedPath) DerivedPath::Opaque { - .path = path, - }); + for (auto & c : context) { + context2.emplace_back(std::visit(overloaded { + [&](const NixStringContextElem::DrvDeep & d) -> DerivedPath { + /* We want all outputs of the drv */ + return DerivedPath::Built { + .drvPath = d.drvPath, + .outputs = {}, + }; + }, + [&](const NixStringContextElem::Built & b) -> DerivedPath { + return DerivedPath::Built { + .drvPath = b.drvPath, + .outputs = { b.output }, + }; + }, + [&](const NixStringContextElem::Opaque & o) -> DerivedPath { + return DerivedPath::Opaque { + .path = o.path, + }; + }, + }, c.raw())); } return UnresolvedApp{App { diff --git a/src/nix/flake.cc b/src/nix/flake.cc index bb020d51e..33ce3f401 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -348,7 +348,7 @@ struct CmdFlakeCheck : FlakeCommand // FIXME auto app = App(*state, v); for (auto & i : app.context) { - auto [drvPathS, outputName] = decodeContext(i); + auto [drvPathS, outputName] = NixStringContextElem::parse(i); store->parseStorePath(drvPathS); } #endif diff --git a/tests/plugins/local.mk b/tests/plugins/local.mk index 82ad99402..8182a6a83 100644 --- a/tests/plugins/local.mk +++ b/tests/plugins/local.mk @@ -8,4 +8,4 @@ libplugintest_ALLOW_UNDEFINED := 1 libplugintest_EXCLUDE_FROM_LIBRARY_LIST := 1 -libplugintest_CXXFLAGS := -I src/libutil -I src/libexpr +libplugintest_CXXFLAGS := -I src/libutil -I src/libstore -I src/libexpr From be10c09d2350019bbf4075c5e22ddb1f97d3dad0 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 3 Jan 2023 08:53:29 +0100 Subject: [PATCH 18/41] manual: Check links mdbook-linkcheck is not consistent about its warning setting. It disables some warnings, but not the warnings about lack of fragment checking support; hence the extra filtering. --- doc/manual/book.toml | 9 +++++++++ doc/manual/local.mk | 5 ++++- flake.nix | 1 + 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/doc/manual/book.toml b/doc/manual/book.toml index 46ced7ff7..73fb7e75e 100644 --- a/doc/manual/book.toml +++ b/doc/manual/book.toml @@ -10,3 +10,12 @@ git-repository-url = "https://github.com/NixOS/nix" [preprocessor.anchors] renderers = ["html"] command = "jq --from-file doc/manual/anchors.jq" + +[output.linkcheck] +# no Internet during the build (in the sandbox) +follow-web-links = false + +# mdbook-linkcheck does not understand [foo]{#bar} style links, resulting in +# excessive "Potential incomplete link" warnings. No other kind of warning was +# produced at the time of writing. +warning-policy = "ignore" diff --git a/doc/manual/local.mk b/doc/manual/local.mk index c0f69e00f..2a32f1a63 100644 --- a/doc/manual/local.mk +++ b/doc/manual/local.mk @@ -102,6 +102,9 @@ doc/manual/generated/man1/nix3-manpages: $(d)/src/command-ref/new-cli @touch $@ $(docdir)/manual/index.html: $(MANUAL_SRCS) $(d)/book.toml $(d)/anchors.jq $(d)/custom.css $(d)/src/SUMMARY.md $(d)/src/command-ref/new-cli $(d)/src/command-ref/conf-file.md $(d)/src/language/builtins.md - $(trace-gen) RUST_LOG=warn mdbook build doc/manual -d $(DESTDIR)$(docdir)/manual + $(trace-gen) \ + set -euo pipefail; \ + RUST_LOG=warn mdbook build doc/manual -d $(DESTDIR)$(docdir)/manual 2>&1 \ + | { grep -Fv "because fragment resolution isn't implemented" || :; } endif diff --git a/flake.nix b/flake.nix index 652695989..68011a16b 100644 --- a/flake.nix +++ b/flake.nix @@ -96,6 +96,7 @@ buildPackages.flex (lib.getBin buildPackages.lowdown-nix) buildPackages.mdbook + buildPackages.mdbook-linkcheck buildPackages.autoconf-archive buildPackages.autoreconfHook buildPackages.pkg-config From 34a1e0d29b55ff4f3fc7c923a3f03a65c3ff79a3 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 3 Jan 2023 10:09:08 +0100 Subject: [PATCH 19/41] doc/manual: Introduce @docroot@ as a stable base for includable snippets This way the links are clearly within the manual (ie not absolute paths), while allowing snippets to reference the documentation root reliably, regardless of at which base url they're included. --- doc/manual/local.mk | 10 +++++++--- src/libcmd/common-eval-args.cc | 4 ++-- src/libexpr/primops.cc | 2 +- src/libstore/globals.hh | 2 +- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/doc/manual/local.mk b/doc/manual/local.mk index 2a32f1a63..fdcd131de 100644 --- a/doc/manual/local.mk +++ b/doc/manual/local.mk @@ -50,11 +50,14 @@ $(d)/src/SUMMARY.md: $(d)/src/SUMMARY.md.in $(d)/src/command-ref/new-cli $(d)/src/command-ref/new-cli: $(d)/nix.json $(d)/generate-manpage.nix $(bindir)/nix @rm -rf $@ - $(trace-gen) $(nix-eval) --write-to $@ --expr 'import doc/manual/generate-manpage.nix { toplevel = builtins.readFile $<; }' + $(trace-gen) $(nix-eval) --write-to $@.tmp --expr 'import doc/manual/generate-manpage.nix { toplevel = builtins.readFile $<; }' + $(trace-gen) sed -i $@.tmp/*.md -e 's^@docroot@^../..^g' + @mv $@.tmp $@ $(d)/src/command-ref/conf-file.md: $(d)/conf-file.json $(d)/generate-options.nix $(d)/src/command-ref/conf-file-prefix.md $(bindir)/nix @cat doc/manual/src/command-ref/conf-file-prefix.md > $@.tmp - $(trace-gen) $(nix-eval) --expr 'import doc/manual/generate-options.nix (builtins.fromJSON (builtins.readFile $<))' >> $@.tmp + $(trace-gen) $(nix-eval) --expr 'import doc/manual/generate-options.nix (builtins.fromJSON (builtins.readFile $<))' \ + | sed -e 's^@docroot@^..^g'>> $@.tmp @mv $@.tmp $@ $(d)/nix.json: $(bindir)/nix @@ -67,7 +70,8 @@ $(d)/conf-file.json: $(bindir)/nix $(d)/src/language/builtins.md: $(d)/builtins.json $(d)/generate-builtins.nix $(d)/src/language/builtins-prefix.md $(bindir)/nix @cat doc/manual/src/language/builtins-prefix.md > $@.tmp - $(trace-gen) $(nix-eval) --expr 'import doc/manual/generate-builtins.nix (builtins.fromJSON (builtins.readFile $<))' >> $@.tmp + $(trace-gen) $(nix-eval) --expr 'import doc/manual/generate-builtins.nix (builtins.fromJSON (builtins.readFile $<))' \ + | sed -e 's^@docroot@^..^g' >> $@.tmp @cat doc/manual/src/language/builtins-suffix.md >> $@.tmp @mv $@.tmp $@ diff --git a/src/libcmd/common-eval-args.cc b/src/libcmd/common-eval-args.cc index 0e321e5e4..908127b4d 100644 --- a/src/libcmd/common-eval-args.cc +++ b/src/libcmd/common-eval-args.cc @@ -34,8 +34,8 @@ MixEvalArgs::MixEvalArgs() .shortName = 'I', .description = R"( Add *path* to the Nix search path. The Nix search path is - initialized from the colon-separated [`NIX_PATH`](./env-common.md#env-NIX_PATH) environment - variable, and is used to look up the location of Nix expressions using [paths](../language/values.md#type-path) enclosed in angle + initialized from the colon-separated [`NIX_PATH`](@docroot@/command-ref/env-common.md#env-NIX_PATH) environment + variable, and is used to look up the location of Nix expressions using [paths](@docroot@/language/values.md#type-path) enclosed in angle brackets (i.e., ``). For instance, passing diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 9ef91cbc5..5519642b1 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1917,7 +1917,7 @@ static RegisterPrimOp primop_toFile({ ``` Note that `${configFile}` is a - [string interpolation](language/values.md#type-string), so the result of the + [string interpolation](@docroot@/language/values.md#type-string), so the result of the expression `configFile` (i.e., a path like `/nix/store/m7p7jfny445k...-foo.conf`) will be spliced into the resulting string. diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index f026c8808..7111def92 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -676,7 +676,7 @@ public: - the store object is signed by one of the [`trusted-public-keys`](#conf-trusted-public-keys) - the substituter is in the [`trusted-substituters`](#conf-trusted-substituters) list - the [`require-sigs`](#conf-require-sigs) option has been set to `false` - - the store object is [output-addressed](glossary.md#gloss-output-addressed-store-object) + - the store object is [output-addressed](@docroot@/glossary.md#gloss-output-addressed-store-object) )", {"binary-caches"}}; From e79f93571862f62447b7c95d10e797369a0be880 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 3 Jan 2023 10:11:06 +0100 Subject: [PATCH 20/41] doc/manual: Fix broken internal links The targets I could find. --- doc/manual/src/architecture/architecture.md | 2 +- doc/manual/src/command-ref/env-common.md | 2 +- doc/manual/src/command-ref/nix-copy-closure.md | 2 +- src/libexpr/primops.cc | 6 +++--- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/doc/manual/src/architecture/architecture.md b/doc/manual/src/architecture/architecture.md index 2d1b26558..e51958052 100644 --- a/doc/manual/src/architecture/architecture.md +++ b/doc/manual/src/architecture/architecture.md @@ -68,7 +68,7 @@ It can also execute build plans to produce new data, which are made available to A build plan itself is a series of *build tasks*, together with their build inputs. > **Important** -> A build task in Nix is called [derivation](../glossary#gloss-derivation). +> A build task in Nix is called [derivation](../glossary.md#gloss-derivation). Each build task has a special build input executed as *build instructions* in order to perform the build. The result of a build task can be input to another build task. diff --git a/doc/manual/src/command-ref/env-common.md b/doc/manual/src/command-ref/env-common.md index 5845bdc43..bb85a6b07 100644 --- a/doc/manual/src/command-ref/env-common.md +++ b/doc/manual/src/command-ref/env-common.md @@ -11,7 +11,7 @@ Most Nix commands interpret the following environment variables: expressions using [paths](../language/values.md#type-path) enclosed in angle brackets (i.e., ``), e.g. `/home/eelco/Dev:/etc/nixos`. It can be extended using the - [`-I` option](./opt-common#opt-I). + [`-I` option](./opt-common.md#opt-I). - [`NIX_IGNORE_SYMLINK_STORE`]{#env-NIX_IGNORE_SYMLINK_STORE}\ Normally, the Nix store directory (typically `/nix/store`) is not diff --git a/doc/manual/src/command-ref/nix-copy-closure.md b/doc/manual/src/command-ref/nix-copy-closure.md index 83e8a2936..cd8e351bb 100644 --- a/doc/manual/src/command-ref/nix-copy-closure.md +++ b/doc/manual/src/command-ref/nix-copy-closure.md @@ -49,7 +49,7 @@ authentication, you can avoid typing the passphrase with `ssh-agent`. - `--include-outputs`\ Also copy the outputs of [store derivation]s included in the closure. - [store derivation]: ../../glossary.md#gloss-store-derivation + [store derivation]: ../glossary.md#gloss-store-derivation - `--use-substitutes` / `-s`\ Attempt to download missing paths on the target machine using Nix’s diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 5519642b1..23ddd607c 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -240,6 +240,7 @@ static RegisterPrimOp primop_scopedImport(RegisterPrimOp::Info { static RegisterPrimOp primop_import({ .name = "import", .args = {"path"}, + // TODO turn "normal path values" into link below .doc = R"( Load, parse and return the Nix expression in the file *path*. If *path* is a directory, the file ` default.nix ` in that directory @@ -253,7 +254,7 @@ static RegisterPrimOp primop_import({ > > Unlike some languages, `import` is a regular function in Nix. > Paths using the angle bracket syntax (e.g., `import` *\*) - > are [normal path values](language-values.md). + > are normal path values. A Nix expression loaded by `import` must not contain any *free variables* (identifiers that are not defined in the Nix expression @@ -1872,8 +1873,7 @@ static RegisterPrimOp primop_toFile({ path. The file has suffix *name*. This file can be used as an input to derivations. One application is to write builders “inline”. For instance, the following Nix expression combines the - [Nix expression for GNU Hello](expression-syntax.md) and its - [build script](build-script.md) into one file: + Nix expression for GNU Hello and its build script into one file: ```nix { stdenv, fetchurl, perl }: From d5c8289f1e39397fcf562fa750719e9e86569041 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 3 Jan 2023 10:29:05 +0100 Subject: [PATCH 21/41] doc/manual: Document hacking on the manual links --- doc/manual/src/contributing/hacking.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/doc/manual/src/contributing/hacking.md b/doc/manual/src/contributing/hacking.md index c9da1962f..cd3788efc 100644 --- a/doc/manual/src/contributing/hacking.md +++ b/doc/manual/src/contributing/hacking.md @@ -249,3 +249,22 @@ search/replaced in it for each new build. The installer now supports a `--tarball-url-prefix` flag which _may_ have solved this need? --> + +### Checking the manual links + +The build checks for broken internal links, but this happens late in the process, +so `nix build .` is not suitable for iterating. To check the manual incrementally, run: + +```console +make html -j $NIX_BUILD_CORES +``` + +When iterating on the makefile, run: + +```console +rm $(git ls-files doc/manual/ -o | grep -F '.md') && rmdir doc/manual/src/command-ref/new-cli && make html -j $NIX_BUILD_CORES +``` + +If a broken link occurs in a snippet that was inserted into multiple generated files in different directories, use `@docroot@` to reference the `doc/manual/src` directory. + +`mdbook-linkcheck` does not implement fragment checking yet. From fd2af69e600fdf0e06c996df607d560b222f7a38 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 3 Jan 2023 11:04:52 +0100 Subject: [PATCH 22/41] doc/manual: Move the html files back where they were before ... before the link checking "output" was added, bumping the html output into a subdirectory. --- doc/manual/local.mk | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/manual/local.mk b/doc/manual/local.mk index fdcd131de..7639f510a 100644 --- a/doc/manual/local.mk +++ b/doc/manual/local.mk @@ -108,7 +108,10 @@ doc/manual/generated/man1/nix3-manpages: $(d)/src/command-ref/new-cli $(docdir)/manual/index.html: $(MANUAL_SRCS) $(d)/book.toml $(d)/anchors.jq $(d)/custom.css $(d)/src/SUMMARY.md $(d)/src/command-ref/new-cli $(d)/src/command-ref/conf-file.md $(d)/src/language/builtins.md $(trace-gen) \ set -euo pipefail; \ - RUST_LOG=warn mdbook build doc/manual -d $(DESTDIR)$(docdir)/manual 2>&1 \ + RUST_LOG=warn mdbook build doc/manual -d $(DESTDIR)$(docdir)/manual.tmp 2>&1 \ | { grep -Fv "because fragment resolution isn't implemented" || :; } + @rm -rf $(DESTDIR)$(docdir)/manual + @mv $(DESTDIR)$(docdir)/manual.tmp/html $(DESTDIR)$(docdir)/manual + @rm -rf $(DESTDIR)$(docdir)/manual.tmp endif From fefa3a49ce67998923af0b6c927cb9cea1f4ceaa Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 3 Jan 2023 11:21:21 +0100 Subject: [PATCH 23/41] doc/manual: Apply suggestions from code review Co-authored-by: Valentin Gagarin --- doc/manual/src/contributing/hacking.md | 16 +++++++++++----- src/libexpr/primops.cc | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/doc/manual/src/contributing/hacking.md b/doc/manual/src/contributing/hacking.md index cd3788efc..fe24294f8 100644 --- a/doc/manual/src/contributing/hacking.md +++ b/doc/manual/src/contributing/hacking.md @@ -250,16 +250,19 @@ The installer now supports a `--tarball-url-prefix` flag which _may_ have solved this need? --> -### Checking the manual links +### Checking links in the manual -The build checks for broken internal links, but this happens late in the process, -so `nix build .` is not suitable for iterating. To check the manual incrementally, run: +The build checks for broken internal links. +This happens late in the process, so `nix build` is not suitable for iterating. +To build the manual incrementally, run: ```console make html -j $NIX_BUILD_CORES ``` -When iterating on the makefile, run: +In order to reflect changes to the [Makefile], clear all generated files before re-building: + +[Makefile]: https://github.com/NixOS/nix/blob/master/doc/manual/local.mk ```console rm $(git ls-files doc/manual/ -o | grep -F '.md') && rmdir doc/manual/src/command-ref/new-cli && make html -j $NIX_BUILD_CORES @@ -267,4 +270,7 @@ rm $(git ls-files doc/manual/ -o | grep -F '.md') && rmdir doc/manual/src/comman If a broken link occurs in a snippet that was inserted into multiple generated files in different directories, use `@docroot@` to reference the `doc/manual/src` directory. -`mdbook-linkcheck` does not implement fragment checking yet. +[`mdbook-linkcheck`] does not implement checking [URI fragments] yet. + +[`mdbook-linkcheck`]: https://github.com/Michael-F-Bryan/mdbook-linkcheck +[URI fragments]: https://en.m.wikipedia.org/wiki/URI_fragment diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 23ddd607c..fc1524599 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -254,7 +254,7 @@ static RegisterPrimOp primop_import({ > > Unlike some languages, `import` is a regular function in Nix. > Paths using the angle bracket syntax (e.g., `import` *\*) - > are normal path values. + > are normal [path values](@docroot@/language/values.md#type-path). A Nix expression loaded by `import` must not contain any *free variables* (identifiers that are not defined in the Nix expression From da4d4feacf76a829ee7929c3d69f6f3d6ba86f1f Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 3 Jan 2023 11:36:01 +0100 Subject: [PATCH 24/41] doc/manual/hacking: Document @docroot@ variable --- doc/manual/local.mk | 3 +++ doc/manual/src/contributing/hacking.md | 10 ++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/doc/manual/local.mk b/doc/manual/local.mk index 7639f510a..190f0258a 100644 --- a/doc/manual/local.mk +++ b/doc/manual/local.mk @@ -51,11 +51,13 @@ $(d)/src/SUMMARY.md: $(d)/src/SUMMARY.md.in $(d)/src/command-ref/new-cli $(d)/src/command-ref/new-cli: $(d)/nix.json $(d)/generate-manpage.nix $(bindir)/nix @rm -rf $@ $(trace-gen) $(nix-eval) --write-to $@.tmp --expr 'import doc/manual/generate-manpage.nix { toplevel = builtins.readFile $<; }' + # @docroot@: https://nixos.org/manual/nix/unstable/contributing/hacking.html#docroot-variable $(trace-gen) sed -i $@.tmp/*.md -e 's^@docroot@^../..^g' @mv $@.tmp $@ $(d)/src/command-ref/conf-file.md: $(d)/conf-file.json $(d)/generate-options.nix $(d)/src/command-ref/conf-file-prefix.md $(bindir)/nix @cat doc/manual/src/command-ref/conf-file-prefix.md > $@.tmp + # @docroot@: https://nixos.org/manual/nix/unstable/contributing/hacking.html#docroot-variable $(trace-gen) $(nix-eval) --expr 'import doc/manual/generate-options.nix (builtins.fromJSON (builtins.readFile $<))' \ | sed -e 's^@docroot@^..^g'>> $@.tmp @mv $@.tmp $@ @@ -70,6 +72,7 @@ $(d)/conf-file.json: $(bindir)/nix $(d)/src/language/builtins.md: $(d)/builtins.json $(d)/generate-builtins.nix $(d)/src/language/builtins-prefix.md $(bindir)/nix @cat doc/manual/src/language/builtins-prefix.md > $@.tmp + # @docroot@: https://nixos.org/manual/nix/unstable/contributing/hacking.html#docroot-variable $(trace-gen) $(nix-eval) --expr 'import doc/manual/generate-builtins.nix (builtins.fromJSON (builtins.readFile $<))' \ | sed -e 's^@docroot@^..^g' >> $@.tmp @cat doc/manual/src/language/builtins-suffix.md >> $@.tmp diff --git a/doc/manual/src/contributing/hacking.md b/doc/manual/src/contributing/hacking.md index fe24294f8..b9b327fba 100644 --- a/doc/manual/src/contributing/hacking.md +++ b/doc/manual/src/contributing/hacking.md @@ -268,9 +268,15 @@ In order to reflect changes to the [Makefile], clear all generated files before rm $(git ls-files doc/manual/ -o | grep -F '.md') && rmdir doc/manual/src/command-ref/new-cli && make html -j $NIX_BUILD_CORES ``` -If a broken link occurs in a snippet that was inserted into multiple generated files in different directories, use `@docroot@` to reference the `doc/manual/src` directory. - [`mdbook-linkcheck`] does not implement checking [URI fragments] yet. [`mdbook-linkcheck`]: https://github.com/Michael-F-Bryan/mdbook-linkcheck [URI fragments]: https://en.m.wikipedia.org/wiki/URI_fragment + +#### `@docroot@` variable + +`@docroot@` provides a base path for links that occur in reusable snippets or other documentation that doesn't have a base path of its own. + +If a broken link occurs in a snippet that was inserted into multiple generated files in different directories, use `@docroot@` to reference the `doc/manual/src` directory. + +If the `@docroot@` literal appears in an error message from the `mdbook-linkcheck` tool, the `@docroot@` replacement needs to be applied to the (probably) generated source file that mentions it. See existing `@docroot@` logic in the [Makefile]. Regular markdown files that are not copied to other markdown files do have a base path of their own and they can use relative paths instead of `@docroot@`. From 6ae4d762d0a1b208eaa47e9bd8960571ac908af7 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 3 Jan 2023 13:52:17 +0100 Subject: [PATCH 25/41] doc/manual/src/contributing/hacking.md: Apply suggestion Co-authored-by: Valentin Gagarin --- doc/manual/src/contributing/hacking.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/manual/src/contributing/hacking.md b/doc/manual/src/contributing/hacking.md index b9b327fba..aeb0d41b3 100644 --- a/doc/manual/src/contributing/hacking.md +++ b/doc/manual/src/contributing/hacking.md @@ -279,4 +279,6 @@ rm $(git ls-files doc/manual/ -o | grep -F '.md') && rmdir doc/manual/src/comman If a broken link occurs in a snippet that was inserted into multiple generated files in different directories, use `@docroot@` to reference the `doc/manual/src` directory. -If the `@docroot@` literal appears in an error message from the `mdbook-linkcheck` tool, the `@docroot@` replacement needs to be applied to the (probably) generated source file that mentions it. See existing `@docroot@` logic in the [Makefile]. Regular markdown files that are not copied to other markdown files do have a base path of their own and they can use relative paths instead of `@docroot@`. +If the `@docroot@` literal appears in an error message from the `mdbook-linkcheck` tool, the `@docroot@` replacement needs to be applied to the generated source file that mentions it. +See existing `@docroot@` logic in the [Makefile]. +Regular markdown files used for the manual have a base path of their own and they can use relative paths instead of `@docroot@`. From 7515617ad046a08f2b9d0dd1f552e8841be4971d Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 11 Jan 2023 13:49:39 +0100 Subject: [PATCH 26/41] Backport getLine tests from lazy-trees --- src/libutil/tests/tests.cc | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/libutil/tests/tests.cc b/src/libutil/tests/tests.cc index 6e325db98..250e83a38 100644 --- a/src/libutil/tests/tests.cc +++ b/src/libutil/tests/tests.cc @@ -311,6 +311,42 @@ namespace nix { ASSERT_THROW(base64Decode("cXVvZCBlcm_0IGRlbW9uc3RyYW5kdW0="), Error); } + /* ---------------------------------------------------------------------------- + * getLine + * --------------------------------------------------------------------------*/ + + TEST(getLine, all) { + { + auto [line, rest] = getLine("foo\nbar\nxyzzy"); + ASSERT_EQ(line, "foo"); + ASSERT_EQ(rest, "bar\nxyzzy"); + } + + { + auto [line, rest] = getLine("foo\r\nbar\r\nxyzzy"); + ASSERT_EQ(line, "foo"); + ASSERT_EQ(rest, "bar\r\nxyzzy"); + } + + { + auto [line, rest] = getLine("foo\n"); + ASSERT_EQ(line, "foo"); + ASSERT_EQ(rest, ""); + } + + { + auto [line, rest] = getLine("foo"); + ASSERT_EQ(line, "foo"); + ASSERT_EQ(rest, ""); + } + + { + auto [line, rest] = getLine(""); + ASSERT_EQ(line, ""); + ASSERT_EQ(rest, ""); + } + } + /* ---------------------------------------------------------------------------- * toLower * --------------------------------------------------------------------------*/ From a8f45b5e5a42daa9bdee640255464d4dbb431352 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 01:51:14 -0500 Subject: [PATCH 27/41] Improve `OutputsSpec` slightly A few little changes preparing for the rest. --- src/libcmd/installables.cc | 3 ++- src/libexpr/flake/flakeref.cc | 2 +- src/libstore/outputs-spec.cc | 26 ++++++++++++++------------ src/libstore/outputs-spec.hh | 26 +++++++++++++++++++------- src/libstore/path-with-outputs.hh | 1 - src/libstore/tests/outputs-spec.cc | 14 +++++++------- src/nix/profile.cc | 6 +++--- 7 files changed, 46 insertions(+), 32 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index c0db2a715..ce40986d0 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -1,5 +1,6 @@ #include "globals.hh" #include "installables.hh" +#include "outputs-spec.hh" #include "util.hh" #include "command.hh" #include "attr-path.hh" @@ -796,7 +797,7 @@ std::vector> SourceExprCommand::parseInstallables( } for (auto & s : ss) { - auto [prefix, outputsSpec] = parseOutputsSpec(s); + auto [prefix, outputsSpec] = OutputsSpec::parse(s); result.push_back( std::make_shared( state, *this, vFile, diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index eede493f8..cfa279fb4 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -244,7 +244,7 @@ std::tuple parseFlakeRefWithFragmentAndOutpu bool allowMissing, bool isFlake) { - auto [prefix, outputsSpec] = parseOutputsSpec(url); + auto [prefix, outputsSpec] = OutputsSpec::parse(url); auto [flakeRef, fragment] = parseFlakeRefWithFragment(prefix, baseDir, allowMissing, isFlake); return {std::move(flakeRef), fragment, outputsSpec}; } diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index 76779d193..b5ea7e325 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -1,3 +1,4 @@ +#include "util.hh" #include "outputs-spec.hh" #include "nlohmann/json.hpp" @@ -5,7 +6,7 @@ namespace nix { -std::pair parseOutputsSpec(const std::string & s) +std::pair OutputsSpec::parse(std::string s) { static std::regex regex(R"((.*)\^((\*)|([a-z]+(,[a-z]+)*)))"); @@ -19,18 +20,19 @@ std::pair parseOutputsSpec(const std::string & s) return {match[1], tokenizeString(match[4].str(), ",")}; } -std::string printOutputsSpec(const OutputsSpec & outputsSpec) +std::string OutputsSpec::to_string() const { - if (std::get_if(&outputsSpec)) - return ""; - - if (std::get_if(&outputsSpec)) - return "^*"; - - if (auto outputNames = std::get_if(&outputsSpec)) - return "^" + concatStringsSep(",", *outputNames); - - assert(false); + return std::visit(overloaded { + [&](const OutputsSpec::Default &) -> std::string { + return ""; + }, + [&](const OutputsSpec::All &) -> std::string { + return "*"; + }, + [&](const OutputsSpec::Names & outputNames) -> std::string { + return "^" + concatStringsSep(",", outputNames); + }, + }, raw()); } void to_json(nlohmann::json & json, const OutputsSpec & outputsSpec) diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index e2cf1d12b..6f886ccb6 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -1,9 +1,8 @@ #pragma once +#include #include -#include "util.hh" - #include "nlohmann/json_fwd.hpp" namespace nix { @@ -18,13 +17,26 @@ struct DefaultOutputs { bool operator < (const DefaultOutputs & _) const { return false; } }; -typedef std::variant OutputsSpec; +typedef std::variant _OutputsSpecRaw; -/* Parse a string of the form 'prefix^output1,...outputN' or - 'prefix^*', returning the prefix and the outputs spec. */ -std::pair parseOutputsSpec(const std::string & s); +struct OutputsSpec : _OutputsSpecRaw { + using Raw = _OutputsSpecRaw; + using Raw::Raw; -std::string printOutputsSpec(const OutputsSpec & outputsSpec); + using Names = OutputNames; + using All = AllOutputs; + using Default = DefaultOutputs; + + inline const Raw & raw() const { + return static_cast(*this); + } + + /* Parse a string of the form 'prefix^output1,...outputN' or + 'prefix^*', returning the prefix and the outputs spec. */ + static std::pair parse(std::string s); + + std::string to_string() const; +}; void to_json(nlohmann::json &, const OutputsSpec &); void from_json(const nlohmann::json &, OutputsSpec &); diff --git a/src/libstore/path-with-outputs.hh b/src/libstore/path-with-outputs.hh index ed55cc333..5d25656a5 100644 --- a/src/libstore/path-with-outputs.hh +++ b/src/libstore/path-with-outputs.hh @@ -2,7 +2,6 @@ #include "path.hh" #include "derived-path.hh" -#include "nlohmann/json_fwd.hpp" namespace nix { diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index d781a930e..552380837 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -4,40 +4,40 @@ namespace nix { -TEST(parseOutputsSpec, basic) +TEST(OutputsSpec_parse, basic) { { - auto [prefix, outputsSpec] = parseOutputsSpec("foo"); + auto [prefix, outputsSpec] = OutputsSpec::parse("foo"); ASSERT_EQ(prefix, "foo"); ASSERT_TRUE(std::get_if(&outputsSpec)); } { - auto [prefix, outputsSpec] = parseOutputsSpec("foo^*"); + auto [prefix, outputsSpec] = OutputsSpec::parse("foo^*"); ASSERT_EQ(prefix, "foo"); ASSERT_TRUE(std::get_if(&outputsSpec)); } { - auto [prefix, outputsSpec] = parseOutputsSpec("foo^out"); + auto [prefix, outputsSpec] = OutputsSpec::parse("foo^out"); ASSERT_EQ(prefix, "foo"); ASSERT_TRUE(std::get(outputsSpec) == OutputNames({"out"})); } { - auto [prefix, outputsSpec] = parseOutputsSpec("foo^out,bin"); + auto [prefix, outputsSpec] = OutputsSpec::parse("foo^out,bin"); ASSERT_EQ(prefix, "foo"); ASSERT_TRUE(std::get(outputsSpec) == OutputNames({"out", "bin"})); } { - auto [prefix, outputsSpec] = parseOutputsSpec("foo^bar^out,bin"); + auto [prefix, outputsSpec] = OutputsSpec::parse("foo^bar^out,bin"); ASSERT_EQ(prefix, "foo^bar"); ASSERT_TRUE(std::get(outputsSpec) == OutputNames({"out", "bin"})); } { - auto [prefix, outputsSpec] = parseOutputsSpec("foo^&*()"); + auto [prefix, outputsSpec] = OutputsSpec::parse("foo^&*()"); ASSERT_EQ(prefix, "foo^&*()"); ASSERT_TRUE(std::get_if(&outputsSpec)); } diff --git a/src/nix/profile.cc b/src/nix/profile.cc index 22ee51ab9..346c4e117 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -44,7 +44,7 @@ struct ProfileElement std::string describe() const { if (source) - return fmt("%s#%s%s", source->originalRef, source->attrPath, printOutputsSpec(source->outputs)); + return fmt("%s#%s%s", source->originalRef, source->attrPath, source->outputs.to_string()); StringSet names; for (auto & path : storePaths) names.insert(DrvName(path.name()).name); @@ -553,8 +553,8 @@ struct CmdProfileList : virtual EvalCommand, virtual StoreCommand, MixDefaultPro for (size_t i = 0; i < manifest.elements.size(); ++i) { auto & element(manifest.elements[i]); logger->cout("%d %s %s %s", i, - element.source ? element.source->originalRef.to_string() + "#" + element.source->attrPath + printOutputsSpec(element.source->outputs) : "-", - element.source ? element.source->resolvedRef.to_string() + "#" + element.source->attrPath + printOutputsSpec(element.source->outputs) : "-", + element.source ? element.source->originalRef.to_string() + "#" + element.source->attrPath + element.source->outputs.to_string() : "-", + element.source ? element.source->resolvedRef.to_string() + "#" + element.source->attrPath + element.source->outputs.to_string() : "-", concatStringsSep(" ", store->printStorePathSet(element.storePaths))); } } From a7c0cff07f3e1af60bdbcd5bf7e13f8ae768da90 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 02:00:44 -0500 Subject: [PATCH 28/41] Rename `OutputPath` -> `ExtendedOutputPath` Do this prior to making a new more limitted `OutputPath` we will use in more places. --- src/libcmd/installables.cc | 28 ++++++++++++++-------------- src/libcmd/installables.hh | 6 +++--- src/libexpr/flake/flakeref.cc | 6 +++--- src/libexpr/flake/flakeref.hh | 2 +- src/libstore/outputs-spec.cc | 26 +++++++++++++------------- src/libstore/outputs-spec.hh | 12 ++++++------ src/libstore/tests/outputs-spec.cc | 26 +++++++++++++------------- src/nix/bundle.cc | 4 ++-- src/nix/profile.cc | 10 +++++----- 9 files changed, 60 insertions(+), 60 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index ce40986d0..b80ae4df1 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -446,19 +446,19 @@ struct InstallableAttrPath : InstallableValue SourceExprCommand & cmd; RootValue v; std::string attrPath; - OutputsSpec outputsSpec; + ExtendedOutputsSpec extendedOutputsSpec; InstallableAttrPath( ref state, SourceExprCommand & cmd, Value * v, const std::string & attrPath, - OutputsSpec outputsSpec) + ExtendedOutputsSpec extendedOutputsSpec) : InstallableValue(state) , cmd(cmd) , v(allocRootValue(v)) , attrPath(attrPath) - , outputsSpec(std::move(outputsSpec)) + , extendedOutputsSpec(std::move(extendedOutputsSpec)) { } std::string what() const override { return attrPath; } @@ -490,10 +490,10 @@ struct InstallableAttrPath : InstallableValue std::set outputsToInstall; - if (auto outputNames = std::get_if(&outputsSpec)) + if (auto outputNames = std::get_if(&extendedOutputsSpec)) outputsToInstall = *outputNames; else - for (auto & output : drvInfo.queryOutputs(false, std::get_if(&outputsSpec))) + for (auto & output : drvInfo.queryOutputs(false, std::get_if(&extendedOutputsSpec))) outputsToInstall.insert(output.first); auto derivedPath = byDrvPath.emplace(*drvPath, DerivedPath::Built { .drvPath = *drvPath }).first; @@ -581,7 +581,7 @@ InstallableFlake::InstallableFlake( ref state, FlakeRef && flakeRef, std::string_view fragment, - OutputsSpec outputsSpec, + ExtendedOutputsSpec extendedOutputsSpec, Strings attrPaths, Strings prefixes, const flake::LockFlags & lockFlags) @@ -589,7 +589,7 @@ InstallableFlake::InstallableFlake( flakeRef(flakeRef), attrPaths(fragment == "" ? attrPaths : Strings{(std::string) fragment}), prefixes(fragment == "" ? Strings{} : prefixes), - outputsSpec(std::move(outputsSpec)), + extendedOutputsSpec(std::move(extendedOutputsSpec)), lockFlags(lockFlags) { if (cmd && cmd->getAutoArgs(*state)->size()) @@ -657,7 +657,7 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() priority = aPriority->getInt(); } - if (outputsToInstall.empty() || std::get_if(&outputsSpec)) { + if (outputsToInstall.empty() || std::get_if(&extendedOutputsSpec)) { outputsToInstall.clear(); if (auto aOutputs = attr->maybeGetAttr(state->sOutputs)) for (auto & s : aOutputs->getListOfStrings()) @@ -667,7 +667,7 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() if (outputsToInstall.empty()) outputsToInstall.insert("out"); - if (auto outputNames = std::get_if(&outputsSpec)) + if (auto outputNames = std::get_if(&extendedOutputsSpec)) outputsToInstall = *outputNames; return {{ @@ -680,7 +680,7 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() .originalRef = flakeRef, .resolvedRef = getLockedFlake()->flake.lockedRef, .attrPath = attrPath, - .outputsSpec = outputsSpec, + .extendedOutputsSpec = extendedOutputsSpec, } }}; } @@ -797,12 +797,12 @@ std::vector> SourceExprCommand::parseInstallables( } for (auto & s : ss) { - auto [prefix, outputsSpec] = OutputsSpec::parse(s); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(s); result.push_back( std::make_shared( state, *this, vFile, prefix == "." ? "" : prefix, - outputsSpec)); + extendedOutputsSpec)); } } else { @@ -837,13 +837,13 @@ std::vector> SourceExprCommand::parseInstallables( } try { - auto [flakeRef, fragment, outputsSpec] = parseFlakeRefWithFragmentAndOutputsSpec(s, absPath(".")); + auto [flakeRef, fragment, extendedOutputsSpec] = parseFlakeRefWithFragmentAndExtendedOutputsSpec(s, absPath(".")); result.push_back(std::make_shared( this, getEvalState(), std::move(flakeRef), fragment, - outputsSpec, + extendedOutputsSpec, getDefaultFlakeAttrPaths(), getDefaultFlakeAttrPathPrefixes(), lockFlags)); diff --git a/src/libcmd/installables.hh b/src/libcmd/installables.hh index 9b92cc4be..3d12639b0 100644 --- a/src/libcmd/installables.hh +++ b/src/libcmd/installables.hh @@ -59,7 +59,7 @@ struct ExtraPathInfo std::optional resolvedRef; std::optional attrPath; // FIXME: merge with DerivedPath's 'outputs' field? - std::optional outputsSpec; + std::optional extendedOutputsSpec; }; /* A derived path with any additional info that commands might @@ -169,7 +169,7 @@ struct InstallableFlake : InstallableValue FlakeRef flakeRef; Strings attrPaths; Strings prefixes; - OutputsSpec outputsSpec; + ExtendedOutputsSpec extendedOutputsSpec; const flake::LockFlags & lockFlags; mutable std::shared_ptr _lockedFlake; @@ -178,7 +178,7 @@ struct InstallableFlake : InstallableValue ref state, FlakeRef && flakeRef, std::string_view fragment, - OutputsSpec outputsSpec, + ExtendedOutputsSpec extendedOutputsSpec, Strings attrPaths, Strings prefixes, const flake::LockFlags & lockFlags); diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index cfa279fb4..bc61e2c9a 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -238,15 +238,15 @@ std::pair FlakeRef::fetchTree(ref store) const return {std::move(tree), FlakeRef(std::move(lockedInput), subdir)}; } -std::tuple parseFlakeRefWithFragmentAndOutputsSpec( +std::tuple parseFlakeRefWithFragmentAndExtendedOutputsSpec( const std::string & url, const std::optional & baseDir, bool allowMissing, bool isFlake) { - auto [prefix, outputsSpec] = OutputsSpec::parse(url); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(url); auto [flakeRef, fragment] = parseFlakeRefWithFragment(prefix, baseDir, allowMissing, isFlake); - return {std::move(flakeRef), fragment, outputsSpec}; + return {std::move(flakeRef), fragment, extendedOutputsSpec}; } } diff --git a/src/libexpr/flake/flakeref.hh b/src/libexpr/flake/flakeref.hh index 4ec79fb73..c4142fc20 100644 --- a/src/libexpr/flake/flakeref.hh +++ b/src/libexpr/flake/flakeref.hh @@ -80,7 +80,7 @@ std::pair parseFlakeRefWithFragment( std::optional> maybeParseFlakeRefWithFragment( const std::string & url, const std::optional & baseDir = {}); -std::tuple parseFlakeRefWithFragmentAndOutputsSpec( +std::tuple parseFlakeRefWithFragmentAndExtendedOutputsSpec( const std::string & url, const std::optional & baseDir = {}, bool allowMissing = false, diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index b5ea7e325..c446976bc 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -6,7 +6,7 @@ namespace nix { -std::pair OutputsSpec::parse(std::string s) +std::pair ExtendedOutputsSpec::parse(std::string s) { static std::regex regex(R"((.*)\^((\*)|([a-z]+(,[a-z]+)*)))"); @@ -20,43 +20,43 @@ std::pair OutputsSpec::parse(std::string s) return {match[1], tokenizeString(match[4].str(), ",")}; } -std::string OutputsSpec::to_string() const +std::string ExtendedOutputsSpec::to_string() const { return std::visit(overloaded { - [&](const OutputsSpec::Default &) -> std::string { + [&](const ExtendedOutputsSpec::Default &) -> std::string { return ""; }, - [&](const OutputsSpec::All &) -> std::string { + [&](const ExtendedOutputsSpec::All &) -> std::string { return "*"; }, - [&](const OutputsSpec::Names & outputNames) -> std::string { + [&](const ExtendedOutputsSpec::Names & outputNames) -> std::string { return "^" + concatStringsSep(",", outputNames); }, }, raw()); } -void to_json(nlohmann::json & json, const OutputsSpec & outputsSpec) +void to_json(nlohmann::json & json, const ExtendedOutputsSpec & extendedOutputsSpec) { - if (std::get_if(&outputsSpec)) + if (std::get_if(&extendedOutputsSpec)) json = nullptr; - else if (std::get_if(&outputsSpec)) + else if (std::get_if(&extendedOutputsSpec)) json = std::vector({"*"}); - else if (auto outputNames = std::get_if(&outputsSpec)) + else if (auto outputNames = std::get_if(&extendedOutputsSpec)) json = *outputNames; } -void from_json(const nlohmann::json & json, OutputsSpec & outputsSpec) +void from_json(const nlohmann::json & json, ExtendedOutputsSpec & extendedOutputsSpec) { if (json.is_null()) - outputsSpec = DefaultOutputs(); + extendedOutputsSpec = DefaultOutputs(); else { auto names = json.get(); if (names == OutputNames({"*"})) - outputsSpec = AllOutputs(); + extendedOutputsSpec = AllOutputs(); else - outputsSpec = names; + extendedOutputsSpec = names; } } diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index 6f886ccb6..5ed711a62 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -17,10 +17,10 @@ struct DefaultOutputs { bool operator < (const DefaultOutputs & _) const { return false; } }; -typedef std::variant _OutputsSpecRaw; +typedef std::variant _ExtendedOutputsSpecRaw; -struct OutputsSpec : _OutputsSpecRaw { - using Raw = _OutputsSpecRaw; +struct ExtendedOutputsSpec : _ExtendedOutputsSpecRaw { + using Raw = _ExtendedOutputsSpecRaw; using Raw::Raw; using Names = OutputNames; @@ -33,12 +33,12 @@ struct OutputsSpec : _OutputsSpecRaw { /* Parse a string of the form 'prefix^output1,...outputN' or 'prefix^*', returning the prefix and the outputs spec. */ - static std::pair parse(std::string s); + static std::pair parse(std::string s); std::string to_string() const; }; -void to_json(nlohmann::json &, const OutputsSpec &); -void from_json(const nlohmann::json &, OutputsSpec &); +void to_json(nlohmann::json &, const ExtendedOutputsSpec &); +void from_json(const nlohmann::json &, ExtendedOutputsSpec &); } diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index 552380837..a3dd42341 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -4,42 +4,42 @@ namespace nix { -TEST(OutputsSpec_parse, basic) +TEST(ExtendedOutputsSpec_parse, basic) { { - auto [prefix, outputsSpec] = OutputsSpec::parse("foo"); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get_if(&outputsSpec)); + ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); } { - auto [prefix, outputsSpec] = OutputsSpec::parse("foo^*"); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^*"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get_if(&outputsSpec)); + ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); } { - auto [prefix, outputsSpec] = OutputsSpec::parse("foo^out"); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get(outputsSpec) == OutputNames({"out"})); + ASSERT_TRUE(std::get(extendedOutputsSpec) == OutputNames({"out"})); } { - auto [prefix, outputsSpec] = OutputsSpec::parse("foo^out,bin"); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out,bin"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get(outputsSpec) == OutputNames({"out", "bin"})); + ASSERT_TRUE(std::get(extendedOutputsSpec) == OutputNames({"out", "bin"})); } { - auto [prefix, outputsSpec] = OutputsSpec::parse("foo^bar^out,bin"); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^bar^out,bin"); ASSERT_EQ(prefix, "foo^bar"); - ASSERT_TRUE(std::get(outputsSpec) == OutputNames({"out", "bin"})); + ASSERT_TRUE(std::get(extendedOutputsSpec) == OutputNames({"out", "bin"})); } { - auto [prefix, outputsSpec] = OutputsSpec::parse("foo^&*()"); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^&*()"); ASSERT_EQ(prefix, "foo^&*()"); - ASSERT_TRUE(std::get_if(&outputsSpec)); + ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); } } diff --git a/src/nix/bundle.cc b/src/nix/bundle.cc index 74a7973b0..a09dadff4 100644 --- a/src/nix/bundle.cc +++ b/src/nix/bundle.cc @@ -75,10 +75,10 @@ struct CmdBundle : InstallableCommand auto val = installable->toValue(*evalState).first; - auto [bundlerFlakeRef, bundlerName, outputsSpec] = parseFlakeRefWithFragmentAndOutputsSpec(bundler, absPath(".")); + auto [bundlerFlakeRef, bundlerName, extendedOutputsSpec] = parseFlakeRefWithFragmentAndExtendedOutputsSpec(bundler, absPath(".")); const flake::LockFlags lockFlags{ .writeLockFile = false }; InstallableFlake bundler{this, - evalState, std::move(bundlerFlakeRef), bundlerName, outputsSpec, + evalState, std::move(bundlerFlakeRef), bundlerName, extendedOutputsSpec, {"bundlers." + settings.thisSystem.get() + ".default", "defaultBundler." + settings.thisSystem.get() }, diff --git a/src/nix/profile.cc b/src/nix/profile.cc index 346c4e117..32364e720 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -22,7 +22,7 @@ struct ProfileElementSource // FIXME: record original attrpath. FlakeRef resolvedRef; std::string attrPath; - OutputsSpec outputs; + ExtendedOutputsSpec outputs; bool operator < (const ProfileElementSource & other) const { @@ -126,7 +126,7 @@ struct ProfileManifest parseFlakeRef(e[sOriginalUrl]), parseFlakeRef(e[sUrl]), e["attrPath"], - e["outputs"].get() + e["outputs"].get() }; } elements.emplace_back(std::move(element)); @@ -308,12 +308,12 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile auto & [res, info] = builtPaths[installable.get()]; - if (info.originalRef && info.resolvedRef && info.attrPath && info.outputsSpec) { + if (info.originalRef && info.resolvedRef && info.attrPath && info.extendedOutputsSpec) { element.source = ProfileElementSource { .originalRef = *info.originalRef, .resolvedRef = *info.resolvedRef, .attrPath = *info.attrPath, - .outputs = *info.outputsSpec, + .outputs = *info.extendedOutputsSpec, }; } @@ -497,7 +497,7 @@ struct CmdProfileUpgrade : virtual SourceExprCommand, MixDefaultProfile, MixProf .originalRef = installable->flakeRef, .resolvedRef = *info.resolvedRef, .attrPath = *info.attrPath, - .outputs = installable->outputsSpec, + .outputs = installable->extendedOutputsSpec, }; installables.push_back(installable); From ce2f91d356438297fd795bd3edb8f9f4536db7da Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 18:57:18 -0500 Subject: [PATCH 29/41] Split `OutputsSpec` and `ExtendedOutputsSpec`, use the former more `DerivedPath::Built` and `DerivationGoal` were previously using a regular set with the convention that the empty set means all outputs. But it is easy to forget about this rule when processing those sets. Using `OutputSpec` forces us to get it right. --- src/libcmd/installables.cc | 107 ++++++-------- src/libexpr/flake/flakeref.cc | 2 +- src/libexpr/primops.cc | 16 +-- src/libstore/build/derivation-goal.cc | 47 ++++--- src/libstore/build/derivation-goal.hh | 9 +- src/libstore/build/entry-points.cc | 3 +- src/libstore/build/local-derivation-goal.cc | 2 +- src/libstore/build/worker.cc | 6 +- src/libstore/build/worker.hh | 6 +- src/libstore/derivations.cc | 6 - src/libstore/derivations.hh | 2 - src/libstore/derived-path.cc | 25 ++-- src/libstore/derived-path.hh | 3 +- src/libstore/misc.cc | 45 +++++- src/libstore/outputs-spec.cc | 148 ++++++++++++++++---- src/libstore/outputs-spec.hh | 46 +++++- src/libstore/path-with-outputs.cc | 23 ++- src/libstore/path.hh | 1 - src/libstore/remote-store.cc | 11 +- src/libstore/store-api.hh | 10 ++ src/libstore/tests/outputs-spec.cc | 42 ++++-- src/nix-build/nix-build.cc | 4 +- src/nix/app.cc | 6 +- 23 files changed, 377 insertions(+), 193 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index b80ae4df1..58b5ef9b9 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -488,18 +488,23 @@ struct InstallableAttrPath : InstallableValue if (!drvPath) throw Error("'%s' is not a derivation", what()); - std::set outputsToInstall; + auto derivedPath = byDrvPath.emplace(*drvPath, DerivedPath::Built { + .drvPath = *drvPath, + // Not normally legal, but we will merge right below + .outputs = OutputsSpec::Names { }, + }).first; - if (auto outputNames = std::get_if(&extendedOutputsSpec)) - outputsToInstall = *outputNames; - else - for (auto & output : drvInfo.queryOutputs(false, std::get_if(&extendedOutputsSpec))) - outputsToInstall.insert(output.first); - - auto derivedPath = byDrvPath.emplace(*drvPath, DerivedPath::Built { .drvPath = *drvPath }).first; - - for (auto & output : outputsToInstall) - derivedPath->second.outputs.insert(output); + derivedPath->second.outputs.merge(std::visit(overloaded { + [&](const ExtendedOutputsSpec::Default & d) -> OutputsSpec { + std::set outputsToInstall; + for (auto & output : drvInfo.queryOutputs(false, true)) + outputsToInstall.insert(output.first); + return OutputsSpec::Names { std::move(outputsToInstall) }; + }, + [&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec { + return e; + }, + }, extendedOutputsSpec.raw())); } DerivedPathsWithInfo res; @@ -639,41 +644,40 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() auto drvPath = attr->forceDerivation(); - std::set outputsToInstall; std::optional priority; - if (auto aOutputSpecified = attr->maybeGetAttr(state->sOutputSpecified)) { - if (aOutputSpecified->getBool()) { - if (auto aOutputName = attr->maybeGetAttr("outputName")) - outputsToInstall = { aOutputName->getString() }; - } - } - - else if (auto aMeta = attr->maybeGetAttr(state->sMeta)) { - if (auto aOutputsToInstall = aMeta->maybeGetAttr("outputsToInstall")) - for (auto & s : aOutputsToInstall->getListOfStrings()) - outputsToInstall.insert(s); + if (attr->maybeGetAttr(state->sOutputSpecified)) { + } else if (auto aMeta = attr->maybeGetAttr(state->sMeta)) { if (auto aPriority = aMeta->maybeGetAttr("priority")) priority = aPriority->getInt(); } - if (outputsToInstall.empty() || std::get_if(&extendedOutputsSpec)) { - outputsToInstall.clear(); - if (auto aOutputs = attr->maybeGetAttr(state->sOutputs)) - for (auto & s : aOutputs->getListOfStrings()) - outputsToInstall.insert(s); - } - - if (outputsToInstall.empty()) - outputsToInstall.insert("out"); - - if (auto outputNames = std::get_if(&extendedOutputsSpec)) - outputsToInstall = *outputNames; - return {{ .path = DerivedPath::Built { .drvPath = std::move(drvPath), - .outputs = std::move(outputsToInstall), + .outputs = std::visit(overloaded { + [&](const ExtendedOutputsSpec::Default & d) -> OutputsSpec { + std::set outputsToInstall; + if (auto aOutputSpecified = attr->maybeGetAttr(state->sOutputSpecified)) { + if (aOutputSpecified->getBool()) { + if (auto aOutputName = attr->maybeGetAttr("outputName")) + outputsToInstall = { aOutputName->getString() }; + } + } else if (auto aMeta = attr->maybeGetAttr(state->sMeta)) { + if (auto aOutputsToInstall = aMeta->maybeGetAttr("outputsToInstall")) + for (auto & s : aOutputsToInstall->getListOfStrings()) + outputsToInstall.insert(s); + } + + if (outputsToInstall.empty()) + outputsToInstall.insert("out"); + + return OutputsSpec::Names { std::move(outputsToInstall) }; + }, + [&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec { + return e; + }, + }, extendedOutputsSpec.raw()), }, .info = { .priority = priority, @@ -801,7 +805,7 @@ std::vector> SourceExprCommand::parseInstallables( result.push_back( std::make_shared( state, *this, vFile, - prefix == "." ? "" : prefix, + prefix == "." ? "" : std::string { prefix }, extendedOutputsSpec)); } @@ -918,32 +922,7 @@ std::vector, BuiltPathWithResult>> Instal for (auto & aux : backmap[path]) { std::visit(overloaded { [&](const DerivedPath::Built & bfd) { - OutputPathMap outputs; - auto drv = evalStore->readDerivation(bfd.drvPath); - auto outputHashes = staticOutputHashes(*evalStore, drv); // FIXME: expensive - auto drvOutputs = drv.outputsAndOptPaths(*store); - for (auto & output : bfd.outputs) { - auto outputHash = get(outputHashes, output); - if (!outputHash) - throw Error( - "the derivation '%s' doesn't have an output named '%s'", - store->printStorePath(bfd.drvPath), output); - if (settings.isExperimentalFeatureEnabled(Xp::CaDerivations)) { - DrvOutput outputId { *outputHash, output }; - auto realisation = store->queryRealisation(outputId); - if (!realisation) - throw MissingRealisation(outputId); - outputs.insert_or_assign(output, realisation->outPath); - } else { - // If ca-derivations isn't enabled, assume that - // the output path is statically known. - auto drvOutput = get(drvOutputs, output); - assert(drvOutput); - assert(drvOutput->second); - outputs.insert_or_assign( - output, *drvOutput->second); - } - } + auto outputs = resolveDerivedPath(*store, bfd, &*evalStore); res.push_back({aux.installable, { .path = BuiltPath::Built { bfd.drvPath, outputs }, .info = aux.info}}); diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index bc61e2c9a..08adbe0c9 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -245,7 +245,7 @@ std::tuple parseFlakeRefWithFragment bool isFlake) { auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(url); - auto [flakeRef, fragment] = parseFlakeRefWithFragment(prefix, baseDir, allowMissing, isFlake); + auto [flakeRef, fragment] = parseFlakeRefWithFragment(std::string { prefix }, baseDir, allowMissing, isFlake); return {std::move(flakeRef), fragment, extendedOutputsSpec}; } diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index a08fef011..9cff4b365 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -53,7 +53,7 @@ StringMap EvalState::realiseContext(const PathSet & context) [&](const NixStringContextElem::Built & b) { drvs.push_back(DerivedPath::Built { .drvPath = b.drvPath, - .outputs = std::set { b.output }, + .outputs = OutputsSpec::Names { b.output }, }); ensureValid(b.drvPath); }, @@ -84,16 +84,12 @@ StringMap EvalState::realiseContext(const PathSet & context) store->buildPaths(buildReqs); /* Get all the output paths corresponding to the placeholders we had */ - for (auto & [drvPath, outputs] : drvs) { - const auto outputPaths = store->queryDerivationOutputMap(drvPath); - for (auto & outputName : outputs) { - auto outputPath = get(outputPaths, outputName); - if (!outputPath) - debugThrowLastTrace(Error("derivation '%s' does not have an output named '%s'", - store->printStorePath(drvPath), outputName)); + for (auto & drv : drvs) { + auto outputs = resolveDerivedPath(*store, drv); + for (auto & [outputName, outputPath] : outputs) { res.insert_or_assign( - downstreamPlaceholder(*store, drvPath, outputName), - store->printStorePath(*outputPath) + downstreamPlaceholder(*store, drv.drvPath, outputName), + store->printStorePath(outputPath) ); } } diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 5e86b5269..98c1ddaae 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -63,7 +63,7 @@ namespace nix { DerivationGoal::DerivationGoal(const StorePath & drvPath, - const StringSet & wantedOutputs, Worker & worker, BuildMode buildMode) + const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode) : Goal(worker, DerivedPath::Built { .drvPath = drvPath, .outputs = wantedOutputs }) , useDerivation(true) , drvPath(drvPath) @@ -82,7 +82,7 @@ DerivationGoal::DerivationGoal(const StorePath & drvPath, DerivationGoal::DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv, - const StringSet & wantedOutputs, Worker & worker, BuildMode buildMode) + const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode) : Goal(worker, DerivedPath::Built { .drvPath = drvPath, .outputs = wantedOutputs }) , useDerivation(false) , drvPath(drvPath) @@ -142,18 +142,11 @@ void DerivationGoal::work() (this->*state)(); } -void DerivationGoal::addWantedOutputs(const StringSet & outputs) +void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs) { - /* If we already want all outputs, there is nothing to do. */ - if (wantedOutputs.empty()) return; - - if (outputs.empty()) { - wantedOutputs.clear(); + bool newOutputs = wantedOutputs.merge(outputs); + if (newOutputs) needRestart = true; - } else - for (auto & i : outputs) - if (wantedOutputs.insert(i).second) - needRestart = true; } @@ -390,7 +383,7 @@ void DerivationGoal::repairClosure() auto outputs = queryDerivationOutputMap(); StorePathSet outputClosure; for (auto & i : outputs) { - if (!wantOutput(i.first, wantedOutputs)) continue; + if (!wantedOutputs.contains(i.first)) continue; worker.store.computeFSClosure(i.second, outputClosure); } @@ -422,7 +415,7 @@ void DerivationGoal::repairClosure() if (drvPath2 == outputsToDrv.end()) addWaitee(upcast_goal(worker.makePathSubstitutionGoal(i, Repair))); else - addWaitee(worker.makeDerivationGoal(drvPath2->second, StringSet(), bmRepair)); + addWaitee(worker.makeDerivationGoal(drvPath2->second, OutputsSpec::All(), bmRepair)); } if (waitees.empty()) { @@ -991,10 +984,15 @@ void DerivationGoal::resolvedFinished() StorePathSet outputPaths; - // `wantedOutputs` might be empty, which means “all the outputs” - auto realWantedOutputs = wantedOutputs; - if (realWantedOutputs.empty()) - realWantedOutputs = resolvedDrv.outputNames(); + // `wantedOutputs` might merely indicate “all the outputs” + auto realWantedOutputs = std::visit(overloaded { + [&](const OutputsSpec::All &) { + return resolvedDrv.outputNames(); + }, + [&](const OutputsSpec::Names & names) { + return names; + }, + }, wantedOutputs.raw()); for (auto & wantedOutput : realWantedOutputs) { auto initialOutput = get(initialOutputs, wantedOutput); @@ -1322,7 +1320,14 @@ std::pair DerivationGoal::checkPathValidity() if (!drv->type().isPure()) return { false, {} }; bool checkHash = buildMode == bmRepair; - auto wantedOutputsLeft = wantedOutputs; + auto wantedOutputsLeft = std::visit(overloaded { + [&](const OutputsSpec::All &) { + return StringSet {}; + }, + [&](const OutputsSpec::Names & names) { + return names; + }, + }, wantedOutputs.raw()); DrvOutputs validOutputs; for (auto & i : queryPartialDerivationOutputMap()) { @@ -1331,7 +1336,7 @@ std::pair DerivationGoal::checkPathValidity() // this is an invalid output, gets catched with (!wantedOutputsLeft.empty()) continue; auto & info = *initialOutput; - info.wanted = wantOutput(i.first, wantedOutputs); + info.wanted = wantedOutputs.contains(i.first); if (info.wanted) wantedOutputsLeft.erase(i.first); if (i.second) { @@ -1369,7 +1374,7 @@ std::pair DerivationGoal::checkPathValidity() validOutputs.emplace(drvOutput, Realisation { drvOutput, info.known->path }); } - // If we requested all the outputs via the empty set, we are always fine. + // If we requested all the outputs, we are always fine. // If we requested specific elements, the loop above removes all the valid // ones, so any that are left must be invalid. if (!wantedOutputsLeft.empty()) diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index d33e04cbc..707e38b4b 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -2,6 +2,7 @@ #include "parsed-derivations.hh" #include "lock.hh" +#include "outputs-spec.hh" #include "store-api.hh" #include "pathlocks.hh" #include "goal.hh" @@ -55,7 +56,7 @@ struct DerivationGoal : public Goal /* The specific outputs that we need to build. Empty means all of them. */ - StringSet wantedOutputs; + OutputsSpec wantedOutputs; /* Mapping from input derivations + output names to actual store paths. This is filled in by waiteeDone() as each dependency @@ -128,10 +129,10 @@ struct DerivationGoal : public Goal std::string machineName; DerivationGoal(const StorePath & drvPath, - const StringSet & wantedOutputs, Worker & worker, + const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode = bmNormal); DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv, - const StringSet & wantedOutputs, Worker & worker, + const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode = bmNormal); virtual ~DerivationGoal(); @@ -142,7 +143,7 @@ struct DerivationGoal : public Goal void work() override; /* Add wanted outputs to an already existing derivation goal. */ - void addWantedOutputs(const StringSet & outputs); + void addWantedOutputs(const OutputsSpec & outputs); /* The states. */ void getDerivation(); diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index e1b80165e..df7722733 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -130,7 +130,8 @@ void LocalStore::repairPath(const StorePath & path) auto info = queryPathInfo(path); if (info->deriver && isValidPath(*info->deriver)) { goals.clear(); - goals.insert(worker.makeDerivationGoal(*info->deriver, StringSet(), bmRepair)); + // FIXME: Should just build the specific output we need. + goals.insert(worker.makeDerivationGoal(*info->deriver, OutputsSpec::All { }, bmRepair)); worker.run(goals); } else throw Error(worker.exitStatus(), "cannot repair path '%s'", printStorePath(path)); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 488e06d8c..09cb6b233 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2735,7 +2735,7 @@ DrvOutputs LocalDerivationGoal::registerOutputs() signRealisation(thisRealisation); worker.store.registerDrvOutput(thisRealisation); } - if (wantOutput(outputName, wantedOutputs)) + if (wantedOutputs.contains(outputName)) builtOutputs.emplace(thisRealisation.id, thisRealisation); } diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index b192fbc77..b94fb8416 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -42,7 +42,7 @@ Worker::~Worker() std::shared_ptr Worker::makeDerivationGoalCommon( const StorePath & drvPath, - const StringSet & wantedOutputs, + const OutputsSpec & wantedOutputs, std::function()> mkDrvGoal) { std::weak_ptr & goal_weak = derivationGoals[drvPath]; @@ -59,7 +59,7 @@ std::shared_ptr Worker::makeDerivationGoalCommon( std::shared_ptr Worker::makeDerivationGoal(const StorePath & drvPath, - const StringSet & wantedOutputs, BuildMode buildMode) + const OutputsSpec & wantedOutputs, BuildMode buildMode) { return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() -> std::shared_ptr { return !dynamic_cast(&store) @@ -70,7 +70,7 @@ std::shared_ptr Worker::makeDerivationGoal(const StorePath & drv std::shared_ptr Worker::makeBasicDerivationGoal(const StorePath & drvPath, - const BasicDerivation & drv, const StringSet & wantedOutputs, BuildMode buildMode) + const BasicDerivation & drv, const OutputsSpec & wantedOutputs, BuildMode buildMode) { return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() -> std::shared_ptr { return !dynamic_cast(&store) diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index a1e036a96..6d68d3cf1 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -140,15 +140,15 @@ public: /* derivation goal */ private: std::shared_ptr makeDerivationGoalCommon( - const StorePath & drvPath, const StringSet & wantedOutputs, + const StorePath & drvPath, const OutputsSpec & wantedOutputs, std::function()> mkDrvGoal); public: std::shared_ptr makeDerivationGoal( const StorePath & drvPath, - const StringSet & wantedOutputs, BuildMode buildMode = bmNormal); + const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal); std::shared_ptr makeBasicDerivationGoal( const StorePath & drvPath, const BasicDerivation & drv, - const StringSet & wantedOutputs, BuildMode buildMode = bmNormal); + const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal); /* substitution goal */ std::shared_ptr makePathSubstitutionGoal(const StorePath & storePath, RepairFlag repair = NoRepair, std::optional ca = std::nullopt); diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 42a53912e..cf18724ef 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -688,12 +688,6 @@ std::map staticOutputHashes(Store & store, const Derivation & } -bool wantOutput(const std::string & output, const std::set & wanted) -{ - return wanted.empty() || wanted.find(output) != wanted.end(); -} - - static DerivationOutput readDerivationOutput(Source & in, const Store & store) { const auto pathS = readString(in); diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index f3cd87fb1..7ee3ded6a 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -294,8 +294,6 @@ typedef std::map DrvHashes; // FIXME: global, though at least thread-safe. extern Sync drvHashes; -bool wantOutput(const std::string & output, const std::set & wanted); - struct Source; struct Sink; diff --git a/src/libstore/derived-path.cc b/src/libstore/derived-path.cc index 3fa5ae4f7..e0d86a42f 100644 --- a/src/libstore/derived-path.cc +++ b/src/libstore/derived-path.cc @@ -19,11 +19,11 @@ nlohmann::json DerivedPath::Built::toJSON(ref store) const { res["drvPath"] = store->printStorePath(drvPath); // Fallback for the input-addressed derivation case: We expect to always be // able to print the output paths, so let’s do it - const auto knownOutputs = store->queryPartialDerivationOutputMap(drvPath); - for (const auto & output : outputs) { - auto knownOutput = get(knownOutputs, output); - if (knownOutput && *knownOutput) - res["outputs"][output] = store->printStorePath(**knownOutput); + const auto outputMap = store->queryPartialDerivationOutputMap(drvPath); + for (const auto & [output, outputPathOpt] : outputMap) { + if (!outputs.contains(output)) continue; + if (outputPathOpt) + res["outputs"][output] = store->printStorePath(*outputPathOpt); else res["outputs"][output] = nullptr; } @@ -63,7 +63,7 @@ std::string DerivedPath::Built::to_string(const Store & store) const { return store.printStorePath(drvPath) + "!" - + (outputs.empty() ? std::string { "*" } : concatStringsSep(",", outputs)); + + outputs.to_string(); } std::string DerivedPath::to_string(const Store & store) const @@ -81,15 +81,10 @@ DerivedPath::Opaque DerivedPath::Opaque::parse(const Store & store, std::string_ DerivedPath::Built DerivedPath::Built::parse(const Store & store, std::string_view drvS, std::string_view outputsS) { - auto drvPath = store.parseStorePath(drvS); - std::set outputs; - if (outputsS != "*") { - outputs = tokenizeString>(outputsS, ","); - if (outputs.empty()) - throw Error( - "Explicit list of wanted outputs '%s' must not be empty. Consider using '*' as a wildcard meaning all outputs if no output in particular is wanted.", outputsS); - } - return {drvPath, outputs}; + return { + .drvPath = store.parseStorePath(drvS), + .outputs = OutputsSpec::parse(outputsS), + }; } DerivedPath DerivedPath::parse(const Store & store, std::string_view s) diff --git a/src/libstore/derived-path.hh b/src/libstore/derived-path.hh index 706e5dcb4..4edff7467 100644 --- a/src/libstore/derived-path.hh +++ b/src/libstore/derived-path.hh @@ -3,6 +3,7 @@ #include "util.hh" #include "path.hh" #include "realisation.hh" +#include "outputs-spec.hh" #include @@ -44,7 +45,7 @@ struct DerivedPathOpaque { */ struct DerivedPathBuilt { StorePath drvPath; - std::set outputs; + OutputsSpec outputs; std::string to_string(const Store & store) const; static DerivedPathBuilt parse(const Store & store, std::string_view, std::string_view); diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index fb985c97b..1ff855cd0 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -185,7 +185,7 @@ void Store::queryMissing(const std::vector & targets, knownOutputPaths = false; break; } - if (wantOutput(outputName, bfd.outputs) && !isValidPath(*pathOpt)) + if (bfd.outputs.contains(outputName) && !isValidPath(*pathOpt)) invalid.insert(*pathOpt); } if (knownOutputPaths && invalid.empty()) return; @@ -301,4 +301,47 @@ std::map drvOutputReferences( return drvOutputReferences(Realisation::closure(store, inputRealisations), info->references); } +OutputPathMap resolveDerivedPath(Store & store, const DerivedPath::Built & bfd, Store * evalStore_) +{ + auto & evalStore = evalStore_ ? *evalStore_ : store; + + OutputPathMap outputs; + auto drv = evalStore.readDerivation(bfd.drvPath); + auto outputHashes = staticOutputHashes(store, drv); + auto drvOutputs = drv.outputsAndOptPaths(store); + auto outputNames = std::visit(overloaded { + [&](const OutputsSpec::All &) { + StringSet names; + for (auto & [outputName, _] : drv.outputs) + names.insert(outputName); + return names; + }, + [&](const OutputsSpec::Names & names) { + return names; + }, + }, bfd.outputs); + for (auto & output : outputNames) { + auto outputHash = get(outputHashes, output); + if (!outputHash) + throw Error( + "the derivation '%s' doesn't have an output named '%s'", + store.printStorePath(bfd.drvPath), output); + if (settings.isExperimentalFeatureEnabled(Xp::CaDerivations)) { + DrvOutput outputId { *outputHash, output }; + auto realisation = store.queryRealisation(outputId); + if (!realisation) + throw MissingRealisation(outputId); + outputs.insert_or_assign(output, realisation->outPath); + } else { + // If ca-derivations isn't enabled, assume that + // the output path is statically known. + auto drvOutput = get(drvOutputs, output); + assert(drvOutput); + assert(drvOutput->second); + outputs.insert_or_assign(output, *drvOutput->second); + } + } + return outputs; +} + } diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index c446976bc..e7bd8ebd8 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -6,57 +6,153 @@ namespace nix { -std::pair ExtendedOutputsSpec::parse(std::string s) +bool OutputsSpec::contains(const std::string & outputName) const { - static std::regex regex(R"((.*)\^((\*)|([a-z]+(,[a-z]+)*)))"); + return std::visit(overloaded { + [&](const OutputsSpec::All &) { + return true; + }, + [&](const OutputsSpec::Names & outputNames) { + return outputNames.count(outputName) > 0; + }, + }, raw()); +} + + +std::optional OutputsSpec::parseOpt(std::string_view s) +{ + static std::regex regex(R"((\*)|([a-z]+(,[a-z]+)*))"); std::smatch match; - if (!std::regex_match(s, match, regex)) - return {s, DefaultOutputs()}; + std::string s2 { s }; // until some improves std::regex + if (!std::regex_match(s2, match, regex)) + return std::nullopt; - if (match[3].matched) - return {match[1], AllOutputs()}; + if (match[1].matched) + return { OutputsSpec::All {} }; - return {match[1], tokenizeString(match[4].str(), ",")}; + if (match[2].matched) + return { tokenizeString(match[2].str(), ",") }; + + assert(false); } + +OutputsSpec OutputsSpec::parse(std::string_view s) +{ + std::optional spec = OutputsSpec::parseOpt(s); + if (!spec) + throw Error("Invalid outputs specifier: '%s'", s); + return *spec; +} + + +std::pair ExtendedOutputsSpec::parse(std::string_view s) +{ + auto found = s.rfind('^'); + + if (found == std::string::npos) + return { s, ExtendedOutputsSpec::Default {} }; + + auto spec = OutputsSpec::parse(s.substr(found + 1)); + return { s.substr(0, found), ExtendedOutputsSpec::Explicit { spec } }; +} + + +std::string OutputsSpec::to_string() const +{ + return std::visit(overloaded { + [&](const OutputsSpec::All &) -> std::string { + return "*"; + }, + [&](const OutputsSpec::Names & outputNames) -> std::string { + return concatStringsSep(",", outputNames); + }, + }, raw()); +} + + std::string ExtendedOutputsSpec::to_string() const { return std::visit(overloaded { [&](const ExtendedOutputsSpec::Default &) -> std::string { return ""; }, - [&](const ExtendedOutputsSpec::All &) -> std::string { - return "*"; - }, - [&](const ExtendedOutputsSpec::Names & outputNames) -> std::string { - return "^" + concatStringsSep(",", outputNames); + [&](const ExtendedOutputsSpec::Explicit & outputSpec) -> std::string { + return "^" + outputSpec.to_string(); }, }, raw()); } + +bool OutputsSpec::merge(const OutputsSpec & that) +{ + return std::visit(overloaded { + [&](OutputsSpec::All &) { + /* If we already refer to all outputs, there is nothing to do. */ + return false; + }, + [&](OutputsSpec::Names & theseNames) { + return std::visit(overloaded { + [&](const OutputsSpec::All &) { + *this = OutputsSpec::All {}; + return true; + }, + [&](const OutputsSpec::Names & thoseNames) { + bool ret = false; + for (auto & i : thoseNames) + if (theseNames.insert(i).second) + ret = true; + return ret; + }, + }, that.raw()); + }, + }, raw()); +} + + +void to_json(nlohmann::json & json, const OutputsSpec & outputsSpec) +{ + std::visit(overloaded { + [&](const OutputsSpec::All &) { + json = std::vector({"*"}); + }, + [&](const OutputsSpec::Names & names) { + json = names; + }, + }, outputsSpec); +} + + void to_json(nlohmann::json & json, const ExtendedOutputsSpec & extendedOutputsSpec) { - if (std::get_if(&extendedOutputsSpec)) - json = nullptr; - - else if (std::get_if(&extendedOutputsSpec)) - json = std::vector({"*"}); - - else if (auto outputNames = std::get_if(&extendedOutputsSpec)) - json = *outputNames; + std::visit(overloaded { + [&](const ExtendedOutputsSpec::Default &) { + json = nullptr; + }, + [&](const ExtendedOutputsSpec::Explicit & e) { + to_json(json, e); + }, + }, extendedOutputsSpec); } + +void from_json(const nlohmann::json & json, OutputsSpec & outputsSpec) +{ + auto names = json.get(); + if (names == OutputNames({"*"})) + outputsSpec = OutputsSpec::All {}; + else + outputsSpec = names; +} + + void from_json(const nlohmann::json & json, ExtendedOutputsSpec & extendedOutputsSpec) { if (json.is_null()) - extendedOutputsSpec = DefaultOutputs(); + extendedOutputsSpec = ExtendedOutputsSpec::Default {}; else { - auto names = json.get(); - if (names == OutputNames({"*"})) - extendedOutputsSpec = AllOutputs(); - else - extendedOutputsSpec = names; + extendedOutputsSpec = ExtendedOutputsSpec::Explicit { json.get() }; } } diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index 5ed711a62..e81695da9 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -1,5 +1,6 @@ #pragma once +#include #include #include @@ -13,31 +14,66 @@ struct AllOutputs { bool operator < (const AllOutputs & _) const { return false; } }; +typedef std::variant _OutputsSpecRaw; + +struct OutputsSpec : _OutputsSpecRaw { + using Raw = _OutputsSpecRaw; + using Raw::Raw; + + using Names = OutputNames; + using All = AllOutputs; + + inline const Raw & raw() const { + return static_cast(*this); + } + + inline Raw & raw() { + return static_cast(*this); + } + + bool contains(const std::string & output) const; + + /* Modify the receiver outputs spec so it is the union of it's old value + and the argument. Return whether the output spec needed to be modified + --- if it didn't it was already "large enough". */ + bool merge(const OutputsSpec & outputs); + + /* Parse a string of the form 'output1,...outputN' or + '*', returning the outputs spec. */ + static OutputsSpec parse(std::string_view s); + static std::optional parseOpt(std::string_view s); + + std::string to_string() const; +}; + struct DefaultOutputs { bool operator < (const DefaultOutputs & _) const { return false; } }; -typedef std::variant _ExtendedOutputsSpecRaw; +typedef std::variant _ExtendedOutputsSpecRaw; struct ExtendedOutputsSpec : _ExtendedOutputsSpecRaw { using Raw = _ExtendedOutputsSpecRaw; using Raw::Raw; - using Names = OutputNames; - using All = AllOutputs; using Default = DefaultOutputs; + using Explicit = OutputsSpec; inline const Raw & raw() const { return static_cast(*this); } /* Parse a string of the form 'prefix^output1,...outputN' or - 'prefix^*', returning the prefix and the outputs spec. */ - static std::pair parse(std::string s); + 'prefix^*', returning the prefix and the extended outputs spec. */ + static std::pair parse(std::string_view s); std::string to_string() const; }; + +void to_json(nlohmann::json &, const OutputsSpec &); +void from_json(const nlohmann::json &, OutputsSpec &); + void to_json(nlohmann::json &, const ExtendedOutputsSpec &); void from_json(const nlohmann::json &, ExtendedOutputsSpec &); diff --git a/src/libstore/path-with-outputs.cc b/src/libstore/path-with-outputs.cc index 17230ffc4..10e0cc63e 100644 --- a/src/libstore/path-with-outputs.cc +++ b/src/libstore/path-with-outputs.cc @@ -15,10 +15,14 @@ std::string StorePathWithOutputs::to_string(const Store & store) const DerivedPath StorePathWithOutputs::toDerivedPath() const { - if (!outputs.empty() || path.isDerivation()) - return DerivedPath::Built { path, outputs }; - else + if (!outputs.empty()) { + return DerivedPath::Built { path, OutputsSpec::Names { outputs } }; + } else if (path.isDerivation()) { + assert(outputs.empty()); + return DerivedPath::Built { path, OutputsSpec::All { } }; + } else { return DerivedPath::Opaque { path }; + } } @@ -41,7 +45,18 @@ std::variant StorePathWithOutputs::tryFromDeriv return StorePathWithOutputs { bo.path }; }, [&](const DerivedPath::Built & bfd) -> std::variant { - return StorePathWithOutputs { bfd.drvPath, bfd.outputs }; + return StorePathWithOutputs { + .path = bfd.drvPath, + // Use legacy encoding of wildcard as empty set + .outputs = std::visit(overloaded { + [&](const OutputsSpec::All &) -> StringSet { + return {}; + }, + [&](const OutputsSpec::Names & outputs) { + return outputs; + }, + }, bfd.outputs.raw()), + }; }, }, p.raw()); } diff --git a/src/libstore/path.hh b/src/libstore/path.hh index 77fd0f8dc..0694b4c18 100644 --- a/src/libstore/path.hh +++ b/src/libstore/path.hh @@ -64,7 +64,6 @@ public: typedef std::set StorePathSet; typedef std::vector StorePaths; -typedef std::map OutputPathMap; typedef std::map> StorePathCAMap; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index ccf7d7e8b..832be08f7 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -867,8 +867,8 @@ std::vector RemoteStore::buildPathsWithResults( OutputPathMap outputs; auto drv = evalStore->readDerivation(bfd.drvPath); const auto outputHashes = staticOutputHashes(*evalStore, drv); // FIXME: expensive - const auto drvOutputs = drv.outputsAndOptPaths(*this); - for (auto & output : bfd.outputs) { + auto built = resolveDerivedPath(*this, bfd, &*evalStore); + for (auto & [output, outputPath] : built) { auto outputHash = get(outputHashes, output); if (!outputHash) throw Error( @@ -882,16 +882,11 @@ std::vector RemoteStore::buildPathsWithResults( throw MissingRealisation(outputId); res.builtOutputs.emplace(realisation->id, *realisation); } else { - // If ca-derivations isn't enabled, assume that - // the output path is statically known. - const auto drvOutput = get(drvOutputs, output); - assert(drvOutput); - assert(drvOutput->second); res.builtOutputs.emplace( outputId, Realisation { .id = outputId, - .outPath = *drvOutput->second, + .outPath = outputPath, }); } } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 4a88d7216..ad3a7c8c5 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -71,6 +71,9 @@ class NarInfoDiskCache; class Store; +typedef std::map OutputPathMap; + + enum CheckSigsFlag : bool { NoCheckSigs = false, CheckSigs = true }; enum SubstituteFlag : bool { NoSubstitute = false, Substitute = true }; enum AllowInvalidFlag : bool { DisallowInvalid = false, AllowInvalid = true }; @@ -120,6 +123,8 @@ public: typedef std::map Params; + + protected: struct PathInfoCacheValue { @@ -719,6 +724,11 @@ void copyClosure( void removeTempRoots(); +/* Resolve the derived path completely, failing if any derivation output + is unknown. */ +OutputPathMap resolveDerivedPath(Store &, const DerivedPath::Built &, Store * evalStore = nullptr); + + /* Return a Store object to access the Nix store denoted by ‘uri’ (slight misnomer...). Supported values are: diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index a3dd42341..03259e7c7 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -4,42 +4,62 @@ namespace nix { +TEST(OutputsSpec_parse, basic) +{ + { + auto outputsSpec = OutputsSpec::parse("*"); + ASSERT_TRUE(std::get_if(&outputsSpec)); + } + + { + auto outputsSpec = OutputsSpec::parse("out"); + ASSERT_TRUE(std::get(outputsSpec) == OutputsSpec::Names({"out"})); + } + + { + auto outputsSpec = OutputsSpec::parse("out,bin"); + ASSERT_TRUE(std::get(outputsSpec) == OutputsSpec::Names({"out", "bin"})); + } + + { + std::optional outputsSpecOpt = OutputsSpec::parseOpt("&*()"); + ASSERT_FALSE(outputsSpecOpt); + } +} + + TEST(ExtendedOutputsSpec_parse, basic) { { auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); + ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); } { auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^*"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); + auto * explicit_p = std::get_if(&extendedOutputsSpec); + ASSERT_TRUE(explicit_p); + ASSERT_TRUE(std::get_if(explicit_p)); } { auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get(extendedOutputsSpec) == OutputNames({"out"})); + ASSERT_TRUE(std::get(std::get(extendedOutputsSpec)) == OutputsSpec::Names({"out"})); } { auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out,bin"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get(extendedOutputsSpec) == OutputNames({"out", "bin"})); + ASSERT_TRUE(std::get(std::get(extendedOutputsSpec)) == OutputsSpec::Names({"out", "bin"})); } { auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^bar^out,bin"); ASSERT_EQ(prefix, "foo^bar"); - ASSERT_TRUE(std::get(extendedOutputsSpec) == OutputNames({"out", "bin"})); - } - - { - auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^&*()"); - ASSERT_EQ(prefix, "foo^&*()"); - ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); + ASSERT_TRUE(std::get(std::get(extendedOutputsSpec)) == OutputsSpec::Names({"out", "bin"})); } } diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index adcaab686..0a7a21de2 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -397,7 +397,7 @@ static void main_nix_build(int argc, char * * argv) auto bashDrv = drv->requireDrvPath(); pathsToBuild.push_back(DerivedPath::Built { .drvPath = bashDrv, - .outputs = {"out"}, + .outputs = OutputsSpec::Names {"out"}, }); pathsToCopy.insert(bashDrv); shellDrv = bashDrv; @@ -591,7 +591,7 @@ static void main_nix_build(int argc, char * * argv) if (outputName == "") throw Error("derivation '%s' lacks an 'outputName' attribute", store->printStorePath(drvPath)); - pathsToBuild.push_back(DerivedPath::Built{drvPath, {outputName}}); + pathsToBuild.push_back(DerivedPath::Built{drvPath, OutputsSpec::Names{outputName}}); pathsToBuildOrdered.push_back({drvPath, {outputName}}); drvsToCopy.insert(drvPath); diff --git a/src/nix/app.cc b/src/nix/app.cc index c9637dcf5..08cd0ccd4 100644 --- a/src/nix/app.cc +++ b/src/nix/app.cc @@ -86,13 +86,13 @@ UnresolvedApp Installable::toApp(EvalState & state) /* We want all outputs of the drv */ return DerivedPath::Built { .drvPath = d.drvPath, - .outputs = {}, + .outputs = OutputsSpec::All {}, }; }, [&](const NixStringContextElem::Built & b) -> DerivedPath { return DerivedPath::Built { .drvPath = b.drvPath, - .outputs = { b.output }, + .outputs = OutputsSpec::Names { b.output }, }; }, [&](const NixStringContextElem::Opaque & o) -> DerivedPath { @@ -127,7 +127,7 @@ UnresolvedApp Installable::toApp(EvalState & state) return UnresolvedApp { App { .context = { DerivedPath::Built { .drvPath = drvPath, - .outputs = {outputName}, + .outputs = OutputsSpec::Names { outputName }, } }, .program = program, }}; From 8a3b1b7ced3e00d29a0274dde110801dea4a1e0e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 13 Dec 2022 13:00:34 -0500 Subject: [PATCH 30/41] Simplify and document store path installable parsing --- src/libcmd/installables.cc | 66 ++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 58b5ef9b9..c791eef39 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -402,18 +402,6 @@ struct InstallableStorePath : Installable ref store; DerivedPath req; - InstallableStorePath(ref store, StorePath && storePath) - : store(store), - req(storePath.isDerivation() - ? (DerivedPath) DerivedPath::Built { - .drvPath = std::move(storePath), - .outputs = {}, - } - : (DerivedPath) DerivedPath::Opaque { - .path = std::move(storePath), - }) - { } - InstallableStorePath(ref store, DerivedPath && req) : store(store), req(std::move(req)) { } @@ -814,24 +802,46 @@ std::vector> SourceExprCommand::parseInstallables( for (auto & s : ss) { std::exception_ptr ex; - auto found = s.rfind('^'); - if (found != std::string::npos) { - try { - result.push_back(std::make_shared( - store, - DerivedPath::Built::parse(*store, s.substr(0, found), s.substr(found + 1)))); - continue; - } catch (BadStorePath &) { - } catch (...) { - if (!ex) - ex = std::current_exception(); - } - } + auto [prefix_, extendedOutputsSpec_] = ExtendedOutputsSpec::parse(s); + // To avoid clang's pedantry + auto prefix = std::move(prefix_); + auto extendedOutputsSpec = std::move(extendedOutputsSpec_); - found = s.find('/'); + auto found = prefix.find('/'); if (found != std::string::npos) { try { - result.push_back(std::make_shared(store, store->followLinksToStorePath(s))); + auto derivedPath = std::visit(overloaded { + // If the user did not use ^, we treat the output more liberally. + [&](const ExtendedOutputsSpec::Default &) -> DerivedPath { + // First, we accept a symlink chain or an actual store path. + auto storePath = store->followLinksToStorePath(prefix); + // Second, we see if the store path ends in `.drv` to decide what sort + // of derived path they want. + // + // This handling predates the `^` syntax. The `^*` in + // `/nix/store/hash-foo.drv^*` unambiguously means "do the + // `DerivedPath::Built` case", so plain `/nix/store/hash-foo.drv` could + // also unambiguously mean "do the DerivedPath::Opaque` case". + // + // Issue #7261 tracks reconsidering this `.drv` dispatching. + return storePath.isDerivation() + ? (DerivedPath) DerivedPath::Built { + .drvPath = std::move(storePath), + .outputs = {}, + } + : (DerivedPath) DerivedPath::Opaque { + .path = std::move(storePath), + }; + }, + // If the user did use ^, we just do exactly what is written. + [&](const ExtendedOutputsSpec::Explicit & outputSpec) -> DerivedPath { + return DerivedPath::Built { + .drvPath = store->parseStorePath(prefix), + .outputs = outputSpec, + }; + }, + }, extendedOutputsSpec.raw()); + result.push_back(std::make_shared(store, std::move(derivedPath))); continue; } catch (BadStorePath &) { } catch (...) { @@ -841,7 +851,7 @@ std::vector> SourceExprCommand::parseInstallables( } try { - auto [flakeRef, fragment, extendedOutputsSpec] = parseFlakeRefWithFragmentAndExtendedOutputsSpec(s, absPath(".")); + auto [flakeRef, fragment] = parseFlakeRefWithFragment(std::string { prefix }, absPath(".")); result.push_back(std::make_shared( this, getEvalState(), From 114a6e2b09eda7f23e7776e1cdf77715044e073e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 11:54:43 -0500 Subject: [PATCH 31/41] Make it hard to construct an empty `OutputsSpec::Names` This should be a non-empty set, and so we don't want people doing this by accident. We remove the zero-0 constructor with a little inheritance trickery. --- src/libcmd/installables.cc | 2 +- src/libstore/build/derivation-goal.cc | 4 ++-- src/libstore/misc.cc | 2 +- src/libstore/outputs-spec.cc | 8 ++++---- src/libstore/outputs-spec.hh | 17 ++++++++++++++++- src/libstore/path-with-outputs.cc | 2 +- src/nix-build/nix-build.cc | 2 +- 7 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index c791eef39..3457f2e47 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -479,7 +479,7 @@ struct InstallableAttrPath : InstallableValue auto derivedPath = byDrvPath.emplace(*drvPath, DerivedPath::Built { .drvPath = *drvPath, // Not normally legal, but we will merge right below - .outputs = OutputsSpec::Names { }, + .outputs = OutputsSpec::Names { StringSet { } }, }).first; derivedPath->second.outputs.merge(std::visit(overloaded { diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 98c1ddaae..61169635a 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -990,7 +990,7 @@ void DerivationGoal::resolvedFinished() return resolvedDrv.outputNames(); }, [&](const OutputsSpec::Names & names) { - return names; + return static_cast>(names); }, }, wantedOutputs.raw()); @@ -1325,7 +1325,7 @@ std::pair DerivationGoal::checkPathValidity() return StringSet {}; }, [&](const OutputsSpec::Names & names) { - return names; + return static_cast(names); }, }, wantedOutputs.raw()); DrvOutputs validOutputs; diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 1ff855cd0..5758c3d93 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -317,7 +317,7 @@ OutputPathMap resolveDerivedPath(Store & store, const DerivedPath::Built & bfd, return names; }, [&](const OutputsSpec::Names & names) { - return names; + return static_cast>(names); }, }, bfd.outputs); for (auto & output : outputNames) { diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index e7bd8ebd8..891252990 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -32,7 +32,7 @@ std::optional OutputsSpec::parseOpt(std::string_view s) return { OutputsSpec::All {} }; if (match[2].matched) - return { tokenizeString(match[2].str(), ",") }; + return OutputsSpec::Names { tokenizeString(match[2].str(), ",") }; assert(false); } @@ -139,11 +139,11 @@ void to_json(nlohmann::json & json, const ExtendedOutputsSpec & extendedOutputsS void from_json(const nlohmann::json & json, OutputsSpec & outputsSpec) { - auto names = json.get(); - if (names == OutputNames({"*"})) + auto names = json.get(); + if (names == StringSet({"*"})) outputsSpec = OutputsSpec::All {}; else - outputsSpec = names; + outputsSpec = OutputsSpec::Names { std::move(names) }; } diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index e81695da9..9c477ee2b 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -8,7 +8,22 @@ namespace nix { -typedef std::set OutputNames; +struct OutputNames : std::set { + using std::set::set; + + // These need to be "inherited manually" + OutputNames(const std::set & s) + : std::set(s) + { } + OutputNames(std::set && s) + : std::set(s) + { } + + /* This set should always be non-empty, so we delete this + constructor in order make creating empty ones by mistake harder. + */ + OutputNames() = delete; +}; struct AllOutputs { bool operator < (const AllOutputs & _) const { return false; } diff --git a/src/libstore/path-with-outputs.cc b/src/libstore/path-with-outputs.cc index 10e0cc63e..6c0704ed9 100644 --- a/src/libstore/path-with-outputs.cc +++ b/src/libstore/path-with-outputs.cc @@ -53,7 +53,7 @@ std::variant StorePathWithOutputs::tryFromDeriv return {}; }, [&](const OutputsSpec::Names & outputs) { - return outputs; + return static_cast(outputs); }, }, bfd.outputs.raw()), }; diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 0a7a21de2..049838bb1 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -421,7 +421,7 @@ static void main_nix_build(int argc, char * * argv) { pathsToBuild.push_back(DerivedPath::Built { .drvPath = inputDrv, - .outputs = inputOutputs + .outputs = OutputsSpec::Names { inputOutputs }, }); pathsToCopy.insert(inputDrv); } From 5ba6e5d0d9bed2806ddb59c8a3305b3cb5784d53 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 16:32:30 -0500 Subject: [PATCH 32/41] Remove default constructor from `OutputsSpec` This forces us to be explicit. It also requires to rework how `from_json` works. A `JSON_IMPL` is added to assist with this. --- src/libcmd/installables.cc | 2 +- src/libcmd/repl.cc | 7 +++- src/libstore/build/entry-points.cc | 7 ++-- src/libstore/legacy-ssh-store.cc | 7 +++- src/libstore/outputs-spec.cc | 53 +++++++++++++++--------------- src/libstore/outputs-spec.hh | 15 ++++----- src/libstore/remote-store.cc | 7 +++- src/libutil/json-impls.hh | 14 ++++++++ src/nix-env/nix-env.cc | 18 +++++++--- src/nix/bundle.cc | 7 +++- src/nix/develop.cc | 7 +++- src/nix/flake.cc | 8 +++-- 12 files changed, 103 insertions(+), 49 deletions(-) create mode 100644 src/libutil/json-impls.hh diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 3457f2e47..d73109873 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -827,7 +827,7 @@ std::vector> SourceExprCommand::parseInstallables( return storePath.isDerivation() ? (DerivedPath) DerivedPath::Built { .drvPath = std::move(storePath), - .outputs = {}, + .outputs = OutputsSpec::All {}, } : (DerivedPath) DerivedPath::Opaque { .path = std::move(storePath), diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 71a7e079a..9b12f8fa2 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -641,7 +641,12 @@ bool NixRepl::processLine(std::string line) Path drvPathRaw = state->store->printStorePath(drvPath); if (command == ":b" || command == ":bl") { - state->store->buildPaths({DerivedPath::Built{drvPath}}); + state->store->buildPaths({ + DerivedPath::Built { + .drvPath = drvPath, + .outputs = OutputsSpec::All { }, + }, + }); auto drv = state->store->readDerivation(drvPath); logger->cout("\nThis derivation produced the following outputs:"); for (auto & [outputName, outputPath] : state->store->queryDerivationOutputMap(drvPath)) { diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index df7722733..2925fe3ca 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -80,7 +80,7 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat BuildMode buildMode) { Worker worker(*this, *this); - auto goal = worker.makeBasicDerivationGoal(drvPath, drv, {}, buildMode); + auto goal = worker.makeBasicDerivationGoal(drvPath, drv, OutputsSpec::All {}, buildMode); try { worker.run(Goals{goal}); @@ -89,7 +89,10 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat return BuildResult { .status = BuildResult::MiscFailure, .errorMsg = e.msg(), - .path = DerivedPath::Built { .drvPath = drvPath }, + .path = DerivedPath::Built { + .drvPath = drvPath, + .outputs = OutputsSpec::All { }, + }, }; }; } diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index 4d398b21d..e1a4e13a3 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -279,7 +279,12 @@ public: conn->to.flush(); - BuildResult status { .path = DerivedPath::Built { .drvPath = drvPath } }; + BuildResult status { + .path = DerivedPath::Built { + .drvPath = drvPath, + .outputs = OutputsSpec::All { }, + }, + }; status.status = (BuildResult::Status) readInt(conn->from); conn->from >> status.errorMsg; diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index 891252990..1eeab1aa8 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -110,9 +110,21 @@ bool OutputsSpec::merge(const OutputsSpec & that) }, raw()); } +} -void to_json(nlohmann::json & json, const OutputsSpec & outputsSpec) -{ +namespace nlohmann { + +using namespace nix; + +OutputsSpec adl_serializer::from_json(const json & json) { + auto names = json.get(); + if (names == StringSet({"*"})) + return OutputsSpec::All {}; + else + return OutputsSpec::Names { std::move(names) }; +} + +void adl_serializer::to_json(json & json, OutputsSpec t) { std::visit(overloaded { [&](const OutputsSpec::All &) { json = std::vector({"*"}); @@ -120,40 +132,27 @@ void to_json(nlohmann::json & json, const OutputsSpec & outputsSpec) [&](const OutputsSpec::Names & names) { json = names; }, - }, outputsSpec); + }, t); } -void to_json(nlohmann::json & json, const ExtendedOutputsSpec & extendedOutputsSpec) -{ +ExtendedOutputsSpec adl_serializer::from_json(const json & json) { + if (json.is_null()) + return ExtendedOutputsSpec::Default {}; + else { + return ExtendedOutputsSpec::Explicit { json.get() }; + } +} + +void adl_serializer::to_json(json & json, ExtendedOutputsSpec t) { std::visit(overloaded { [&](const ExtendedOutputsSpec::Default &) { json = nullptr; }, [&](const ExtendedOutputsSpec::Explicit & e) { - to_json(json, e); + adl_serializer::to_json(json, e); }, - }, extendedOutputsSpec); -} - - -void from_json(const nlohmann::json & json, OutputsSpec & outputsSpec) -{ - auto names = json.get(); - if (names == StringSet({"*"})) - outputsSpec = OutputsSpec::All {}; - else - outputsSpec = OutputsSpec::Names { std::move(names) }; -} - - -void from_json(const nlohmann::json & json, ExtendedOutputsSpec & extendedOutputsSpec) -{ - if (json.is_null()) - extendedOutputsSpec = ExtendedOutputsSpec::Default {}; - else { - extendedOutputsSpec = ExtendedOutputsSpec::Explicit { json.get() }; - } + }, t); } } diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index 9c477ee2b..babf29d16 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -4,7 +4,7 @@ #include #include -#include "nlohmann/json_fwd.hpp" +#include "json-impls.hh" namespace nix { @@ -35,6 +35,9 @@ struct OutputsSpec : _OutputsSpecRaw { using Raw = _OutputsSpecRaw; using Raw::Raw; + /* Force choosing a variant */ + OutputsSpec() = delete; + using Names = OutputNames; using All = AllOutputs; @@ -85,11 +88,7 @@ struct ExtendedOutputsSpec : _ExtendedOutputsSpecRaw { std::string to_string() const; }; - -void to_json(nlohmann::json &, const OutputsSpec &); -void from_json(const nlohmann::json &, OutputsSpec &); - -void to_json(nlohmann::json &, const ExtendedOutputsSpec &); -void from_json(const nlohmann::json &, ExtendedOutputsSpec &); - } + +JSON_IMPL(OutputsSpec) +JSON_IMPL(ExtendedOutputsSpec) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 832be08f7..ff57a77ca 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -910,7 +910,12 @@ BuildResult RemoteStore::buildDerivation(const StorePath & drvPath, const BasicD writeDerivation(conn->to, *this, drv); conn->to << buildMode; conn.processStderr(); - BuildResult res { .path = DerivedPath::Built { .drvPath = drvPath } }; + BuildResult res { + .path = DerivedPath::Built { + .drvPath = drvPath, + .outputs = OutputsSpec::All { }, + }, + }; res.status = (BuildResult::Status) readInt(conn->from); conn->from >> res.errorMsg; if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 29) { diff --git a/src/libutil/json-impls.hh b/src/libutil/json-impls.hh new file mode 100644 index 000000000..bd75748ad --- /dev/null +++ b/src/libutil/json-impls.hh @@ -0,0 +1,14 @@ +#pragma once + +#include "nlohmann/json_fwd.hpp" + +// Following https://github.com/nlohmann/json#how-can-i-use-get-for-non-default-constructiblenon-copyable-types +#define JSON_IMPL(TYPE) \ + namespace nlohmann { \ + using namespace nix; \ + template <> \ + struct adl_serializer { \ + static TYPE from_json(const json & json); \ + static void to_json(json & json, TYPE t); \ + }; \ + } diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index 31823a966..406e548c0 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -478,9 +478,14 @@ static void printMissing(EvalState & state, DrvInfos & elems) std::vector targets; for (auto & i : elems) if (auto drvPath = i.queryDrvPath()) - targets.push_back(DerivedPath::Built{*drvPath}); + targets.push_back(DerivedPath::Built{ + .drvPath = *drvPath, + .outputs = OutputsSpec::All { }, + }); else - targets.push_back(DerivedPath::Opaque{i.queryOutPath()}); + targets.push_back(DerivedPath::Opaque{ + .path = i.queryOutPath(), + }); printMissing(state.store, targets); } @@ -751,8 +756,13 @@ static void opSet(Globals & globals, Strings opFlags, Strings opArgs) auto drvPath = drv.queryDrvPath(); std::vector paths { drvPath - ? (DerivedPath) (DerivedPath::Built { *drvPath }) - : (DerivedPath) (DerivedPath::Opaque { drv.queryOutPath() }), + ? (DerivedPath) (DerivedPath::Built { + .drvPath = *drvPath, + .outputs = OutputsSpec::All { }, + }) + : (DerivedPath) (DerivedPath::Opaque { + .path = drv.queryOutPath(), + }), }; printMissing(globals.state->store, paths); if (globals.dryRun) return; diff --git a/src/nix/bundle.cc b/src/nix/bundle.cc index a09dadff4..6ae9460f6 100644 --- a/src/nix/bundle.cc +++ b/src/nix/bundle.cc @@ -105,7 +105,12 @@ struct CmdBundle : InstallableCommand auto outPath = evalState->coerceToStorePath(attr2->pos, *attr2->value, context2, ""); - store->buildPaths({ DerivedPath::Built { drvPath } }); + store->buildPaths({ + DerivedPath::Built { + .drvPath = drvPath, + .outputs = OutputsSpec::All { }, + }, + }); auto outPathS = store->printStorePath(outPath); diff --git a/src/nix/develop.cc b/src/nix/develop.cc index 6aa675386..16bbd8613 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -232,7 +232,12 @@ static StorePath getDerivationEnvironment(ref store, ref evalStore auto shellDrvPath = writeDerivation(*evalStore, drv); /* Build the derivation. */ - store->buildPaths({DerivedPath::Built{shellDrvPath}}, bmNormal, evalStore); + store->buildPaths( + { DerivedPath::Built { + .drvPath = shellDrvPath, + .outputs = OutputsSpec::All { }, + }}, + bmNormal, evalStore); for (auto & [_0, optPath] : evalStore->queryPartialDerivationOutputMap(shellDrvPath)) { assert(optPath); diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 33ce3f401..d16d88ef8 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -513,8 +513,12 @@ struct CmdFlakeCheck : FlakeCommand auto drvPath = checkDerivation( fmt("%s.%s.%s", name, attr_name, state->symbols[attr2.name]), *attr2.value, attr2.pos); - if (drvPath && attr_name == settings.thisSystem.get()) - drvPaths.push_back(DerivedPath::Built{*drvPath}); + if (drvPath && attr_name == settings.thisSystem.get()) { + drvPaths.push_back(DerivedPath::Built { + .drvPath = *drvPath, + .outputs = OutputsSpec::All { }, + }); + } } } } From 0faf5326bd333eeef126730683dc02009a06402f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 17:31:32 -0500 Subject: [PATCH 33/41] Improve tests for `OutputsSpec` --- src/libstore/outputs-spec.cc | 21 +++-- src/libstore/outputs-spec.hh | 9 +-- src/libstore/tests/outputs-spec.cc | 123 ++++++++++++++++++----------- 3 files changed, 97 insertions(+), 56 deletions(-) diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index 1eeab1aa8..8e6e40c2b 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -40,22 +40,33 @@ std::optional OutputsSpec::parseOpt(std::string_view s) OutputsSpec OutputsSpec::parse(std::string_view s) { - std::optional spec = OutputsSpec::parseOpt(s); + std::optional spec = parseOpt(s); if (!spec) throw Error("Invalid outputs specifier: '%s'", s); return *spec; } -std::pair ExtendedOutputsSpec::parse(std::string_view s) +std::optional> ExtendedOutputsSpec::parseOpt(std::string_view s) { auto found = s.rfind('^'); if (found == std::string::npos) - return { s, ExtendedOutputsSpec::Default {} }; + return std::pair { s, ExtendedOutputsSpec::Default {} }; - auto spec = OutputsSpec::parse(s.substr(found + 1)); - return { s.substr(0, found), ExtendedOutputsSpec::Explicit { spec } }; + auto specOpt = OutputsSpec::parseOpt(s.substr(found + 1)); + if (!specOpt) + return std::nullopt; + return std::pair { s.substr(0, found), ExtendedOutputsSpec::Explicit { *std::move(specOpt) } }; +} + + +std::pair ExtendedOutputsSpec::parse(std::string_view s) +{ + std::optional spec = parseOpt(s); + if (!spec) + throw Error("Invalid extended outputs specifier: '%s'", s); + return *spec; } diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index babf29d16..9211a4fc6 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -25,9 +25,7 @@ struct OutputNames : std::set { OutputNames() = delete; }; -struct AllOutputs { - bool operator < (const AllOutputs & _) const { return false; } -}; +struct AllOutputs : std::monostate { }; typedef std::variant _OutputsSpecRaw; @@ -64,9 +62,7 @@ struct OutputsSpec : _OutputsSpecRaw { std::string to_string() const; }; -struct DefaultOutputs { - bool operator < (const DefaultOutputs & _) const { return false; } -}; +struct DefaultOutputs : std::monostate { }; typedef std::variant _ExtendedOutputsSpecRaw; @@ -84,6 +80,7 @@ struct ExtendedOutputsSpec : _ExtendedOutputsSpecRaw { /* Parse a string of the form 'prefix^output1,...outputN' or 'prefix^*', returning the prefix and the extended outputs spec. */ static std::pair parse(std::string_view s); + static std::optional> parseOpt(std::string_view s); std::string to_string() const; }; diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index 03259e7c7..6d07795aa 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -4,63 +4,96 @@ namespace nix { -TEST(OutputsSpec_parse, basic) -{ - { - auto outputsSpec = OutputsSpec::parse("*"); - ASSERT_TRUE(std::get_if(&outputsSpec)); +#define TEST_DONT_PARSE(NAME, STR) \ + TEST(OutputsSpec, bad_ ## NAME) { \ + std::optional OutputsSpecOpt = \ + OutputsSpec::parseOpt(STR); \ + ASSERT_FALSE(OutputsSpecOpt); \ } - { - auto outputsSpec = OutputsSpec::parse("out"); - ASSERT_TRUE(std::get(outputsSpec) == OutputsSpec::Names({"out"})); - } +TEST_DONT_PARSE(empty, "") +TEST_DONT_PARSE(garbage, "&*()") +TEST_DONT_PARSE(double_star, "**") +TEST_DONT_PARSE(star_first, "*,foo") +TEST_DONT_PARSE(star_second, "foo,*") - { - auto outputsSpec = OutputsSpec::parse("out,bin"); - ASSERT_TRUE(std::get(outputsSpec) == OutputsSpec::Names({"out", "bin"})); - } +#undef TEST_DONT_PARSE - { - std::optional outputsSpecOpt = OutputsSpec::parseOpt("&*()"); - ASSERT_FALSE(outputsSpecOpt); - } +TEST(OutputsSpec, all) { + std::string_view str = "*"; + OutputsSpec expected = OutputsSpec::All { }; + ASSERT_EQ(OutputsSpec::parse(str), expected); + ASSERT_EQ(expected.to_string(), str); +} + +TEST(OutputsSpec, names_out) { + std::string_view str = "out"; + OutputsSpec expected = OutputsSpec::Names { "out" }; + ASSERT_EQ(OutputsSpec::parse(str), expected); + ASSERT_EQ(expected.to_string(), str); +} + +TEST(OutputsSpec, names_out_bin) { + OutputsSpec expected = OutputsSpec::Names { "out", "bin" }; + ASSERT_EQ(OutputsSpec::parse("out,bin"), expected); + // N.B. This normalization is OK. + ASSERT_EQ(expected.to_string(), "bin,out"); } -TEST(ExtendedOutputsSpec_parse, basic) -{ - { - auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo"); - ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); +#define TEST_DONT_PARSE(NAME, STR) \ + TEST(ExtendedOutputsSpec, bad_ ## NAME) { \ + std::optional extendedOutputsSpecOpt = \ + ExtendedOutputsSpec::parseOpt(STR); \ + ASSERT_FALSE(extendedOutputsSpecOpt); \ } - { - auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^*"); - ASSERT_EQ(prefix, "foo"); - auto * explicit_p = std::get_if(&extendedOutputsSpec); - ASSERT_TRUE(explicit_p); - ASSERT_TRUE(std::get_if(explicit_p)); - } +TEST_DONT_PARSE(carot_empty, "^") +TEST_DONT_PARSE(prefix_carot_empty, "foo^") - { - auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out"); - ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get(std::get(extendedOutputsSpec)) == OutputsSpec::Names({"out"})); - } +#undef TEST_DONT_PARSE - { - auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out,bin"); - ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get(std::get(extendedOutputsSpec)) == OutputsSpec::Names({"out", "bin"})); - } +TEST(ExtendedOutputsSpec, defeault) { + std::string_view str = "foo"; + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(str); + ASSERT_EQ(prefix, "foo"); + ExtendedOutputsSpec expected = ExtendedOutputsSpec::Default { }; + ASSERT_EQ(extendedOutputsSpec, expected); + ASSERT_EQ(std::string { prefix } + expected.to_string(), str); +} - { - auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^bar^out,bin"); - ASSERT_EQ(prefix, "foo^bar"); - ASSERT_TRUE(std::get(std::get(extendedOutputsSpec)) == OutputsSpec::Names({"out", "bin"})); - } +TEST(ExtendedOutputsSpec, all) { + std::string_view str = "foo^*"; + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(str); + ASSERT_EQ(prefix, "foo"); + ExtendedOutputsSpec expected = OutputsSpec::All { }; + ASSERT_EQ(extendedOutputsSpec, expected); + ASSERT_EQ(std::string { prefix } + expected.to_string(), str); +} + +TEST(ExtendedOutputsSpec, out) { + std::string_view str = "foo^out"; + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(str); + ASSERT_EQ(prefix, "foo"); + ExtendedOutputsSpec expected = OutputsSpec::Names { "out" }; + ASSERT_EQ(extendedOutputsSpec, expected); + ASSERT_EQ(std::string { prefix } + expected.to_string(), str); +} + +TEST(ExtendedOutputsSpec, out_bin) { + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out,bin"); + ASSERT_EQ(prefix, "foo"); + ExtendedOutputsSpec expected = OutputsSpec::Names { "out", "bin" }; + ASSERT_EQ(extendedOutputsSpec, expected); + ASSERT_EQ(std::string { prefix } + expected.to_string(), "foo^bin,out"); +} + +TEST(ExtendedOutputsSpec, many_carrot) { + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^bar^out,bin"); + ASSERT_EQ(prefix, "foo^bar"); + ExtendedOutputsSpec expected = OutputsSpec::Names { "out", "bin" }; + ASSERT_EQ(extendedOutputsSpec, expected); + ASSERT_EQ(std::string { prefix } + expected.to_string(), "foo^bar^bin,out"); } } From 48b2a3a0d03d524a0c1048e7a74acdead11a57b4 Mon Sep 17 00:00:00 2001 From: Valentin Gagarin Date: Thu, 12 Jan 2023 13:23:32 +0100 Subject: [PATCH 34/41] remove unncessary cast --- src/libstore/path-with-outputs.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/path-with-outputs.cc b/src/libstore/path-with-outputs.cc index 17230ffc4..eae5553c5 100644 --- a/src/libstore/path-with-outputs.cc +++ b/src/libstore/path-with-outputs.cc @@ -52,8 +52,8 @@ std::pair parsePathWithOutputs(std::string_view s) size_t n = s.find("!"); return n == s.npos ? std::make_pair(s, std::set()) - : std::make_pair(((std::string_view) s).substr(0, n), - tokenizeString>(((std::string_view) s).substr(n + 1), ",")); + : std::make_pair(s.substr(0, n), + tokenizeString>(s.substr(n + 1), ",")); } From 7de8af526e196f16b757289b41cde65ccd68f4e8 Mon Sep 17 00:00:00 2001 From: Valentin Gagarin Date: Thu, 8 Dec 2022 12:01:58 +0100 Subject: [PATCH 35/41] state priorities in triaging and discussion process based on - Nix team decisions https://discourse.nixos.org/t/2022-11-11-nix-team-meeting-minutes-7/23451#planning-discussion-1 https://discourse.nixos.org/t/2022-12-02-nix-team-meeting-minutes-13/23731#discussion-3 - proposal to deal use labels more effectively https://discourse.nixos.org/t/improving-nix-developer-experience/21629 - documentation team decision to foster gauging interest using upvotes https://github.com/NixOS/nix/pull/7387 --- maintainers/README.md | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/maintainers/README.md b/maintainers/README.md index 60768db0a..08d197c1b 100644 --- a/maintainers/README.md +++ b/maintainers/README.md @@ -36,17 +36,45 @@ Issues on the board progress through the following states: - No Status - Team members can add pull requests or issues to discuss or review together. - During the discussion meeting, the team triages new items. + To be considered, issues and pull requests must have a high-level description to provide the whole team with the necessary context at a glance. + + On every meeting, at least one item from each of the following categories is inspected: + + 1. [critical](https://github.com/NixOS/nix/labels/critical) + 2. [security](https://github.com/NixOS/nix/labels/security) + 3. [regression](https://github.com/NixOS/nix/labels/regression) + 4. [bug](https://github.com/NixOS/nix/issues?q=is%3Aopen+label%3Abug+sort%3Areactions-%2B1-desc) + + - [oldest pull requests](https://github.com/NixOS/nix/pulls?q=is%3Apr+is%3Aopen+sort%3Acreated-asc) + - [most popular pull requests](https://github.com/NixOS/nix/pulls?q=is%3Apr+is%3Aopen+sort%3Areactions-%2B1-desc) + - [oldest issues](https://github.com/NixOS/nix/issues?q=is%3Aissue+is%3Aopen+sort%3Acreated-asc) + - [most popular issues](https://github.com/NixOS/nix/issues?q=is%3Aissue+is%3Aopen+sort%3Areactions-%2B1-desc) + + Team members can also add pull requests or issues they would like the whole team to consider. + If there is disagreement on the general idea behind an issue or pull request, it is moved to _To discuss_, otherwise to _In review_. - To discuss - Pull requests and issues that are important and controversial are discussed by the team during discussion meetings. + Pull requests and issues that are deemed important and controversial are discussed by the team during discussion meetings. This may be where the merit of the change itself or the implementation strategy is contested by a team member. + As a general guideline, the order of items is determined as follows: + + - Prioritise pull requests over issues + + Contributors who took the time to implement concrete change proposals should not wait indefinitely. + + - Prioritise fixing bugs over documentation, improvements or new features + + The team values stability and accessibility higher than raw functionality. + + - Interleave issues and PRs + + This way issues without attempts at a solution get a chance to get addressed. + - In review Pull requests in this column are reviewed together during work meetings. From 31875bcfb7ccbbf6e88c2cc62714a2a3794994ec Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 12 Jan 2023 20:20:27 -0500 Subject: [PATCH 36/41] Split `OutputsSpec::merge` into `OuputsSpec::{union_, isSubsetOf}` Additionally get rid of the evil time we made an empty `OutputSpec::Names()`. --- src/libcmd/installables.cc | 26 ++++++++------- src/libstore/build/derivation-goal.cc | 5 +-- src/libstore/outputs-spec.cc | 46 +++++++++++++++++++-------- src/libstore/outputs-spec.hh | 9 +++--- 4 files changed, 56 insertions(+), 30 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index d73109873..5090ea6d2 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -469,20 +469,14 @@ struct InstallableAttrPath : InstallableValue // Backward compatibility hack: group results by drvPath. This // helps keep .all output together. - std::map byDrvPath; + std::map byDrvPath; for (auto & drvInfo : drvInfos) { auto drvPath = drvInfo.queryDrvPath(); if (!drvPath) throw Error("'%s' is not a derivation", what()); - auto derivedPath = byDrvPath.emplace(*drvPath, DerivedPath::Built { - .drvPath = *drvPath, - // Not normally legal, but we will merge right below - .outputs = OutputsSpec::Names { StringSet { } }, - }).first; - - derivedPath->second.outputs.merge(std::visit(overloaded { + auto newOutputs = std::visit(overloaded { [&](const ExtendedOutputsSpec::Default & d) -> OutputsSpec { std::set outputsToInstall; for (auto & output : drvInfo.queryOutputs(false, true)) @@ -492,12 +486,22 @@ struct InstallableAttrPath : InstallableValue [&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec { return e; }, - }, extendedOutputsSpec.raw())); + }, extendedOutputsSpec.raw()); + + auto [iter, didInsert] = byDrvPath.emplace(*drvPath, newOutputs); + + if (!didInsert) + iter->second = iter->second.union_(newOutputs); } DerivedPathsWithInfo res; - for (auto & [_, info] : byDrvPath) - res.push_back({ .path = { info } }); + for (auto & [drvPath, outputs] : byDrvPath) + res.push_back({ + .path = DerivedPath::Built { + .drvPath = drvPath, + .outputs = outputs, + }, + }); return res; } diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 61169635a..2021d0023 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -144,9 +144,10 @@ void DerivationGoal::work() void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs) { - bool newOutputs = wantedOutputs.merge(outputs); - if (newOutputs) + auto newWanted = wantedOutputs.union_(outputs); + if (!newWanted.isSubsetOf(wantedOutputs)) needRestart = true; + wantedOutputs = newWanted; } diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index 8e6e40c2b..d0f39a854 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -96,24 +96,20 @@ std::string ExtendedOutputsSpec::to_string() const } -bool OutputsSpec::merge(const OutputsSpec & that) +OutputsSpec OutputsSpec::union_(const OutputsSpec & that) const { return std::visit(overloaded { - [&](OutputsSpec::All &) { - /* If we already refer to all outputs, there is nothing to do. */ - return false; + [&](const OutputsSpec::All &) -> OutputsSpec { + return OutputsSpec::All { }; }, - [&](OutputsSpec::Names & theseNames) { + [&](const OutputsSpec::Names & theseNames) -> OutputsSpec { return std::visit(overloaded { - [&](const OutputsSpec::All &) { - *this = OutputsSpec::All {}; - return true; + [&](const OutputsSpec::All &) -> OutputsSpec { + return OutputsSpec::All {}; }, - [&](const OutputsSpec::Names & thoseNames) { - bool ret = false; - for (auto & i : thoseNames) - if (theseNames.insert(i).second) - ret = true; + [&](const OutputsSpec::Names & thoseNames) -> OutputsSpec { + OutputsSpec::Names ret = theseNames; + ret.insert(thoseNames.begin(), thoseNames.end()); return ret; }, }, that.raw()); @@ -121,6 +117,30 @@ bool OutputsSpec::merge(const OutputsSpec & that) }, raw()); } + +bool OutputsSpec::isSubsetOf(const OutputsSpec & that) const +{ + return std::visit(overloaded { + [&](const OutputsSpec::All &) { + return true; + }, + [&](const OutputsSpec::Names & thoseNames) { + return std::visit(overloaded { + [&](const OutputsSpec::All &) { + return false; + }, + [&](const OutputsSpec::Names & theseNames) { + bool ret = true; + for (auto & o : theseNames) + if (thoseNames.count(o) == 0) + ret = false; + return ret; + }, + }, raw()); + }, + }, that.raw()); +} + } namespace nlohmann { diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index 9211a4fc6..82dfad479 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -49,10 +49,11 @@ struct OutputsSpec : _OutputsSpecRaw { bool contains(const std::string & output) const; - /* Modify the receiver outputs spec so it is the union of it's old value - and the argument. Return whether the output spec needed to be modified - --- if it didn't it was already "large enough". */ - bool merge(const OutputsSpec & outputs); + /* Create a new OutputsSpec which is the union of this and that. */ + OutputsSpec union_(const OutputsSpec & that) const; + + /* Whether this OutputsSpec is a subset of that. */ + bool isSubsetOf(const OutputsSpec & outputs) const; /* Parse a string of the form 'output1,...outputN' or '*', returning the outputs spec. */ From e947aa540129441ebb3df1980c9ba05a935473ca Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 12 Jan 2023 20:33:50 -0500 Subject: [PATCH 37/41] Unit test `OuputsSpec::{union_, isSubsetOf}` --- src/libstore/tests/outputs-spec.cc | 49 ++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index 6d07795aa..5daac9234 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -40,6 +40,55 @@ TEST(OutputsSpec, names_out_bin) { ASSERT_EQ(expected.to_string(), "bin,out"); } +#define TEST_SUBSET(X, THIS, THAT) \ + X((OutputsSpec { THIS }).isSubsetOf(THAT)); + +TEST(OutputsSpec, subsets_all_all) { + TEST_SUBSET(ASSERT_TRUE, OutputsSpec::All { }, OutputsSpec::All { }); +} + +TEST(OutputsSpec, subsets_names_all) { + TEST_SUBSET(ASSERT_TRUE, OutputsSpec::Names { "a" }, OutputsSpec::All { }); +} + +TEST(OutputsSpec, subsets_names_names_eq) { + TEST_SUBSET(ASSERT_TRUE, OutputsSpec::Names { "a" }, OutputsSpec::Names { "a" }); +} + +TEST(OutputsSpec, subsets_names_names_noneq) { + TEST_SUBSET(ASSERT_TRUE, OutputsSpec::Names { "a" }, (OutputsSpec::Names { "a", "b" })); +} + +TEST(OutputsSpec, not_subsets_all_names) { + TEST_SUBSET(ASSERT_FALSE, OutputsSpec::All { }, OutputsSpec::Names { "a" }); +} + +TEST(OutputsSpec, not_subsets_names_names) { + TEST_SUBSET(ASSERT_FALSE, (OutputsSpec::Names { "a", "b" }), (OutputsSpec::Names { "a" })); +} + +#undef TEST_SUBSET + +#define TEST_UNION(RES, THIS, THAT) \ + ASSERT_EQ(OutputsSpec { RES }, (OutputsSpec { THIS }).union_(THAT)); + +TEST(OutputsSpec, union_all_all) { + TEST_UNION(OutputsSpec::All { }, OutputsSpec::All { }, OutputsSpec::All { }); +} + +TEST(OutputsSpec, union_all_names) { + TEST_UNION(OutputsSpec::All { }, OutputsSpec::All { }, OutputsSpec::Names { "a" }); +} + +TEST(OutputsSpec, union_names_all) { + TEST_UNION(OutputsSpec::All { }, OutputsSpec::Names { "a" }, OutputsSpec::All { }); +} + +TEST(OutputsSpec, union_names_names) { + TEST_UNION((OutputsSpec::Names { "a", "b" }), OutputsSpec::Names { "a" }, OutputsSpec::Names { "b" }); +} + +#undef TEST_UNION #define TEST_DONT_PARSE(NAME, STR) \ TEST(ExtendedOutputsSpec, bad_ ## NAME) { \ From d29eb085630aac6cbefeafe51937314ce0263593 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 12 Jan 2023 20:41:29 -0500 Subject: [PATCH 38/41] Assert on construction that `OutputsSpec::Names` is non-empty --- src/libstore/outputs-spec.hh | 9 ++++++--- src/libstore/tests/outputs-spec.cc | 6 ++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index 82dfad479..46bc35ebc 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -11,13 +12,15 @@ namespace nix { struct OutputNames : std::set { using std::set::set; - // These need to be "inherited manually" + /* These need to be "inherited manually" */ + OutputNames(const std::set & s) : std::set(s) - { } + { assert(!empty()); } + OutputNames(std::set && s) : std::set(s) - { } + { assert(!empty()); } /* This set should always be non-empty, so we delete this constructor in order make creating empty ones by mistake harder. diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index 5daac9234..c5e3f382b 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -4,6 +4,12 @@ namespace nix { +#ifndef NDEBUG +TEST(OutputsSpec, no_empty_names) { + ASSERT_DEATH(OutputsSpec::Names { std::set { } }, ""); +} +#endif + #define TEST_DONT_PARSE(NAME, STR) \ TEST(OutputsSpec, bad_ ## NAME) { \ std::optional OutputsSpecOpt = \ From d8512653d480acf69aae820f8b9d4b674dd6fc2f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 12 Jan 2023 22:05:55 -0500 Subject: [PATCH 39/41] Write more (extended) output spec tests --- src/libstore/tests/outputs-spec.cc | 33 ++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index c5e3f382b..c9c2cafd0 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -1,5 +1,6 @@ #include "outputs-spec.hh" +#include #include namespace nix { @@ -105,6 +106,10 @@ TEST(OutputsSpec, union_names_names) { TEST_DONT_PARSE(carot_empty, "^") TEST_DONT_PARSE(prefix_carot_empty, "foo^") +TEST_DONT_PARSE(garbage, "^&*()") +TEST_DONT_PARSE(double_star, "^**") +TEST_DONT_PARSE(star_first, "^*,foo") +TEST_DONT_PARSE(star_second, "^foo,*") #undef TEST_DONT_PARSE @@ -151,4 +156,32 @@ TEST(ExtendedOutputsSpec, many_carrot) { ASSERT_EQ(std::string { prefix } + expected.to_string(), "foo^bar^bin,out"); } + +#define TEST_JSON(TYPE, NAME, STR, VAL) \ + \ + TEST(TYPE, NAME ## _to_json) { \ + using nlohmann::literals::operator "" _json; \ + ASSERT_EQ( \ + STR ## _json, \ + ((nlohmann::json) TYPE { VAL })); \ + } \ + \ + TEST(TYPE, NAME ## _from_json) { \ + using nlohmann::literals::operator "" _json; \ + ASSERT_EQ( \ + TYPE { VAL }, \ + (STR ## _json).get()); \ + } + +TEST_JSON(OutputsSpec, all, R"(["*"])", OutputsSpec::All { }) +TEST_JSON(OutputsSpec, name, R"(["a"])", OutputsSpec::Names { "a" }) +TEST_JSON(OutputsSpec, names, R"(["a","b"])", (OutputsSpec::Names { "a", "b" })) + +TEST_JSON(ExtendedOutputsSpec, def, R"(null)", ExtendedOutputsSpec::Default { }) +TEST_JSON(ExtendedOutputsSpec, all, R"(["*"])", ExtendedOutputsSpec::Explicit { OutputsSpec::All { } }) +TEST_JSON(ExtendedOutputsSpec, name, R"(["a"])", ExtendedOutputsSpec::Explicit { OutputsSpec::Names { "a" } }) +TEST_JSON(ExtendedOutputsSpec, names, R"(["a","b"])", (ExtendedOutputsSpec::Explicit { OutputsSpec::Names { "a", "b" } })) + +#undef TEST_JSON + } From b8a0e9a9b8f0499502d317b7424f6b59fd8b48fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Fri, 13 Jan 2023 11:05:19 +0100 Subject: [PATCH 40/41] Move the `getBuildLog` implementation to its own implementation file Keep the header minimal and clean --- src/libstore/log-store.cc | 12 ++++++++++++ src/libstore/log-store.hh | 7 +------ 2 files changed, 13 insertions(+), 6 deletions(-) create mode 100644 src/libstore/log-store.cc diff --git a/src/libstore/log-store.cc b/src/libstore/log-store.cc new file mode 100644 index 000000000..8a26832ab --- /dev/null +++ b/src/libstore/log-store.cc @@ -0,0 +1,12 @@ +#include "log-store.hh" + +namespace nix { + +std::optional LogStore::getBuildLog(const StorePath & path) { + auto maybePath = getBuildDerivationPath(path); + if (!maybePath) + return std::nullopt; + return getBuildLogExact(maybePath.value()); +} + +} diff --git a/src/libstore/log-store.hh b/src/libstore/log-store.hh index b807e3e71..e4d95bab6 100644 --- a/src/libstore/log-store.hh +++ b/src/libstore/log-store.hh @@ -11,12 +11,7 @@ struct LogStore : public virtual Store /* Return the build log of the specified store path, if available, or null otherwise. */ - std::optional getBuildLog(const StorePath & path) { - auto maybePath = getBuildDerivationPath(path); - if (!maybePath) - return std::nullopt; - return getBuildLogExact(maybePath.value()); - } + std::optional getBuildLog(const StorePath & path); virtual std::optional getBuildLogExact(const StorePath & path) = 0; From a4164762178a0520b6998c864c7752d3497c40d2 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 13 Jan 2023 15:23:29 -0500 Subject: [PATCH 41/41] Move `ValidPathInfo` defintions to `path-info.cc` Originally there was no `path-info.*`, then there was `path-info.hh`, then there was `path-info.cc`, but only for new things. Moving this stuff over makes everything consistent. --- src/libstore/path-info.cc | 75 +++++++++++++++++++++++++++++++++++++++ src/libstore/store-api.cc | 73 ------------------------------------- 2 files changed, 75 insertions(+), 73 deletions(-) diff --git a/src/libstore/path-info.cc b/src/libstore/path-info.cc index fda55b2b6..bd55a9d06 100644 --- a/src/libstore/path-info.cc +++ b/src/libstore/path-info.cc @@ -3,6 +3,80 @@ namespace nix { +std::string ValidPathInfo::fingerprint(const Store & store) const +{ + if (narSize == 0) + throw Error("cannot calculate fingerprint of path '%s' because its size is not known", + store.printStorePath(path)); + return + "1;" + store.printStorePath(path) + ";" + + narHash.to_string(Base32, true) + ";" + + std::to_string(narSize) + ";" + + concatStringsSep(",", store.printStorePathSet(references)); +} + + +void ValidPathInfo::sign(const Store & store, const SecretKey & secretKey) +{ + sigs.insert(secretKey.signDetached(fingerprint(store))); +} + + +bool ValidPathInfo::isContentAddressed(const Store & store) const +{ + if (! ca) return false; + + auto caPath = std::visit(overloaded { + [&](const TextHash & th) { + return store.makeTextPath(path.name(), th.hash, references); + }, + [&](const FixedOutputHash & fsh) { + auto refs = references; + bool hasSelfReference = false; + if (refs.count(path)) { + hasSelfReference = true; + refs.erase(path); + } + return store.makeFixedOutputPath(fsh.method, fsh.hash, path.name(), refs, hasSelfReference); + } + }, *ca); + + bool res = caPath == path; + + if (!res) + printError("warning: path '%s' claims to be content-addressed but isn't", store.printStorePath(path)); + + return res; +} + + +size_t ValidPathInfo::checkSignatures(const Store & store, const PublicKeys & publicKeys) const +{ + if (isContentAddressed(store)) return maxSigs; + + size_t good = 0; + for (auto & sig : sigs) + if (checkSignature(store, publicKeys, sig)) + good++; + return good; +} + + +bool ValidPathInfo::checkSignature(const Store & store, const PublicKeys & publicKeys, const std::string & sig) const +{ + return verifyDetached(fingerprint(store), sig, publicKeys); +} + + +Strings ValidPathInfo::shortRefs() const +{ + Strings refs; + for (auto & r : references) + refs.push_back(std::string(r.to_string())); + return refs; +} + + ValidPathInfo ValidPathInfo::read(Source & source, const Store & store, unsigned int format) { return read(source, store, format, store.parseStorePath(readString(source))); @@ -24,6 +98,7 @@ ValidPathInfo ValidPathInfo::read(Source & source, const Store & store, unsigned return info; } + void ValidPathInfo::write( Sink & sink, const Store & store, diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index f959dfd51..5130409d4 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1210,79 +1210,6 @@ std::string showPaths(const PathSet & paths) } -std::string ValidPathInfo::fingerprint(const Store & store) const -{ - if (narSize == 0) - throw Error("cannot calculate fingerprint of path '%s' because its size is not known", - store.printStorePath(path)); - return - "1;" + store.printStorePath(path) + ";" - + narHash.to_string(Base32, true) + ";" - + std::to_string(narSize) + ";" - + concatStringsSep(",", store.printStorePathSet(references)); -} - - -void ValidPathInfo::sign(const Store & store, const SecretKey & secretKey) -{ - sigs.insert(secretKey.signDetached(fingerprint(store))); -} - -bool ValidPathInfo::isContentAddressed(const Store & store) const -{ - if (! ca) return false; - - auto caPath = std::visit(overloaded { - [&](const TextHash & th) { - return store.makeTextPath(path.name(), th.hash, references); - }, - [&](const FixedOutputHash & fsh) { - auto refs = references; - bool hasSelfReference = false; - if (refs.count(path)) { - hasSelfReference = true; - refs.erase(path); - } - return store.makeFixedOutputPath(fsh.method, fsh.hash, path.name(), refs, hasSelfReference); - } - }, *ca); - - bool res = caPath == path; - - if (!res) - printError("warning: path '%s' claims to be content-addressed but isn't", store.printStorePath(path)); - - return res; -} - - -size_t ValidPathInfo::checkSignatures(const Store & store, const PublicKeys & publicKeys) const -{ - if (isContentAddressed(store)) return maxSigs; - - size_t good = 0; - for (auto & sig : sigs) - if (checkSignature(store, publicKeys, sig)) - good++; - return good; -} - - -bool ValidPathInfo::checkSignature(const Store & store, const PublicKeys & publicKeys, const std::string & sig) const -{ - return verifyDetached(fingerprint(store), sig, publicKeys); -} - - -Strings ValidPathInfo::shortRefs() const -{ - Strings refs; - for (auto & r : references) - refs.push_back(std::string(r.to_string())); - return refs; -} - - Derivation Store::derivationFromPath(const StorePath & drvPath) { ensurePath(drvPath);