From 256f3e306369131cb13756ed94606d47c343103e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 5 Feb 2023 23:28:18 -0500 Subject: [PATCH] Stratify `ExtraPathInfo` along `Installable` hierarchy Instead of having a bunch of optional fields, have a few subclasses which can have mandatory fields. Additionally, the new `getExtraPathInfo`, and `nixpkgsFlakeRef`, are moved to `InstallableValue`. I did these things 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. Also, add a bunch of doc comments for sake of the internal API docs. --- src/libcmd/command.hh | 2 +- src/libcmd/installable-attr-path.cc | 4 + src/libcmd/installable-derived-path.cc | 5 +- src/libcmd/installable-flake.cc | 26 ++++-- src/libcmd/installable-flake.hh | 30 +++++- src/libcmd/installable-value.hh | 71 +++++++++++++- src/libcmd/installables.cc | 2 +- src/libcmd/installables.hh | 123 ++++++++++++++++++------- src/nix/build.cc | 1 - src/nix/bundle.cc | 1 + src/nix/develop.cc | 12 +-- src/nix/profile.cc | 53 +++++++---- 12 files changed, 254 insertions(+), 76 deletions(-) diff --git a/src/libcmd/command.hh b/src/libcmd/command.hh index 874ca3249..c39901618 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 d9377f0d6..cf513126d 100644 --- a/src/libcmd/installable-attr-path.cc +++ b/src/libcmd/installable-attr-path.cc @@ -87,6 +87,10 @@ DerivedPathsWithInfo InstallableAttrPath::toDerivedPaths() .drvPath = drvPath, .outputs = outputs, }, + .info = make_ref(ExtraPathInfoValue::Value { + /* FIXME: reconsider backwards compatibility above + so we can fill in this info. */ + }), }); return res; diff --git a/src/libcmd/installable-derived-path.cc b/src/libcmd/installable-derived-path.cc index 729dc7d31..6ecf54b7c 100644 --- a/src/libcmd/installable-derived-path.cc +++ b/src/libcmd/installable-derived-path.cc @@ -10,7 +10,10 @@ std::string InstallableDerivedPath::what() const DerivedPathsWithInfo InstallableDerivedPath::toDerivedPaths() { - return {{.path = derivedPath, .info = {} }}; + return {{ + .path = derivedPath, + .info = make_ref(), + }}; } std::optional InstallableDerivedPath::getStorePath() diff --git a/src/libcmd/installable-flake.cc b/src/libcmd/installable-flake.cc index 7b0cc376d..a3352af76 100644 --- a/src/libcmd/installable-flake.cc +++ b/src/libcmd/installable-flake.cc @@ -101,7 +101,8 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() return {{ .path = DerivedPath::Opaque { .path = std::move(storePath), - } + }, + .info = make_ref(), }}; } @@ -113,7 +114,8 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() return {{ .path = DerivedPath::Opaque { .path = std::move(*storePath), - } + }, + .info = make_ref(), }}; } else throw Error("flake output attribute '%s' evaluates to the string '%s' which is not a store path", attrPath, s); @@ -160,13 +162,16 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() }, }, extendedOutputsSpec.raw()), }, - .info = { - .priority = priority, - .originalRef = flakeRef, - .resolvedRef = getLockedFlake()->flake.lockedRef, - .attrPath = attrPath, - .extendedOutputsSpec = extendedOutputsSpec, - } + .info = make_ref( + ExtraPathInfoValue::Value { + .priority = priority, + .attrPath = attrPath, + .extendedOutputsSpec = extendedOutputsSpec, + }, + ExtraPathInfoFlake::Flake { + .originalRef = flakeRef, + .resolvedRef = getLockedFlake()->flake.lockedRef, + }), }}; } @@ -212,6 +217,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 +235,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 c75765086..313d2d7a3 100644 --- a/src/libcmd/installable-flake.hh +++ b/src/libcmd/installable-flake.hh @@ -4,6 +4,30 @@ namespace nix { +/** + * Extra info about a \ref DerivedPath "derived path" that ultimately + * come from a Flake. + * + * Invariant: every ExtraPathInfo gotten from an InstallableFlake should + * be possible to downcast to an ExtraPathInfoFlake. + */ +struct ExtraPathInfoFlake : ExtraPathInfoValue +{ + /** + * Extra struct to get around C++ designated initializer limitations + */ + struct Flake { + FlakeRef originalRef; + FlakeRef resolvedRef; + }; + + Flake flake; + + ExtraPathInfoFlake(Value && v, Flake && f) + : ExtraPathInfoValue(std::move(v)), flake(f) + { } +}; + struct InstallableFlake : InstallableValue { FlakeRef flakeRef; @@ -33,8 +57,10 @@ struct InstallableFlake : InstallableValue std::pair toValue(EvalState & state) override; - /* Get a cursor to every attrpath in getActualAttrPaths() - that exists. However if none exists, throw an exception. */ + /** + * Get a cursor to every attrpath in getActualAttrPaths() that + * exists. However if none exists, throw an exception. + */ std::vector> getCursors(EvalState & state) override; diff --git a/src/libcmd/installable-value.hh b/src/libcmd/installable-value.hh index 682c8d942..9e076cb10 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,26 +23,83 @@ struct UnresolvedApp App resolve(ref evalStore, ref store); }; +/** + * Extra info about a \ref DerivedPath "derived path" that ultimately + * come from a Nix language value. + * + * Invariant: every ExtraPathInfo gotten from an InstallableValue should + * be possible to downcast to an ExtraPathInfoValue. + */ +struct ExtraPathInfoValue : ExtraPathInfo +{ + /** + * Extra struct to get around C++ designated initializer limitations + */ + struct Value { + /** + * An optional priority for use with "build envs". See Package + */ + std::optional priority; + + /** + * The attribute path associated with this value. The idea is + * that an installable referring to a value typically refers to + * a larger value, from which we project a smaller value out + * with this. + */ + std::string attrPath; + + /** + * \todo merge with DerivedPath's 'outputs' field? + */ + ExtendedOutputsSpec extendedOutputsSpec; + }; + + Value value; + + ExtraPathInfoValue(Value && v) + : value(v) + { } + + virtual ~ExtraPathInfoValue() = default; +}; + +/** + * An Installable which corresponds a Nix langauge value, in addition to + * a collection of \ref DerivedPath "derived paths". + */ struct InstallableValue : Installable { ref state; InstallableValue(ref state) : state(state) {} + virtual ~InstallableValue() { } + virtual std::pair toValue(EvalState & state) = 0; - /* Get a cursor to each value this Installable could refer to. However - if none exists, throw exception instead of returning empty vector. */ + /** + * Get a cursor to each value this Installable could refer to. + * However if none exists, throw exception instead of returning + * empty vector. + */ virtual std::vector> getCursors(EvalState & state); - /* Get the first and most preferred cursor this Installable could refer - to, or throw an exception if none exists. */ + /** + * Get the first and most preferred cursor this Installable could + * refer to, or throw an exception if none exists. + */ virtual ref getCursor(EvalState & state); 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 e2164ec72..d7120df19 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -517,7 +517,7 @@ std::vector, BuiltPathWithResult>> Installable::build struct Aux { - ExtraPathInfo info; + ref info; ref installable; }; diff --git a/src/libcmd/installables.hh b/src/libcmd/installables.hh index c5f3cd683..b6efc0f17 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,83 +12,144 @@ namespace nix { struct DrvInfo; -struct SourceExprCommand; - -namespace eval_cache { class EvalCache; class AttrCursor; } enum class Realise { - /* Build the derivation. Postcondition: the - derivation outputs exist. */ + /** + * Build the derivation. + * + * Postcondition: the derivation outputs exist. + */ Outputs, - /* Don't build the derivation. Postcondition: the store derivation - exists. */ + /** + * Don't build the derivation. + * + * Postcondition: the store derivation exists. + */ Derivation, - /* Evaluate in dry-run mode. Postcondition: nothing. */ - // FIXME: currently unused, but could be revived if we can - // evaluate derivations in-memory. + /** + * Evaluate in dry-run mode. + * + * Postcondition: nothing. + * + * \todo currently unused, but could be revived if we can evaluate + * derivations in-memory. + */ Nothing }; -/* How to handle derivations in commands that operate on store paths. */ +/** + * How to handle derivations in commands that operate on store paths. + */ enum class OperateOn { - /* Operate on the output path. */ + /** + * Operate on the output path. + */ Output, - /* Operate on the .drv path. */ + /** + * Operate on the .drv path. + */ Derivation }; +/** + * Extra info about a DerivedPath + * + * Yes, this is empty, but that is intended. It will be sub-classed by + * the subclasses of Installable to allow those to provide more info. + * Certain commands will make use of this info. + */ struct ExtraPathInfo { - std::optional priority; - std::optional originalRef; - std::optional resolvedRef; - std::optional attrPath; - // FIXME: merge with DerivedPath's 'outputs' field? - std::optional extendedOutputsSpec; + virtual ~ExtraPathInfo() = default; }; -/* A derived path with any additional info that commands might - need from the derivation. */ +/** + * A DerivedPath with \ref ExtraPathInfo "any additional info" that + * commands might need from the derivation. + */ struct DerivedPathWithInfo { DerivedPath path; - ExtraPathInfo info; + ref info; }; +/** + * Like DerivedPathWithInfo but extending BuiltPath with \ref + * ExtraPathInfo "extra info" and also possibly the \ref BuildResult + * "result of building". + */ struct BuiltPathWithResult { BuiltPath path; - ExtraPathInfo info; + ref info; std::optional result; }; +/** + * Shorthand, for less typing and helping us keep the choice of + * collection in sync. + */ typedef std::vector DerivedPathsWithInfo; struct Installable; + +/** + * Shorthand, for less typing and helping us keep the choice of + * collection in sync. + */ typedef std::vector> Installables; +/** + * Installables are the main positional arguments for the Nix + * Command-line. + * + * This base class is very flexible, and just assumes and the + * Installable refers to a collection of \ref DerivedPath "derived paths" with + * \ref ExtraPathInfo "extra info". + */ struct Installable { virtual ~Installable() { } + /** + * What Installable is this? + * + * Prints back valid CLI syntax that would result in this same + * installable. It doesn't need to be exactly what the user wrote, + * just something that means the same thing. + */ virtual std::string what() const = 0; + /** + * Get the collection of \ref DerivedPathWithInfo "derived paths + * with info" that this \ref Installable instalallable denotes. + * + * This is the main method of this class + */ virtual DerivedPathsWithInfo toDerivedPaths() = 0; + /** + * A convenience wrapper of the above for when we expect an + * installable to produce a single \ref DerivedPath "derived path" + * only. + * + * If no or multiple \ref DerivedPath "derived paths" are produced, + * and error is raised. + */ DerivedPathWithInfo toDerivedPath(); - /* Return a value only if this installable is a store path or a - symlink to it. */ + /** + * Return a value only if this installable is a store path or a + * symlink to it. + * + * \todo should we move this to InstallableDerivedPath? It is only + * supposed to work there anyways. Can always downcast. + */ virtual std::optional getStorePath() { 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 bca20e97c..4e133e288 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" diff --git a/src/nix/bundle.cc b/src/nix/bundle.cc index 7c32a360e..57c355f0c 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 f06ade008..d8de20586 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/profile.cc b/src/nix/profile.cc index d72dd1a13..88d4a3ce5 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -254,13 +254,19 @@ 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]; + auto & r = res.insert({ + &*installable, + { + {}, + make_ref(), + } + }).first->second; /* Note that there could be conflicting info (e.g. meta.priority fields) if the installable returned multiple derivations. So pick one arbitrarily. FIXME: @@ -307,14 +313,16 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile for (auto & installable : installables) { ProfileElement element; - auto & [res, info] = builtPaths[&*installable]; + auto iter = builtPaths.find(&*installable); + if (iter == builtPaths.end()) continue; + auto & [res, info] = iter->second; - if (info.originalRef && info.resolvedRef && info.attrPath && info.extendedOutputsSpec) { + if (auto * info2 = dynamic_cast(&*info)) { element.source = ProfileElementSource { - .originalRef = *info.originalRef, - .resolvedRef = *info.resolvedRef, - .attrPath = *info.attrPath, - .outputs = *info.extendedOutputsSpec, + .originalRef = info2->flake.originalRef, + .resolvedRef = info2->flake.resolvedRef, + .attrPath = info2->value.attrPath, + .outputs = info2->value.extendedOutputsSpec, }; } @@ -323,7 +331,12 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile element.priority = priority ? *priority - : info.priority.value_or(defaultPriority); + : ({ + auto * info2 = dynamic_cast(&*info); + info2 + ? info2->value.priority.value_or(defaultPriority) + : defaultPriority; + }); element.updateStorePaths(getEvalStore(), store, res); @@ -541,19 +554,20 @@ struct CmdProfileUpgrade : virtual SourceExprCommand, MixDefaultProfile, MixProf auto derivedPaths = installable->toDerivedPaths(); if (derivedPaths.empty()) continue; - auto & info = derivedPaths[0].info; + auto * infop = dynamic_cast(&*derivedPaths[0].info); + // `InstallableFlake` should use `ExtraPathInfoFlake`. + assert(infop); + auto & info = *infop; - assert(info.resolvedRef && info.attrPath); - - if (element.source->resolvedRef == info.resolvedRef) continue; + if (element.source->resolvedRef == info.flake.resolvedRef) continue; printInfo("upgrading '%s' from flake '%s' to '%s'", - element.source->attrPath, element.source->resolvedRef, *info.resolvedRef); + element.source->attrPath, element.source->resolvedRef, info.flake.resolvedRef); element.source = ProfileElementSource { .originalRef = installable->flakeRef, - .resolvedRef = *info.resolvedRef, - .attrPath = *info.attrPath, + .resolvedRef = info.flake.resolvedRef, + .attrPath = info.value.attrPath, .outputs = installable->extendedOutputsSpec, }; @@ -582,7 +596,10 @@ 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].first); + element.updateStorePaths( + getEvalStore(), + store, + builtPaths.find(&*installable)->second.first); } updateProfile(manifest.build(store));