Skip to content

Commit

Permalink
Make command infra less stateful and more regular
Browse files Browse the repository at this point in the history
Already, we had classes like `BuiltPathsCommand` and `StorePathsCommand`
which provided alternative `run` virtual functions providing the
implementation with more arguments. This was a very nice and easy way to
make writing command; just fill in the virtual functions and it is
fairly clear what to do.

However, exception to this pattern were `Installable{,s}Command`. These
two classes instead just had a field where the installables would be
stored, and various side-effecting `prepare` and `load` machinery too
fill them in. Command would wish out those fields.

This isn't so clear to use.

What this commit does is make those command classes like the others, with richer run functions.

Not only does this restore the pattern making commands easier to write,
it has a number of other benefits:

- `prepare` and `load` are gone entirely! One command just hands just
  hands off to the next.

- We can use `ref` instead of `std::shared_ptr`. The former must be
  initialized (so it is like Rust's `Box` rather than `Option<Box>`,
  This expresses the invariant that the installable are in fact
  initialized much better.

  This is possible because since we just have local variables not
  fields, we can stop worrying about the not-yet-initialized case.

- Fewer lines of code! (Finally I have a large refactor that makes the
  number go down not up...)

- `nix repl` is now implemented in a clearer way.

The last item deserves further mention. `nix repl` is not like the other
installable commands because instead working from once-loaded
installables, it needs to be able to load them again and again.

To properly support this, we make a new superclass
`RawInstallablesCommand`. This class has the argument parsing and
completion logic, but does *not* hand off parsed installables but
instead just the raw string arguments.

This is exactly what `nix repl` needs, and allows us to instead of
having the logic awkwardly split between `prepare`,
`useDefaultInstallables,` and `load`, have everything right next to each
other. I think this will enable future simplifications of that argument
defaulting logic, but I am saving those for a future PR --- best to keep
code motion and more complicated boolean expression rewriting separate
steps.

Finally do note that I stopped overloading the `run` functions. The
reason was we are liable to get `-Woverloaded-virtual` warnings from
this. Yes, there is a workaround in putting `using
BaseClass::virtual_method;` next to overrides, but since I made the
`run`-delegation chain cheaper that would have required *far* more
`using` statements. I figured it was less obnoxious to just slightly
vary the names than force all the commands to do that.

Helps with NixOS/rfcs#134
  • Loading branch information
Ericson2314 committed Mar 6, 2023
1 parent 02bf521 commit cbeb4e6
Show file tree
Hide file tree
Showing 33 changed files with 140 additions and 143 deletions.
14 changes: 7 additions & 7 deletions src/libcmd/command.cc
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ BuiltPathsCommand::BuiltPathsCommand(bool recursive)
});
}

void BuiltPathsCommand::run(ref<Store> store)
void BuiltPathsCommand::runIs(ref<Store> store, Installables && installables)
{
BuiltPaths paths;
if (all) {
Expand All @@ -190,15 +190,15 @@ void BuiltPathsCommand::run(ref<Store> store)
}
}

run(store, std::move(paths));
runBPs(store, std::move(paths));
}

StorePathsCommand::StorePathsCommand(bool recursive)
: BuiltPathsCommand(recursive)
{
}

void StorePathsCommand::run(ref<Store> store, BuiltPaths && paths)
void StorePathsCommand::runBPs(ref<Store> store, BuiltPaths && paths)
{
StorePathSet storePaths;
for (auto & builtPath : paths)
Expand All @@ -208,15 +208,15 @@ void StorePathsCommand::run(ref<Store> store, BuiltPaths && paths)
auto sorted = store->topoSortPaths(storePaths);
std::reverse(sorted.begin(), sorted.end());

run(store, std::move(sorted));
runSPs(store, std::move(sorted));
}

void StorePathCommand::run(ref<Store> store, std::vector<StorePath> && storePaths)
void StorePathCommand::runSPs(ref<Store> store, StorePaths && storePaths)
{
if (storePaths.size() != 1)
throw UsageError("this command requires exactly one store path");

run(store, *storePaths.begin());
runSP(store, *storePaths.begin());
}

MixProfile::MixProfile()
Expand Down Expand Up @@ -246,7 +246,7 @@ void MixProfile::updateProfile(const BuiltPaths & buildables)
{
if (!profile) return;

std::vector<StorePath> result;
StorePaths result;

for (auto & buildable : buildables) {
std::visit(overloaded {
Expand Down
54 changes: 28 additions & 26 deletions src/libcmd/command.hh
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,10 @@ struct SourceExprCommand : virtual Args, MixFlakeOptions

SourceExprCommand();

std::vector<std::shared_ptr<Installable>> parseInstallables(
Installables parseInstallables(
ref<Store> store, std::vector<std::string> ss);

std::shared_ptr<Installable> parseInstallable(
ref<Installable> parseInstallable(
ref<Store> store, const std::string & installable);

virtual Strings getDefaultFlakeAttrPaths();
Expand All @@ -115,36 +115,44 @@ struct MixReadOnlyOption : virtual Args
MixReadOnlyOption();
};

/* A command that operates on a list of "installables", which can be
store paths, attribute paths, Nix expressions, etc. */
struct InstallablesCommand : virtual Args, SourceExprCommand
/* Like InstallablesCommand but the installables are not loaded */
struct RawInstallablesCommand : virtual Args, SourceExprCommand
{
std::vector<std::shared_ptr<Installable>> installables;
RawInstallablesCommand();

InstallablesCommand();
virtual void runRIs(ref<Store> store, std::vector<std::string> && rawInstallables) = 0;

void prepare() override;
Installables load();
void run(ref<Store> store) override;

virtual bool useDefaultInstallables() { return true; }

bool readFromStdIn;

std::vector<std::string> getFlakesForCompletion() override;

protected:
private:

std::vector<std::string> _installables;
std::vector<std::string> rawInstallables;
};
/* A command that operates on a list of "installables", which can be
store paths, attribute paths, Nix expressions, etc. */
struct InstallablesCommand : RawInstallablesCommand
{
virtual void runIs(ref<Store> store, Installables && installables) = 0;

void runRIs(ref<Store> store, std::vector<std::string> && rawInstallables) override;

std::vector<std::string> getFlakesForCompletion() override;
};

/* A command that operates on exactly one "installable" */
struct InstallableCommand : virtual Args, SourceExprCommand
{
std::shared_ptr<Installable> installable;

InstallableCommand();

void prepare() override;
virtual void runI(ref<Store> store, ref<Installable> installable) = 0;

void run(ref<Store> store) override;

std::vector<std::string> getFlakesForCompletion() override
{
Expand Down Expand Up @@ -179,11 +187,9 @@ public:

BuiltPathsCommand(bool recursive = false);

using StoreCommand::run;
virtual void runBPs(ref<Store> store, BuiltPaths && paths) = 0;

virtual void run(ref<Store> store, BuiltPaths && paths) = 0;

void run(ref<Store> store) override;
void runIs(ref<Store> store, Installables && installables) override;

bool useDefaultInstallables() override { return !all; }
};
Expand All @@ -192,21 +198,17 @@ struct StorePathsCommand : public BuiltPathsCommand
{
StorePathsCommand(bool recursive = false);

using BuiltPathsCommand::run;
virtual void runSPs(ref<Store> store, StorePaths && storePaths) = 0;

virtual void run(ref<Store> store, std::vector<StorePath> && storePaths) = 0;

void run(ref<Store> store, BuiltPaths && paths) override;
void runBPs(ref<Store> store, BuiltPaths && paths) override;
};

/* A command that operates on exactly one store path. */
struct StorePathCommand : public StorePathsCommand
{
using StorePathsCommand::run;

virtual void run(ref<Store> store, const StorePath & storePath) = 0;
virtual void runSP(ref<Store> store, const StorePath & storePath) = 0;

void run(ref<Store> store, std::vector<StorePath> && storePaths) override;
void runSPs(ref<Store> store, StorePaths && storePaths) override;
};

/* A helper class for registering commands globally. */
Expand Down
74 changes: 41 additions & 33 deletions src/libcmd/installables.cc
Original file line number Diff line number Diff line change
Expand Up @@ -422,10 +422,10 @@ ref<eval_cache::EvalCache> openEvalCache(
});
}

std::vector<std::shared_ptr<Installable>> SourceExprCommand::parseInstallables(
Installables SourceExprCommand::parseInstallables(
ref<Store> store, std::vector<std::string> ss)
{
std::vector<std::shared_ptr<Installable>> result;
Installables result;

if (file || expr) {
if (file && expr)
Expand All @@ -451,7 +451,7 @@ std::vector<std::shared_ptr<Installable>> SourceExprCommand::parseInstallables(
for (auto & s : ss) {
auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(s);
result.push_back(
std::make_shared<InstallableAttrPath>(
make_ref<InstallableAttrPath>(
InstallableAttrPath::parse(
state, *this, vFile, prefix, extendedOutputsSpec)));
}
Expand All @@ -468,7 +468,7 @@ std::vector<std::shared_ptr<Installable>> SourceExprCommand::parseInstallables(

if (prefix.find('/') != std::string::npos) {
try {
result.push_back(std::make_shared<InstallableDerivedPath>(
result.push_back(make_ref<InstallableDerivedPath>(
InstallableDerivedPath::parse(store, prefix, extendedOutputsSpec)));
continue;
} catch (BadStorePath &) {
Expand All @@ -480,7 +480,7 @@ std::vector<std::shared_ptr<Installable>> SourceExprCommand::parseInstallables(

try {
auto [flakeRef, fragment] = parseFlakeRefWithFragment(std::string { prefix }, absPath("."));
result.push_back(std::make_shared<InstallableFlake>(
result.push_back(make_ref<InstallableFlake>(
this,
getEvalState(),
std::move(flakeRef),
Expand All @@ -501,7 +501,7 @@ std::vector<std::shared_ptr<Installable>> SourceExprCommand::parseInstallables(
return result;
}

std::shared_ptr<Installable> SourceExprCommand::parseInstallable(
ref<Installable> SourceExprCommand::parseInstallable(
ref<Store> store, const std::string & installable)
{
auto installables = parseInstallables(store, {installable});
Expand All @@ -513,7 +513,7 @@ std::vector<BuiltPathWithResult> Installable::build(
ref<Store> evalStore,
ref<Store> store,
Realise mode,
const std::vector<std::shared_ptr<Installable>> & installables,
const Installables & installables,
BuildMode bMode)
{
std::vector<BuiltPathWithResult> res;
Expand All @@ -522,11 +522,11 @@ std::vector<BuiltPathWithResult> Installable::build(
return res;
}

std::vector<std::pair<std::shared_ptr<Installable>, BuiltPathWithResult>> Installable::build2(
std::vector<std::pair<ref<Installable>, BuiltPathWithResult>> Installable::build2(
ref<Store> evalStore,
ref<Store> store,
Realise mode,
const std::vector<std::shared_ptr<Installable>> & installables,
const Installables & installables,
BuildMode bMode)
{
if (mode == Realise::Nothing)
Expand All @@ -535,7 +535,7 @@ std::vector<std::pair<std::shared_ptr<Installable>, BuiltPathWithResult>> Instal
struct Aux
{
ExtraPathInfo info;
std::shared_ptr<Installable> installable;
ref<Installable> installable;
};

std::vector<DerivedPath> pathsToBuild;
Expand All @@ -548,7 +548,7 @@ std::vector<std::pair<std::shared_ptr<Installable>, BuiltPathWithResult>> Instal
}
}

std::vector<std::pair<std::shared_ptr<Installable>, BuiltPathWithResult>> res;
std::vector<std::pair<ref<Installable>, BuiltPathWithResult>> res;

switch (mode) {

Expand Down Expand Up @@ -620,7 +620,7 @@ BuiltPaths Installable::toBuiltPaths(
ref<Store> store,
Realise mode,
OperateOn operateOn,
const std::vector<std::shared_ptr<Installable>> & installables)
const Installables & installables)
{
if (operateOn == OperateOn::Output) {
BuiltPaths res;
Expand All @@ -642,7 +642,7 @@ StorePathSet Installable::toStorePaths(
ref<Store> evalStore,
ref<Store> store,
Realise mode, OperateOn operateOn,
const std::vector<std::shared_ptr<Installable>> & installables)
const Installables & installables)
{
StorePathSet outPaths;
for (auto & path : toBuiltPaths(evalStore, store, mode, operateOn, installables)) {
Expand All @@ -656,7 +656,7 @@ StorePath Installable::toStorePath(
ref<Store> evalStore,
ref<Store> store,
Realise mode, OperateOn operateOn,
std::shared_ptr<Installable> installable)
ref<Installable> installable)
{
auto paths = toStorePaths(evalStore, store, mode, operateOn, {installable});

Expand All @@ -668,7 +668,7 @@ StorePath Installable::toStorePath(

StorePathSet Installable::toDerivations(
ref<Store> store,
const std::vector<std::shared_ptr<Installable>> & installables,
const Installables & installables,
bool useDeriver)
{
StorePathSet drvPaths;
Expand All @@ -692,9 +692,8 @@ StorePathSet Installable::toDerivations(
return drvPaths;
}

InstallablesCommand::InstallablesCommand()
RawInstallablesCommand::RawInstallablesCommand()
{

addFlag({
.longName = "stdin",
.description = "Read installables from the standard input.",
Expand All @@ -703,40 +702,48 @@ InstallablesCommand::InstallablesCommand()

expectArgs({
.label = "installables",
.handler = {&_installables},
.handler = {&rawInstallables},
.completer = {[&](size_t, std::string_view prefix) {
completeInstallable(prefix);
}}
});
}

void InstallablesCommand::prepare()
{
installables = load();
}

Installables InstallablesCommand::load()
void RawInstallablesCommand::run(ref<Store> store)
{
if (_installables.empty() && useDefaultInstallables() && !readFromStdIn)
if (rawInstallables.empty() && useDefaultInstallables() && !readFromStdIn) {
// FIXME: commands like "nix profile install" should not have a
// default, probably.
_installables.push_back(".");
rawInstallables.push_back(".");
}

if (readFromStdIn && !isatty(STDIN_FILENO)) {
std::string word;
while (std::cin >> word) {
_installables.emplace_back(std::move(word));
rawInstallables.emplace_back(std::move(word));
}
}

return parseInstallables(getStore(), _installables);
runRIs(store, std::move(rawInstallables));
}

std::vector<std::string> RawInstallablesCommand::getFlakesForCompletion()
{
return rawInstallables;
}

void InstallablesCommand::runRIs(ref<Store> store, std::vector<std::string> && rawInstallables)
{
auto installables = parseInstallables(store, rawInstallables);
runIs(store, std::move(installables));
}

std::vector<std::string> InstallablesCommand::getFlakesForCompletion()
{
if (_installables.empty() && useDefaultInstallables())
return {"."};
return _installables;
auto res = RawInstallablesCommand::getFlakesForCompletion();
if (res.empty() && useDefaultInstallables())
res.push_back(".");
return res;
}

InstallableCommand::InstallableCommand()
Expand All @@ -752,9 +759,10 @@ InstallableCommand::InstallableCommand()
});
}

void InstallableCommand::prepare()
void InstallableCommand::run(ref<Store> store)
{
installable = parseInstallable(getStore(), _installable);
auto installable = parseInstallable(store, _installable);
runI(store, std::move(installable));
}

}
Loading

0 comments on commit cbeb4e6

Please sign in to comment.