Skip to content

Commit d850983

Browse files
Ericson2314Ericson2314
committed
Store StructuredAttrs directly in Derivation
Instead of parsing a structured attrs at some later point, we parsed it right away when parsing the A-Term format, and likewise serialize it to `__json = <JSON dump>` when serializing a derivation to A-Term. The JSON format can directly contain the JSON structured attrs without so encoding it, so we just do that.
1 parent f8c1ac9 commit d850983

16 files changed

+183
-101
lines changed

‎src/libexpr/primops.cc‎

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1266,14 +1266,16 @@ static void derivationStrictInternal(
12661266

12671267
/* Check whether attributes should be passed as a JSON file. */
12681268
using nlohmann::json;
1269-
std::optional<json> jsonObject;
1269+
std::optional<StructuredAttrs> jsonObject;
12701270
auto pos = v.determinePos(noPos);
12711271
auto attr = attrs->find(state.sStructuredAttrs);
12721272
if (attr != attrs->end() &&
12731273
state.forceBool(*attr->value, pos,
12741274
"while evaluating the `__structuredAttrs` "
12751275
"attribute passed to builtins.derivationStrict"))
1276-
jsonObject = json::object();
1276+
jsonObject = StructuredAttrs{
1277+
.structuredAttrs = json::object()
1278+
};
12771279

12781280
/* Check whether null attributes should be ignored. */
12791281
bool ignoreNulls = false;
@@ -1382,7 +1384,7 @@ static void derivationStrictInternal(
13821384

13831385
if (i->name == state.sStructuredAttrs) continue;
13841386

1385-
jsonObject->emplace(key, printValueAsJSON(state, true, *i->value, pos, context));
1387+
jsonObject->structuredAttrs.emplace(key, printValueAsJSON(state, true, *i->value, pos, context));
13861388

13871389
if (i->name == state.sBuilder)
13881390
drv.builder = state.forceString(*i->value, context, pos, context_below);
@@ -1419,16 +1421,19 @@ static void derivationStrictInternal(
14191421

14201422
} else {
14211423
auto s = state.coerceToString(pos, *i->value, context, context_below, true).toOwned();
1422-
drv.env.emplace(key, s);
1423-
if (i->name == state.sBuilder) drv.builder = std::move(s);
1424-
else if (i->name == state.sSystem) drv.platform = std::move(s);
1425-
else if (i->name == state.sOutputHash) outputHash = std::move(s);
1426-
else if (i->name == state.sOutputHashAlgo) outputHashAlgo = parseHashAlgoOpt(s);
1427-
else if (i->name == state.sOutputHashMode) handleHashMode(s);
1428-
else if (i->name == state.sOutputs)
1429-
handleOutputs(tokenizeString<Strings>(s));
1430-
else if (i->name == state.sJson)
1424+
if (i->name == state.sJson) {
14311425
warn("In derivation '%s': setting structured attributes via '__json' is deprecated, and may be disallowed in future versions of Nix. Set '__structuredAttrs = true' instead.", drvName);
1426+
drv.structuredAttrs = StructuredAttrs::parse(s);
1427+
} else {
1428+
drv.env.emplace(key, s);
1429+
if (i->name == state.sBuilder) drv.builder = std::move(s);
1430+
else if (i->name == state.sSystem) drv.platform = std::move(s);
1431+
else if (i->name == state.sOutputHash) outputHash = std::move(s);
1432+
else if (i->name == state.sOutputHashAlgo) outputHashAlgo = parseHashAlgoOpt(s);
1433+
else if (i->name == state.sOutputHashMode) handleHashMode(s);
1434+
else if (i->name == state.sOutputs)
1435+
handleOutputs(tokenizeString<Strings>(s));
1436+
}
14321437
}
14331438

14341439
}
@@ -1441,8 +1446,10 @@ static void derivationStrictInternal(
14411446
}
14421447

14431448
if (jsonObject) {
1444-
drv.env.emplace("__json", jsonObject->dump());
1445-
jsonObject.reset();
1449+
/* The only other way `drv.structuredAttrs` can be set is when
1450+
`jsonObject` is not set. */
1451+
assert(!drv.structuredAttrs);
1452+
drv.structuredAttrs = std::move(*jsonObject);
14461453
}
14471454

14481455
/* Everything in the context of the strings in the derivation

‎src/libstore-tests/derivation-advanced-attrs.cc‎

Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -108,10 +108,9 @@ TYPED_TEST(DerivationAdvancedAttrsBothTest, advancedAttributes_defaults)
108108

109109
auto drvPath = writeDerivation(*this->store, got, NoRepair, true);
110110

111-
auto parsedDrv = StructuredAttrs::tryParse(got.env);
112-
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, parsedDrv ? &*parsedDrv : nullptr);
111+
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, got.structuredAttrs);
113112

114-
EXPECT_TRUE(!parsedDrv);
113+
EXPECT_TRUE(!got.structuredAttrs);
115114

116115
EXPECT_EQ(options.additionalSandboxProfile, "");
117116
EXPECT_EQ(options.noChroot, false);
@@ -143,8 +142,7 @@ TEST_F(DerivationAdvancedAttrsTest, advancedAttributes_defaults)
143142

144143
auto drvPath = writeDerivation(*this->store, got, NoRepair, true);
145144

146-
auto parsedDrv = StructuredAttrs::tryParse(got.env);
147-
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, parsedDrv ? &*parsedDrv : nullptr);
145+
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, got.structuredAttrs);
148146

149147
EXPECT_EQ(options.getRequiredSystemFeatures(got), StringSet{});
150148
});
@@ -157,8 +155,7 @@ TEST_F(CaDerivationAdvancedAttrsTest, advancedAttributes_defaults)
157155

158156
auto drvPath = writeDerivation(*this->store, got, NoRepair, true);
159157

160-
auto parsedDrv = StructuredAttrs::tryParse(got.env);
161-
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, parsedDrv ? &*parsedDrv : nullptr);
158+
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, got.structuredAttrs);
162159

163160
EXPECT_EQ(options.getRequiredSystemFeatures(got), StringSet{"ca-derivations"});
164161
});
@@ -171,10 +168,9 @@ TYPED_TEST(DerivationAdvancedAttrsBothTest, advancedAttributes)
171168

172169
auto drvPath = writeDerivation(*this->store, got, NoRepair, true);
173170

174-
auto parsedDrv = StructuredAttrs::tryParse(got.env);
175-
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, parsedDrv ? &*parsedDrv : nullptr);
171+
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, got.structuredAttrs);
176172

177-
EXPECT_TRUE(!parsedDrv);
173+
EXPECT_TRUE(!got.structuredAttrs);
178174

179175
EXPECT_EQ(options.additionalSandboxProfile, "sandcastle");
180176
EXPECT_EQ(options.noChroot, true);
@@ -195,8 +191,7 @@ TEST_F(DerivationAdvancedAttrsTest, advancedAttributes)
195191

196192
auto drvPath = writeDerivation(*this->store, got, NoRepair, true);
197193

198-
auto parsedDrv = StructuredAttrs::tryParse(got.env);
199-
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, parsedDrv ? &*parsedDrv : nullptr);
194+
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, got.structuredAttrs);
200195

201196
EXPECT_EQ(
202197
options.exportReferencesGraph,
@@ -245,8 +240,7 @@ TEST_F(CaDerivationAdvancedAttrsTest, advancedAttributes)
245240

246241
auto drvPath = writeDerivation(*this->store, got, NoRepair, true);
247242

248-
auto parsedDrv = StructuredAttrs::tryParse(got.env);
249-
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, parsedDrv ? &*parsedDrv : nullptr);
243+
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, got.structuredAttrs);
250244

251245
EXPECT_EQ(
252246
options.exportReferencesGraph,
@@ -298,10 +292,9 @@ TYPED_TEST(DerivationAdvancedAttrsBothTest, advancedAttributes_structuredAttrs_d
298292

299293
auto drvPath = writeDerivation(*this->store, got, NoRepair, true);
300294

301-
auto parsedDrv = StructuredAttrs::tryParse(got.env);
302-
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, parsedDrv ? &*parsedDrv : nullptr);
295+
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, got.structuredAttrs);
303296

304-
EXPECT_TRUE(parsedDrv);
297+
EXPECT_TRUE(got.structuredAttrs);
305298

306299
EXPECT_EQ(options.additionalSandboxProfile, "");
307300
EXPECT_EQ(options.noChroot, false);
@@ -332,8 +325,7 @@ TEST_F(DerivationAdvancedAttrsTest, advancedAttributes_structuredAttrs_defaults)
332325

333326
auto drvPath = writeDerivation(*this->store, got, NoRepair, true);
334327

335-
auto parsedDrv = StructuredAttrs::tryParse(got.env);
336-
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, parsedDrv ? &*parsedDrv : nullptr);
328+
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, got.structuredAttrs);
337329

338330
EXPECT_EQ(options.getRequiredSystemFeatures(got), StringSet{});
339331
});
@@ -346,8 +338,7 @@ TEST_F(CaDerivationAdvancedAttrsTest, advancedAttributes_structuredAttrs_default
346338

347339
auto drvPath = writeDerivation(*this->store, got, NoRepair, true);
348340

349-
auto parsedDrv = StructuredAttrs::tryParse(got.env);
350-
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, parsedDrv ? &*parsedDrv : nullptr);
341+
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, got.structuredAttrs);
351342

352343
EXPECT_EQ(options.getRequiredSystemFeatures(got), StringSet{"ca-derivations"});
353344
});
@@ -360,10 +351,9 @@ TYPED_TEST(DerivationAdvancedAttrsBothTest, advancedAttributes_structuredAttrs)
360351

361352
auto drvPath = writeDerivation(*this->store, got, NoRepair, true);
362353

363-
auto parsedDrv = StructuredAttrs::tryParse(got.env);
364-
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, parsedDrv ? &*parsedDrv : nullptr);
354+
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, got.structuredAttrs);
365355

366-
EXPECT_TRUE(parsedDrv);
356+
EXPECT_TRUE(got.structuredAttrs);
367357

368358
EXPECT_EQ(options.additionalSandboxProfile, "sandcastle");
369359
EXPECT_EQ(options.noChroot, true);
@@ -394,8 +384,7 @@ TEST_F(DerivationAdvancedAttrsTest, advancedAttributes_structuredAttrs)
394384

395385
auto drvPath = writeDerivation(*this->store, got, NoRepair, true);
396386

397-
auto parsedDrv = StructuredAttrs::tryParse(got.env);
398-
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, parsedDrv ? &*parsedDrv : nullptr);
387+
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, got.structuredAttrs);
399388

400389
EXPECT_EQ(
401390
options.exportReferencesGraph,
@@ -448,8 +437,7 @@ TEST_F(CaDerivationAdvancedAttrsTest, advancedAttributes_structuredAttrs)
448437

449438
auto drvPath = writeDerivation(*this->store, got, NoRepair, true);
450439

451-
auto parsedDrv = StructuredAttrs::tryParse(got.env);
452-
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, parsedDrv ? &*parsedDrv : nullptr);
440+
DerivationOptions options = DerivationOptions::fromStructuredAttrs(got.env, got.structuredAttrs);
453441

454442
EXPECT_EQ(
455443
options.exportReferencesGraph,

‎src/libstore-tests/derivation.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ Derivation makeSimpleDrv(const Store & store) {
220220
"bar",
221221
"baz",
222222
};
223-
drv.env = {
223+
drv.env = StringPairs{
224224
{
225225
"BIG_BAD",
226226
"WOLF",
@@ -278,7 +278,7 @@ Derivation makeDynDepDerivation(const Store & store) {
278278
"bar",
279279
"baz",
280280
};
281-
drv.env = {
281+
drv.env = StringPairs{
282282
{
283283
"BIG_BAD",
284284
"WOLF",

‎src/libstore/build/derivation-building-goal.cc‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,9 @@ DerivationBuildingGoal::DerivationBuildingGoal(const StorePath & drvPath, const
3232
{
3333
drv = std::make_unique<Derivation>(drv_);
3434

35-
if (auto parsedOpt = StructuredAttrs::tryParse(drv->env)) {
36-
parsedDrv = std::make_unique<StructuredAttrs>(*parsedOpt);
37-
}
3835
try {
3936
drvOptions = std::make_unique<DerivationOptions>(
40-
DerivationOptions::fromStructuredAttrs(drv->env, parsedDrv.get()));
37+
DerivationOptions::fromStructuredAttrs(drv->env, drv->structuredAttrs));
4138
} catch (Error & e) {
4239
e.addTrace({}, "while parsing derivation '%s'", worker.store.printStorePath(drvPath));
4340
throw;
@@ -628,7 +625,6 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
628625
buildMode,
629626
buildResult,
630627
*drv,
631-
parsedDrv.get(),
632628
*drvOptions,
633629
inputPaths,
634630
initialOutputs,

‎src/libstore/build/derivation-goal.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,9 +173,8 @@ Goal::Co DerivationGoal::haveDerivation(StorePath drvPath)
173173
trace("have derivation");
174174

175175
auto drvOptions = [&]() -> DerivationOptions {
176-
auto parsedOpt = StructuredAttrs::tryParse(drv->env);
177176
try {
178-
return DerivationOptions::fromStructuredAttrs(drv->env, parsedOpt ? &*parsedOpt : nullptr);
177+
return DerivationOptions::fromStructuredAttrs(drv->env, drv->structuredAttrs);
179178
} catch (Error & e) {
180179
e.addTrace({}, "while parsing derivation '%s'", worker.store.printStorePath(drvPath));
181180
throw;

‎src/libstore/derivation-options.cc‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,12 @@ using OutputChecks = DerivationOptions::OutputChecks;
9292

9393
using OutputChecksVariant = std::variant<OutputChecks, std::map<std::string, OutputChecks>>;
9494

95+
DerivationOptions DerivationOptions::fromStructuredAttrs(
96+
const StringMap & env, const std::optional<StructuredAttrs> & parsed, bool shouldWarn)
97+
{
98+
return fromStructuredAttrs(env, parsed ? &*parsed : nullptr);
99+
}
100+
95101
DerivationOptions
96102
DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAttrs * parsed, bool shouldWarn)
97103
{

‎src/libstore/derivations.cc‎

Lines changed: 48 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,7 @@ Derivation parseDerivation(
460460
expect(str, ")");
461461
drv.env.insert_or_assign(std::move(name), std::move(value));
462462
}
463+
drv.structuredAttrs = StructuredAttrs::tryExtract(drv.env);
463464

464465
expect(str, ")");
465466
return drv;
@@ -650,11 +651,23 @@ std::string Derivation::unparse(const StoreDirConfig & store, bool maskOutputs,
650651

651652
s += ",[";
652653
first = true;
653-
for (auto & i : env) {
654-
if (first) first = false; else s += ',';
655-
s += '('; printString(s, i.first);
656-
s += ','; printString(s, maskOutputs && outputs.count(i.first) ? "" : i.second);
657-
s += ')';
654+
655+
auto unparseEnv = [&](const StringPairs atermEnv) {
656+
for (auto & i : atermEnv) {
657+
if (first) first = false; else s += ',';
658+
s += '('; printString(s, i.first);
659+
s += ','; printString(s, maskOutputs && outputs.count(i.first) ? "" : i.second);
660+
s += ')';
661+
}
662+
};
663+
664+
StructuredAttrs::checkKeyNotInUse(env);
665+
if (structuredAttrs) {
666+
StringPairs scratch = env;
667+
scratch.insert(structuredAttrs->unparse());
668+
unparseEnv(scratch);
669+
} else {
670+
unparseEnv(env);
658671
}
659672

660673
s += "])";
@@ -953,6 +966,7 @@ Source & readDerivation(Source & in, const StoreDirConfig & store, BasicDerivati
953966
auto value = readString(in);
954967
drv.env[key] = value;
955968
}
969+
drv.structuredAttrs = StructuredAttrs::tryExtract(drv.env);
956970

957971
return in;
958972
}
@@ -995,9 +1009,21 @@ void writeDerivation(Sink & out, const StoreDirConfig & store, const BasicDeriva
9951009
CommonProto::WriteConn { .to = out },
9961010
drv.inputSrcs);
9971011
out << drv.platform << drv.builder << drv.args;
998-
out << drv.env.size();
999-
for (auto & i : drv.env)
1000-
out << i.first << i.second;
1012+
1013+
auto writeEnv = [&](const StringPairs atermEnv) {
1014+
out << atermEnv.size();
1015+
for (auto & [k, v] : atermEnv)
1016+
out << k << v;
1017+
};
1018+
1019+
StructuredAttrs::checkKeyNotInUse(drv.env);
1020+
if (drv.structuredAttrs) {
1021+
StringPairs scratch = drv.env;
1022+
scratch.insert(drv.structuredAttrs->unparse());
1023+
writeEnv(scratch);
1024+
} else {
1025+
writeEnv(drv.env);
1026+
}
10011027
}
10021028

10031029

@@ -1027,6 +1053,17 @@ void BasicDerivation::applyRewrites(const StringMap & rewrites)
10271053
newEnv.emplace(envName, envValue);
10281054
}
10291055
env = std::move(newEnv);
1056+
1057+
if (structuredAttrs) {
1058+
// TODO rewrite the JSON AST properly, rather than dump parse round trip.
1059+
auto [k, jsonS] = structuredAttrs->unparse();
1060+
jsonS = rewriteStrings(jsonS, rewrites);
1061+
StringPairs newEnv;
1062+
newEnv.insert(std::pair{k, std::move(jsonS)});
1063+
auto newStructuredAttrs = StructuredAttrs::tryParse(newEnv);
1064+
assert(newStructuredAttrs);
1065+
structuredAttrs = std::move(*newStructuredAttrs);
1066+
}
10301067
}
10311068

10321069
static void rewriteDerivation(Store & store, BasicDerivation & drv, const StringMap & rewrites)
@@ -1333,10 +1370,8 @@ nlohmann::json Derivation::toJSON(const StoreDirConfig & store) const
13331370
res["args"] = args;
13341371
res["env"] = env;
13351372

1336-
if (auto it = env.find("__json"); it != env.end()) {
1337-
res["env"].erase("__json");
1338-
res["structuredAttrs"] = nlohmann::json::parse(it->second);
1339-
}
1373+
if (structuredAttrs)
1374+
res["structuredAttrs"] = structuredAttrs->structuredAttrs;
13401375

13411376
return res;
13421377
}
@@ -1411,7 +1446,7 @@ Derivation Derivation::fromJSON(
14111446
}
14121447

14131448
if (auto structuredAttrs = get(json, "structuredAttrs"))
1414-
res.env.insert_or_assign("__json", structuredAttrs->dump());
1449+
res.structuredAttrs = StructuredAttrs{*structuredAttrs};
14151450

14161451
return res;
14171452
}

‎src/libstore/include/nix/store/build/derivation-building-goal.hh‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
#pragma once
22
///@file
33

4-
#include "nix/store/parsed-derivations.hh"
54
#include "nix/store/derivations.hh"
5+
#include "nix/store/parsed-derivations.hh"
66
#include "nix/store/derivation-options.hh"
77
#include "nix/store/build/derivation-building-misc.hh"
88
#include "nix/store/outputs-spec.hh"
@@ -41,7 +41,6 @@ struct DerivationBuildingGoal : public Goal
4141
*/
4242
std::unique_ptr<Derivation> drv;
4343

44-
std::unique_ptr<StructuredAttrs> parsedDrv;
4544
std::unique_ptr<DerivationOptions> drvOptions;
4645

4746
/**

0 commit comments

Comments
 (0)