Skip to content

Commit

Permalink
Fixing NixOS#7479
Browse files Browse the repository at this point in the history
Types converted:

- `NixStringContextElem`
- `OutputsSpec`
- `ExtendedOutputsSpec`
- `DerivationOutput`
- `DerivationType`

The `DerivationGoal::derivationType` field had a bogus initialization,
now caught, so I made it `std::optional`. I think NixOS#8829 can make it
non-optional again because it will ensure we always have the derivation
when we construct a `DerivationGoal`.

See that issue (NixOS#7479) for details on the general goal.

`git grep 'Raw::Raw'` indicates the two types I didn't yet convert
`DerivedPath` and `BuiltPath` (and their `Single` variants) . This is
because @roberth and I (can't find issue right now...) plan on reworking
them somewhat, so I didn't want to churn them more just yet.

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
  • Loading branch information
Ericson2314 and edolstra committed Aug 18, 2023
1 parent 284c180 commit fedcd53
Show file tree
Hide file tree
Showing 29 changed files with 338 additions and 317 deletions.
5 changes: 3 additions & 2 deletions src/libcmd/installable-attr-path.cc
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ DerivedPathsWithInfo InstallableAttrPath::toDerivedPaths()
[&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec {
return e;
},
}, extendedOutputsSpec.raw());
}, extendedOutputsSpec.raw);

auto [iter, didInsert] = byDrvPath.emplace(*drvPath, newOutputs);

Expand All @@ -96,6 +96,7 @@ DerivedPathsWithInfo InstallableAttrPath::toDerivedPaths()
.outputs = outputs,
},
.info = make_ref<ExtraPathInfoValue>(ExtraPathInfoValue::Value {
.extendedOutputsSpec = outputs,
/* FIXME: reconsider backwards compatibility above
so we can fill in this info. */
}),
Expand All @@ -114,7 +115,7 @@ InstallableAttrPath InstallableAttrPath::parse(
return {
state, cmd, v,
prefix == "." ? "" : std::string { prefix },
extendedOutputsSpec
std::move(extendedOutputsSpec),
};
}

Expand Down
2 changes: 1 addition & 1 deletion src/libcmd/installable-derived-path.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ InstallableDerivedPath InstallableDerivedPath::parse(
.outputs = outputSpec,
};
},
}, extendedOutputsSpec.raw());
}, extendedOutputsSpec.raw);
return InstallableDerivedPath {
store,
std::move(derivedPath),
Expand Down
2 changes: 1 addition & 1 deletion src/libcmd/installable-flake.cc
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths()
[&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec {
return e;
},
}, extendedOutputsSpec.raw()),
}, extendedOutputsSpec.raw),
},
.info = make_ref<ExtraPathInfoFlake>(
ExtraPathInfoValue::Value {
Expand Down
6 changes: 3 additions & 3 deletions src/libcmd/installables.cc
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ Installables SourceExprCommand::parseInstallables(
result.push_back(
make_ref<InstallableAttrPath>(
InstallableAttrPath::parse(
state, *this, vFile, prefix, extendedOutputsSpec)));
state, *this, vFile, std::move(prefix), std::move(extendedOutputsSpec))));
}

} else {
Expand All @@ -475,7 +475,7 @@ Installables SourceExprCommand::parseInstallables(
if (prefix.find('/') != std::string::npos) {
try {
result.push_back(make_ref<InstallableDerivedPath>(
InstallableDerivedPath::parse(store, prefix, extendedOutputsSpec)));
InstallableDerivedPath::parse(store, prefix, extendedOutputsSpec.raw)));
continue;
} catch (BadStorePath &) {
} catch (...) {
Expand All @@ -491,7 +491,7 @@ Installables SourceExprCommand::parseInstallables(
getEvalState(),
std::move(flakeRef),
fragment,
extendedOutputsSpec,
std::move(extendedOutputsSpec),
getDefaultFlakeAttrPaths(),
getDefaultFlakeAttrPathPrefixes(),
lockFlags));
Expand Down
2 changes: 1 addition & 1 deletion src/libexpr/eval-cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ string_t AttrCursor::getStringWithContext()
[&](const NixStringContextElem::Opaque & o) -> const StorePath & {
return o.path;
},
}, c.raw());
}, c.raw);
if (!root->state.store->isValidPath(path)) {
valid = false;
break;
Expand Down
2 changes: 1 addition & 1 deletion src/libexpr/eval.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2364,7 +2364,7 @@ std::pair<SingleDerivedPath, std::string_view> EvalState::coerceToSingleDerivedP
[&](NixStringContextElem::Built && b) -> SingleDerivedPath {
return std::move(b);
},
}, ((NixStringContextElem &&) *context.begin()).raw());
}, ((NixStringContextElem &&) *context.begin()).raw);
return {
std::move(derivedPath),
std::move(s),
Expand Down
2 changes: 1 addition & 1 deletion src/libexpr/flake/flakeref.cc
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ std::tuple<FlakeRef, std::string, ExtendedOutputsSpec> parseFlakeRefWithFragment
{
auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(url);
auto [flakeRef, fragment] = parseFlakeRefWithFragment(std::string { prefix }, baseDir, allowMissing, isFlake);
return {std::move(flakeRef), fragment, extendedOutputsSpec};
return {std::move(flakeRef), fragment, std::move(extendedOutputsSpec)};
}

}
14 changes: 7 additions & 7 deletions src/libexpr/primops.cc
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ StringMap EvalState::realiseContext(const NixStringContext & context)
res.insert_or_assign(ctxS, ctxS);
ensureValid(d.drvPath);
},
}, c.raw());
}, c.raw);
}

if (drvs.empty()) return {};
Expand Down Expand Up @@ -1265,7 +1265,7 @@ drvName, Bindings * attrs, Value & v)
[&](const NixStringContextElem::Opaque & o) {
drv.inputSrcs.insert(o.path);
},
}, c.raw());
}, c.raw);
}

/* Do we have all required attributes? */
Expand Down Expand Up @@ -1334,13 +1334,13 @@ drvName, Bindings * attrs, Value & v)
if (isImpure)
drv.outputs.insert_or_assign(i,
DerivationOutput::Impure {
.method = method.raw,
.method = method,
.hashType = ht,
});
else
drv.outputs.insert_or_assign(i,
DerivationOutput::CAFloating {
.method = method.raw,
.method = method,
.hashType = ht,
});
}
Expand Down Expand Up @@ -1373,15 +1373,15 @@ drvName, Bindings * attrs, Value & v)
drv.env[i] = state.store->printStorePath(outPath);
drv.outputs.insert_or_assign(
i,
DerivationOutputInputAddressed {
DerivationOutput::InputAddressed {
.path = std::move(outPath),
});
}
break;
;
case DrvHash::Kind::Deferred:
for (auto & i : outputs) {
drv.outputs.insert_or_assign(i, DerivationOutputDeferred {});
drv.outputs.insert_or_assign(i, DerivationOutput::Deferred {});
}
}
}
Expand Down Expand Up @@ -2054,7 +2054,7 @@ static void prim_toFile(EvalState & state, const PosIdx pos, Value * * args, Val
StorePathSet refs;

for (auto c : context) {
if (auto p = std::get_if<NixStringContextElem::Opaque>(&c))
if (auto p = std::get_if<NixStringContextElem::Opaque>(&c.raw))
refs.insert(p->path);
else
state.debugThrowLastTrace(EvalError({
Expand Down
6 changes: 3 additions & 3 deletions src/libexpr/primops/context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,13 @@ static void prim_unsafeDiscardOutputDependency(EvalState & state, const PosIdx p

NixStringContext context2;
for (auto && c : context) {
if (auto * ptr = std::get_if<NixStringContextElem::DrvDeep>(&c)) {
if (auto * ptr = std::get_if<NixStringContextElem::DrvDeep>(&c.raw)) {
context2.emplace(NixStringContextElem::Opaque {
.path = ptr->drvPath
});
} else {
/* Can reuse original item */
context2.emplace(std::move(c));
context2.emplace(std::move(c).raw);
}
}

Expand Down Expand Up @@ -114,7 +114,7 @@ static void prim_getContext(EvalState & state, const PosIdx pos, Value * * args,
[&](NixStringContextElem::Opaque && o) {
contextInfos[std::move(o.path)].path = true;
},
}, ((NixStringContextElem &&) i).raw());
}, ((NixStringContextElem &&) i).raw);
}

auto attrs = state.buildBindings(contextInfos.size());
Expand Down
8 changes: 4 additions & 4 deletions src/libexpr/tests/value/context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ TEST(NixStringContextElemTest, slash_invalid) {
TEST(NixStringContextElemTest, opaque) {
std::string_view opaque = "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x";
auto elem = NixStringContextElem::parse(opaque);
auto * p = std::get_if<NixStringContextElem::Opaque>(&elem);
auto * p = std::get_if<NixStringContextElem::Opaque>(&elem.raw);
ASSERT_TRUE(p);
ASSERT_EQ(p->path, StorePath { opaque });
ASSERT_EQ(elem.to_string(), opaque);
Expand All @@ -60,7 +60,7 @@ TEST(NixStringContextElemTest, opaque) {
TEST(NixStringContextElemTest, drvDeep) {
std::string_view drvDeep = "=g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x.drv";
auto elem = NixStringContextElem::parse(drvDeep);
auto * p = std::get_if<NixStringContextElem::DrvDeep>(&elem);
auto * p = std::get_if<NixStringContextElem::DrvDeep>(&elem.raw);
ASSERT_TRUE(p);
ASSERT_EQ(p->drvPath, StorePath { drvDeep.substr(1) });
ASSERT_EQ(elem.to_string(), drvDeep);
Expand All @@ -73,7 +73,7 @@ TEST(NixStringContextElemTest, drvDeep) {
TEST(NixStringContextElemTest, built_opaque) {
std::string_view built = "!foo!g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x.drv";
auto elem = NixStringContextElem::parse(built);
auto * p = std::get_if<NixStringContextElem::Built>(&elem);
auto * p = std::get_if<NixStringContextElem::Built>(&elem.raw);
ASSERT_TRUE(p);
ASSERT_EQ(p->output, "foo");
ASSERT_EQ(*p->drvPath, ((SingleDerivedPath) SingleDerivedPath::Opaque {
Expand All @@ -96,7 +96,7 @@ TEST(NixStringContextElemTest, built_built) {

std::string_view built = "!foo!bar!g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x.drv";
auto elem = NixStringContextElem::parse(built, mockXpSettings);
auto * p = std::get_if<NixStringContextElem::Built>(&elem);
auto * p = std::get_if<NixStringContextElem::Built>(&elem.raw);
ASSERT_TRUE(p);
ASSERT_EQ(p->output, "foo");
auto * drvPath = std::get_if<SingleDerivedPath::Built>(&*p->drvPath);
Expand Down
2 changes: 1 addition & 1 deletion src/libexpr/value/context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ std::string NixStringContextElem::to_string() const
res += '=';
res += d.drvPath.to_string();
},
}, raw());
}, raw);

return res;
}
Expand Down
94 changes: 41 additions & 53 deletions src/libexpr/value/context.hh
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@
#include "util.hh"
#include "comparator.hh"
#include "derived-path.hh"

#include <variant>
#include "variant-wrapper.hh"

#include <nlohmann/json_fwd.hpp>

Expand All @@ -26,58 +25,47 @@ public:
}
};

/**
* Plain opaque path to some store object.
*
* Encoded as just the path: ‘<path>’.
*/
typedef SingleDerivedPath::Opaque NixStringContextElem_Opaque;

/**
* 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 ‘=<drvPath>’.
*/
struct NixStringContextElem_DrvDeep {
StorePath drvPath;

GENERATE_CMP(NixStringContextElem_DrvDeep, me->drvPath);
};
struct NixStringContextElem {
/**
* Plain opaque path to some store object.
*
* Encoded as just the path: ‘<path>’.
*/
using Opaque = SingleDerivedPath::Opaque;

/**
* Derivation output.
*
* Encoded in the form ‘!<output>!<drvPath>’.
*/
typedef SingleDerivedPath::Built NixStringContextElem_Built;

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<const Raw &>(*this);
}
inline Raw & raw() & {
return static_cast<Raw &>(*this);
}
inline Raw && raw() && {
return static_cast<Raw &&>(*this);
}
/**
* 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 ‘=<drvPath>’.
*/
struct DrvDeep {
StorePath drvPath;

GENERATE_CMP(DrvDeep, me->drvPath);
};

/**
* Derivation output.
*
* Encoded in the form ‘!<output>!<drvPath>’.
*/
using Built = SingleDerivedPath::Built;

using Raw = std::variant<
Opaque,
DrvDeep,
Built
>;

Raw raw;

GENERATE_CMP(NixStringContextElem, me->raw);

MAKE_WRAPPER_CONSTRUCTOR(NixStringContextElem);

/**
* Decode a context string, one of:
Expand Down
7 changes: 4 additions & 3 deletions src/libstore/build/derivation-goal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ void DerivationGoal::inputsRealised()
[&](const DerivationType::Impure &) {
return true;
}
}, drvType.raw());
}, drvType.raw);

if (resolveDrv && !fullDrv.inputDrvs.empty()) {
experimentalFeatureSettings.require(Xp::CaDerivations);
Expand Down Expand Up @@ -996,10 +996,11 @@ void DerivationGoal::buildDone()
}

else {
assert(derivationType);
st =
dynamic_cast<NotDeterministic*>(&e) ? BuildResult::NotDeterministic :
statusOk(status) ? BuildResult::OutputRejected :
!derivationType.isSandboxed() || diskFull ? BuildResult::TransientFailure :
!derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure :
BuildResult::PermanentFailure;
}

Expand Down Expand Up @@ -1358,7 +1359,7 @@ std::pair<bool, SingleDrvOutputs> DerivationGoal::checkPathValidity()
[&](const OutputsSpec::Names & names) {
return static_cast<StringSet>(names);
},
}, wantedOutputs.raw());
}, wantedOutputs.raw);
SingleDrvOutputs validOutputs;

for (auto & i : queryPartialDerivationOutputMap()) {
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/build/derivation-goal.hh
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ struct DerivationGoal : public Goal
/**
* The sort of derivation we are building.
*/
DerivationType derivationType;
std::optional<DerivationType> derivationType;

typedef void (DerivationGoal::*GoalState)();
GoalState state;
Expand Down
Loading

0 comments on commit fedcd53

Please sign in to comment.