From 521e0434f176f57f1b5e4d1932991746baf38794 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 5 Feb 2023 23:28:18 -0500 Subject: [PATCH] Associate each derivation with a single `ExtraPathInfo` Do this as opposed to associating it with each `DerivedPath` produced by the installable. (Installables can produce multiple). `getExtraPathInfo` is a new method which gets instead. Additionally, the new `getExtraPathInfo`, and `nixpkgsFlakeRef`, are moved to `InstallableValue`. To my pleasant surprise, the extra path infos associated with each `DerivedPath` of a given installable were all the same, so this was an easy refactor with no behavior change. I did this because https://github.com/NixOS/rfcs/pull/134 ; with these things moved to `InstallableValue`, the base `Installable` no longer depends on libexpr! This is a major step towards that. --- src/libcmd/command.hh | 2 +- src/libcmd/installable-attr-path.cc | 11 +++-- src/libcmd/installable-attr-path.hh | 4 +- src/libcmd/installable-derived-path.cc | 4 +- src/libcmd/installable-derived-path.hh | 2 +- src/libcmd/installable-flake.cc | 63 ++++++++++++++++---------- src/libcmd/installable-flake.hh | 4 +- src/libcmd/installable-value.hh | 25 ++++++++++ src/libcmd/installables.cc | 34 ++++++-------- src/libcmd/installables.hh | 35 +------------- src/nix/build.cc | 3 +- src/nix/bundle.cc | 1 + src/nix/develop.cc | 12 ++--- src/nix/log.cc | 2 +- src/nix/profile.cc | 7 ++- 15 files changed, 110 insertions(+), 99 deletions(-) diff --git a/src/libcmd/command.hh b/src/libcmd/command.hh index 874ca3249b43..c3990161839f 100644 --- a/src/libcmd/command.hh +++ b/src/libcmd/command.hh @@ -1,6 +1,6 @@ #pragma once -#include "installables.hh" +#include "installable-value.hh" #include "args.hh" #include "common-eval-args.hh" #include "path.hh" diff --git a/src/libcmd/installable-attr-path.cc b/src/libcmd/installable-attr-path.cc index d9377f0d6c72..f6db8c20f89d 100644 --- a/src/libcmd/installable-attr-path.cc +++ b/src/libcmd/installable-attr-path.cc @@ -44,7 +44,7 @@ std::pair InstallableAttrPath::toValue(EvalState & state) return {vRes, pos}; } -DerivedPathsWithInfo InstallableAttrPath::toDerivedPaths() +DerivedPaths InstallableAttrPath::toDerivedPaths() { auto v = toValue(*state).first; @@ -80,10 +80,10 @@ DerivedPathsWithInfo InstallableAttrPath::toDerivedPaths() iter->second = iter->second.union_(newOutputs); } - DerivedPathsWithInfo res; + DerivedPaths res; for (auto & [drvPath, outputs] : byDrvPath) res.push_back({ - .path = DerivedPath::Built { + DerivedPath::Built { .drvPath = drvPath, .outputs = outputs, }, @@ -92,6 +92,11 @@ DerivedPathsWithInfo InstallableAttrPath::toDerivedPaths() return res; } +ExtraPathInfo InstallableAttrPath::getExtraPathInfo() +{ + return {}; +} + InstallableAttrPath InstallableAttrPath::parse( ref state, SourceExprCommand & cmd, diff --git a/src/libcmd/installable-attr-path.hh b/src/libcmd/installable-attr-path.hh index c06132ec8533..e2e6d554a3b6 100644 --- a/src/libcmd/installable-attr-path.hh +++ b/src/libcmd/installable-attr-path.hh @@ -41,7 +41,9 @@ class InstallableAttrPath : public InstallableValue std::pair toValue(EvalState & state) override; - DerivedPathsWithInfo toDerivedPaths() override; + DerivedPaths toDerivedPaths() override; + + ExtraPathInfo getExtraPathInfo() override; public: diff --git a/src/libcmd/installable-derived-path.cc b/src/libcmd/installable-derived-path.cc index 729dc7d316cf..1f074dd12868 100644 --- a/src/libcmd/installable-derived-path.cc +++ b/src/libcmd/installable-derived-path.cc @@ -8,9 +8,9 @@ std::string InstallableDerivedPath::what() const return derivedPath.to_string(*store); } -DerivedPathsWithInfo InstallableDerivedPath::toDerivedPaths() +DerivedPaths InstallableDerivedPath::toDerivedPaths() { - return {{.path = derivedPath, .info = {} }}; + return {derivedPath}; } std::optional InstallableDerivedPath::getStorePath() diff --git a/src/libcmd/installable-derived-path.hh b/src/libcmd/installable-derived-path.hh index 042878b91c81..87332b9f70a9 100644 --- a/src/libcmd/installable-derived-path.hh +++ b/src/libcmd/installable-derived-path.hh @@ -15,7 +15,7 @@ struct InstallableDerivedPath : Installable std::string what() const override; - DerivedPathsWithInfo toDerivedPaths() override; + DerivedPaths toDerivedPaths() override; std::optional getStorePath() override; diff --git a/src/libcmd/installable-flake.cc b/src/libcmd/installable-flake.cc index 7b0cc376d85b..9d61623125bb 100644 --- a/src/libcmd/installable-flake.cc +++ b/src/libcmd/installable-flake.cc @@ -82,7 +82,7 @@ InstallableFlake::InstallableFlake( throw UsageError("'--arg' and '--argstr' are incompatible with flakes"); } -DerivedPathsWithInfo InstallableFlake::toDerivedPaths() +DerivedPaths InstallableFlake::toDerivedPaths() { Activity act(*logger, lvlTalkative, actUnknown, fmt("evaluating derivation '%s'", what())); @@ -98,11 +98,11 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() if (v.type() == nPath) { PathSet context; auto storePath = state->copyPathToStore(context, Path(v.path)); - return {{ - .path = DerivedPath::Opaque { + return { + DerivedPath::Opaque { .path = std::move(storePath), } - }}; + }; } else if (v.type() == nString) { @@ -110,11 +110,11 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() auto s = state->forceString(v, context, noPos, fmt("while evaluating the flake output attribute '%s'", attrPath)); auto storePath = state->store->maybeParseStorePath(s); if (storePath && context.count(std::string(s))) { - return {{ - .path = DerivedPath::Opaque { + return { + DerivedPath::Opaque { .path = std::move(*storePath), } - }}; + }; } else throw Error("flake output attribute '%s' evaluates to the string '%s' which is not a store path", attrPath, s); } @@ -125,16 +125,12 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() auto drvPath = attr->forceDerivation(); - std::optional priority; - - if (attr->maybeGetAttr(state->sOutputSpecified)) { - } else if (auto aMeta = attr->maybeGetAttr(state->sMeta)) { - if (auto aPriority = aMeta->maybeGetAttr("priority")) - priority = aPriority->getInt(); - } + // Do this for side effects, like setting `applyNixConfig`. + // FIXME do this differently. + getLockedFlake(); - return {{ - .path = DerivedPath::Built { + return { + DerivedPath::Built { .drvPath = std::move(drvPath), .outputs = std::visit(overloaded { [&](const ExtendedOutputsSpec::Default & d) -> OutputsSpec { @@ -160,14 +156,30 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() }, }, extendedOutputsSpec.raw()), }, - .info = { - .priority = priority, - .originalRef = flakeRef, - .resolvedRef = getLockedFlake()->flake.lockedRef, - .attrPath = attrPath, - .extendedOutputsSpec = extendedOutputsSpec, - } - }}; + }; +} + +ExtraPathInfo InstallableFlake::getExtraPathInfo() +{ + auto attr = getCursor(*state); + + auto attrPath = attr->getAttrPathStr(); + + std::optional priority; + + if (attr->maybeGetAttr(state->sOutputSpecified)) { + } else if (auto aMeta = attr->maybeGetAttr(state->sMeta)) { + if (auto aPriority = aMeta->maybeGetAttr("priority")) + priority = aPriority->getInt(); + } + + return { + .priority = priority, + .originalRef = flakeRef, + .resolvedRef = getLockedFlake()->flake.lockedRef, + .attrPath = attrPath, + .extendedOutputsSpec = extendedOutputsSpec, + }; } std::pair InstallableFlake::toValue(EvalState & state) @@ -212,6 +224,7 @@ std::shared_ptr InstallableFlake::getLockedFlake() const { if (!_lockedFlake) { flake::LockFlags lockFlagsApplyConfig = lockFlags; + // FIXME why this side effect? lockFlagsApplyConfig.applyNixConfig = true; _lockedFlake = std::make_shared(lockFlake(*state, flakeRef, lockFlagsApplyConfig)); } @@ -229,7 +242,7 @@ FlakeRef InstallableFlake::nixpkgsFlakeRef() const } } - return Installable::nixpkgsFlakeRef(); + return InstallableValue::nixpkgsFlakeRef(); } } diff --git a/src/libcmd/installable-flake.hh b/src/libcmd/installable-flake.hh index c75765086c18..d41492cc2322 100644 --- a/src/libcmd/installable-flake.hh +++ b/src/libcmd/installable-flake.hh @@ -29,7 +29,9 @@ struct InstallableFlake : InstallableValue Value * getFlakeOutputs(EvalState & state, const flake::LockedFlake & lockedFlake); - DerivedPathsWithInfo toDerivedPaths() override; + DerivedPaths toDerivedPaths() override; + + ExtraPathInfo getExtraPathInfo() override; std::pair toValue(EvalState & state) override; diff --git a/src/libcmd/installable-value.hh b/src/libcmd/installable-value.hh index 682c8d9421c6..f20d447732df 100644 --- a/src/libcmd/installable-value.hh +++ b/src/libcmd/installable-value.hh @@ -1,9 +1,15 @@ #pragma once #include "installables.hh" +#include "flake/flake.hh" namespace nix { +struct DrvInfo; +struct SourceExprCommand; + +namespace eval_cache { class EvalCache; class AttrCursor; } + struct App { std::vector context; @@ -17,12 +23,26 @@ struct UnresolvedApp App resolve(ref evalStore, ref store); }; +struct ExtraPathInfo +{ + std::optional priority; + std::optional originalRef; + std::optional resolvedRef; + std::optional attrPath; + // FIXME: merge with DerivedPath's 'outputs' field? + std::optional extendedOutputsSpec; +}; + struct InstallableValue : Installable { ref state; InstallableValue(ref state) : state(state) {} + virtual ~InstallableValue() { } + + virtual ExtraPathInfo getExtraPathInfo() = 0; + virtual std::pair toValue(EvalState & state) = 0; /* Get a cursor to each value this Installable could refer to. However @@ -37,6 +57,11 @@ struct InstallableValue : Installable UnresolvedApp toApp(EvalState & state); + virtual FlakeRef nixpkgsFlakeRef() const + { + return FlakeRef::fromAttrs({{"type","indirect"}, {"id", "nixpkgs"}}); + } + static InstallableValue & require(Installable & installable); static ref require(ref installable); }; diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index e2164ec72d33..923e508d3c76 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -356,7 +356,7 @@ void completeFlakeRef(ref store, std::string_view prefix) } } -DerivedPathWithInfo Installable::toDerivedPath() +DerivedPath Installable::toDerivedPath() { auto buildables = toDerivedPaths(); if (buildables.size() != 1) @@ -515,19 +515,13 @@ std::vector, BuiltPathWithResult>> Installable::build if (mode == Realise::Nothing) settings.readOnlyMode = true; - struct Aux - { - ExtraPathInfo info; - ref installable; - }; - std::vector pathsToBuild; - std::map> backmap; + std::map>> backmap; for (auto & i : installables) { for (auto b : i->toDerivedPaths()) { - pathsToBuild.push_back(b.path); - backmap[b.path].push_back({.info = b.info, .installable = i}); + pathsToBuild.push_back(b); + backmap[b].push_back(i); } } @@ -540,18 +534,18 @@ std::vector, BuiltPathWithResult>> Installable::build printMissing(store, pathsToBuild, lvlError); for (auto & path : pathsToBuild) { - for (auto & aux : backmap[path]) { + for (auto & installable : backmap[path]) { std::visit(overloaded { [&](const DerivedPath::Built & bfd) { auto outputs = resolveDerivedPath(*store, bfd, &*evalStore); - res.push_back({aux.installable, { + res.push_back({installable, { .path = BuiltPath::Built { bfd.drvPath, outputs }, - .info = aux.info}}); + }}); }, [&](const DerivedPath::Opaque & bo) { - res.push_back({aux.installable, { + res.push_back({installable, { .path = BuiltPath::Opaque { bo.path }, - .info = aux.info}}); + }}); }, }, path.raw()); } @@ -567,21 +561,19 @@ std::vector, BuiltPathWithResult>> Installable::build if (!buildResult.success()) buildResult.rethrow(); - for (auto & aux : backmap[buildResult.path]) { + for (auto & installable : 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({aux.installable, { + res.push_back({installable, { .path = BuiltPath::Built { bfd.drvPath, outputs }, - .info = aux.info, .result = buildResult}}); }, [&](const DerivedPath::Opaque & bo) { - res.push_back({aux.installable, { + res.push_back({installable, { .path = BuiltPath::Opaque { bo.path }, - .info = aux.info, .result = buildResult}}); }, }, buildResult.path.raw()); @@ -670,7 +662,7 @@ StorePathSet Installable::toDerivations( [&](const DerivedPath::Built & bfd) { drvPaths.insert(bfd.drvPath); }, - }, b.path.raw()); + }, b.raw()); return drvPaths; } diff --git a/src/libcmd/installables.hh b/src/libcmd/installables.hh index c5f3cd6837c3..6a55e84ce66f 100644 --- a/src/libcmd/installables.hh +++ b/src/libcmd/installables.hh @@ -4,9 +4,7 @@ #include "path.hh" #include "outputs-spec.hh" #include "derived-path.hh" -#include "eval.hh" #include "store-api.hh" -#include "flake/flake.hh" #include "build-result.hh" #include @@ -14,9 +12,6 @@ namespace nix { struct DrvInfo; -struct SourceExprCommand; - -namespace eval_cache { class EvalCache; class AttrCursor; } enum class Realise { /* Build the derivation. Postcondition: the @@ -39,33 +34,12 @@ enum class OperateOn { Derivation }; -struct ExtraPathInfo -{ - std::optional priority; - std::optional originalRef; - std::optional resolvedRef; - std::optional attrPath; - // FIXME: merge with DerivedPath's 'outputs' field? - std::optional extendedOutputsSpec; -}; - -/* A derived path with any additional info that commands might - need from the derivation. */ -struct DerivedPathWithInfo -{ - DerivedPath path; - ExtraPathInfo info; -}; - struct BuiltPathWithResult { BuiltPath path; - ExtraPathInfo info; std::optional result; }; -typedef std::vector DerivedPathsWithInfo; - struct Installable; typedef std::vector> Installables; @@ -75,9 +49,9 @@ struct Installable virtual std::string what() const = 0; - virtual DerivedPathsWithInfo toDerivedPaths() = 0; + virtual DerivedPaths toDerivedPaths() = 0; - DerivedPathWithInfo toDerivedPath(); + DerivedPath toDerivedPath(); /* Return a value only if this installable is a store path or a symlink to it. */ @@ -86,11 +60,6 @@ struct Installable return {}; } - virtual FlakeRef nixpkgsFlakeRef() const - { - return FlakeRef::fromAttrs({{"type","indirect"}, {"id", "nixpkgs"}}); - } - static std::vector build( ref evalStore, ref store, diff --git a/src/nix/build.cc b/src/nix/build.cc index f8593135e68e..f1716f6f4d73 100644 --- a/src/nix/build.cc +++ b/src/nix/build.cc @@ -1,4 +1,3 @@ -#include "eval.hh" #include "command.hh" #include "common-args.hh" #include "shared.hh" @@ -96,7 +95,7 @@ struct CmdBuild : InstallablesCommand, MixDryRun, MixJSON, MixProfile for (auto & i : installables) for (auto & b : i->toDerivedPaths()) - pathsToBuild.push_back(b.path); + pathsToBuild.push_back(b); printMissing(store, pathsToBuild, lvlError); diff --git a/src/nix/bundle.cc b/src/nix/bundle.cc index 7c32a360e7c1..57c355f0ce4c 100644 --- a/src/nix/bundle.cc +++ b/src/nix/bundle.cc @@ -5,6 +5,7 @@ #include "store-api.hh" #include "local-fs-store.hh" #include "fs-accessor.hh" +#include "eval-inline.hh" using namespace nix; diff --git a/src/nix/develop.cc b/src/nix/develop.cc index f06ade008962..d8de20586509 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -1,6 +1,6 @@ #include "eval.hh" -#include "command.hh" #include "installable-flake.hh" +#include "command-installable-value.hh" #include "common-args.hh" #include "shared.hh" #include "store-api.hh" @@ -252,7 +252,7 @@ static StorePath getDerivationEnvironment(ref store, ref evalStore throw Error("get-env.sh failed to produce an environment"); } -struct Common : InstallableCommand, MixProfile +struct Common : InstallableValueCommand, MixProfile { std::set ignoreVars{ "BASHOPTS", @@ -374,7 +374,7 @@ struct Common : InstallableCommand, MixProfile return res; } - StorePath getShellOutPath(ref store, ref installable) + StorePath getShellOutPath(ref store, ref installable) { auto path = installable->getStorePath(); if (path && hasSuffix(path->to_string(), "-env")) @@ -393,7 +393,7 @@ struct Common : InstallableCommand, MixProfile } std::pair - getBuildEnvironment(ref store, ref installable) + getBuildEnvironment(ref store, ref installable) { auto shellOutPath = getShellOutPath(store, installable); @@ -481,7 +481,7 @@ struct CmdDevelop : Common, MixEnvironment ; } - void run(ref store, ref installable) override + void run(ref store, ref installable) override { auto [buildEnvironment, gcroot] = getBuildEnvironment(store, installable); @@ -605,7 +605,7 @@ struct CmdPrintDevEnv : Common, MixJSON Category category() override { return catUtility; } - void run(ref store, ref installable) override + void run(ref store, ref installable) override { auto buildEnvironment = getBuildEnvironment(store, installable).first; diff --git a/src/nix/log.cc b/src/nix/log.cc index aaf82976404f..8446b0927f1f 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.path.raw()); + }, b.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 d72dd1a136c7..436fe5fc1de2 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -261,12 +261,15 @@ builtPathsPerInstallable( std::map> res; for (auto & [installable, builtPath] : builtPaths) { auto & r = res[&*installable]; + /* Note that there could be conflicting info (e.g. meta.priority fields) if the installable returned multiple derivations. So pick one arbitrarily. FIXME: print a warning? */ r.first.push_back(builtPath.path); - r.second = builtPath.info; + + if (auto * i = dynamic_cast(&*installable)) + r.second = i->getExtraPathInfo(); } return res; } @@ -541,7 +544,7 @@ struct CmdProfileUpgrade : virtual SourceExprCommand, MixDefaultProfile, MixProf auto derivedPaths = installable->toDerivedPaths(); if (derivedPaths.empty()) continue; - auto & info = derivedPaths[0].info; + auto info = installable->getExtraPathInfo(); assert(info.resolvedRef && info.attrPath);