Skip to content

Commit 8652b6b

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 b062730 commit 8652b6b

File tree

16 files changed

+177
-109
lines changed

16 files changed

+177
-109
lines changed

‎src/libexpr/primops.cc‎

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1363,7 +1363,7 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName
13631363

13641364
/* Check whether attributes should be passed as a JSON file. */
13651365
using nlohmann::json;
1366-
std::optional<json> jsonObject;
1366+
std::optional<StructuredAttrs> jsonObject;
13671367
auto pos = v.determinePos(noPos);
13681368
auto attr = attrs->find(state.sStructuredAttrs);
13691369
if (attr != attrs->end()
@@ -1372,7 +1372,7 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName
13721372
pos,
13731373
"while evaluating the `__structuredAttrs` "
13741374
"attribute passed to builtins.derivationStrict"))
1375-
jsonObject = json::object();
1375+
jsonObject = StructuredAttrs{.structuredAttrs = json::object()};
13761376

13771377
/* Check whether null attributes should be ignored. */
13781378
bool ignoreNulls = false;
@@ -1484,7 +1484,7 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName
14841484
if (i->name == state.sStructuredAttrs)
14851485
continue;
14861486

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

14891489
if (i->name == state.sBuilder)
14901490
drv.builder = state.forceString(*i->value, context, pos, context_below);
@@ -1532,23 +1532,26 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName
15321532

15331533
} else {
15341534
auto s = state.coerceToString(pos, *i->value, context, context_below, true).toOwned();
1535-
drv.env.emplace(key, s);
1536-
if (i->name == state.sBuilder)
1537-
drv.builder = std::move(s);
1538-
else if (i->name == state.sSystem)
1539-
drv.platform = std::move(s);
1540-
else if (i->name == state.sOutputHash)
1541-
outputHash = std::move(s);
1542-
else if (i->name == state.sOutputHashAlgo)
1543-
outputHashAlgo = parseHashAlgoOpt(s);
1544-
else if (i->name == state.sOutputHashMode)
1545-
handleHashMode(s);
1546-
else if (i->name == state.sOutputs)
1547-
handleOutputs(tokenizeString<Strings>(s));
1548-
else if (i->name == state.sJson)
1535+
if (i->name == state.sJson) {
15491536
warn(
15501537
"In derivation '%s': setting structured attributes via '__json' is deprecated, and may be disallowed in future versions of Nix. Set '__structuredAttrs = true' instead.",
15511538
drvName);
1539+
drv.structuredAttrs = StructuredAttrs::parse(s);
1540+
} else {
1541+
drv.env.emplace(key, s);
1542+
if (i->name == state.sBuilder)
1543+
drv.builder = std::move(s);
1544+
else if (i->name == state.sSystem)
1545+
drv.platform = std::move(s);
1546+
else if (i->name == state.sOutputHash)
1547+
outputHash = std::move(s);
1548+
else if (i->name == state.sOutputHashAlgo)
1549+
outputHashAlgo = parseHashAlgoOpt(s);
1550+
else if (i->name == state.sOutputHashMode)
1551+
handleHashMode(s);
1552+
else if (i->name == state.sOutputs)
1553+
handleOutputs(tokenizeString<Strings>(s));
1554+
}
15521555
}
15531556
}
15541557

@@ -1560,8 +1563,10 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName
15601563
}
15611564

15621565
if (jsonObject) {
1563-
drv.env.emplace("__json", jsonObject->dump());
1564-
jsonObject.reset();
1566+
/* The only other way `drv.structuredAttrs` can be set is when
1567+
`jsonObject` is not set. */
1568+
assert(!drv.structuredAttrs);
1569+
drv.structuredAttrs = std::move(*jsonObject);
15651570
}
15661571

15671572
/* 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
@@ -222,7 +222,7 @@ Derivation makeSimpleDrv(const Store & store)
222222
"bar",
223223
"baz",
224224
};
225-
drv.env = {
225+
drv.env = StringPairs{
226226
{
227227
"BIG_BAD",
228228
"WOLF",
@@ -284,7 +284,7 @@ Derivation makeDynDepDerivation(const Store & store)
284284
"bar",
285285
"baz",
286286
};
287-
drv.env = {
287+
drv.env = StringPairs{
288288
{
289289
"BIG_BAD",
290290
"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(
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 =
40-
std::make_unique<DerivationOptions>(DerivationOptions::fromStructuredAttrs(drv->env, parsedDrv.get()));
37+
std::make_unique<DerivationOptions>(DerivationOptions::fromStructuredAttrs(drv->env, drv->structuredAttrs));
4138
} catch (Error & e) {
4239
e.addTrace({}, "while parsing derivation '%s'", worker.store.printStorePath(drvPath));
4340
throw;
@@ -661,7 +658,6 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
661658
buildMode,
662659
buildResult,
663660
*drv,
664-
parsedDrv.get(),
665661
*drvOptions,
666662
inputPaths,
667663
initialOutputs,

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,8 @@ Goal::Co DerivationGoal::haveDerivation()
6363
trace("have derivation");
6464

6565
auto drvOptions = [&]() -> DerivationOptions {
66-
auto parsedOpt = StructuredAttrs::tryParse(drv->env);
6766
try {
68-
return DerivationOptions::fromStructuredAttrs(drv->env, parsedOpt ? &*parsedOpt : nullptr);
67+
return DerivationOptions::fromStructuredAttrs(drv->env, drv->structuredAttrs);
6968
} catch (Error & e) {
7069
e.addTrace({}, "while parsing derivation '%s'", worker.store.printStorePath(drvPath));
7170
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: 52 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -452,6 +452,7 @@ Derivation parseDerivation(
452452
expect(str, ")");
453453
drv.env.insert_or_assign(std::move(name), std::move(value));
454454
}
455+
drv.structuredAttrs = StructuredAttrs::tryExtract(drv.env);
455456

456457
expect(str, ")");
457458
return drv;
@@ -685,16 +686,28 @@ std::string Derivation::unparse(
685686

686687
s += ",[";
687688
first = true;
688-
for (auto & i : env) {
689-
if (first)
690-
first = false;
691-
else
689+
690+
auto unparseEnv = [&](const StringPairs atermEnv) {
691+
for (auto & i : atermEnv) {
692+
if (first)
693+
first = false;
694+
else
695+
s += ',';
696+
s += '(';
697+
printString(s, i.first);
692698
s += ',';
693-
s += '(';
694-
printString(s, i.first);
695-
s += ',';
696-
printString(s, maskOutputs && outputs.count(i.first) ? "" : i.second);
697-
s += ')';
699+
printString(s, maskOutputs && outputs.count(i.first) ? "" : i.second);
700+
s += ')';
701+
}
702+
};
703+
704+
StructuredAttrs::checkKeyNotInUse(env);
705+
if (structuredAttrs) {
706+
StringPairs scratch = env;
707+
scratch.insert(structuredAttrs->unparse());
708+
unparseEnv(scratch);
709+
} else {
710+
unparseEnv(env);
698711
}
699712

700713
s += "])";
@@ -948,6 +961,7 @@ Source & readDerivation(Source & in, const StoreDirConfig & store, BasicDerivati
948961
auto value = readString(in);
949962
drv.env[key] = value;
950963
}
964+
drv.structuredAttrs = StructuredAttrs::tryExtract(drv.env);
951965

952966
return in;
953967
}
@@ -983,9 +997,21 @@ void writeDerivation(Sink & out, const StoreDirConfig & store, const BasicDeriva
983997
}
984998
CommonProto::write(store, CommonProto::WriteConn{.to = out}, drv.inputSrcs);
985999
out << drv.platform << drv.builder << drv.args;
986-
out << drv.env.size();
987-
for (auto & i : drv.env)
988-
out << i.first << i.second;
1000+
1001+
auto writeEnv = [&](const StringPairs atermEnv) {
1002+
out << atermEnv.size();
1003+
for (auto & [k, v] : atermEnv)
1004+
out << k << v;
1005+
};
1006+
1007+
StructuredAttrs::checkKeyNotInUse(drv.env);
1008+
if (drv.structuredAttrs) {
1009+
StringPairs scratch = drv.env;
1010+
scratch.insert(drv.structuredAttrs->unparse());
1011+
writeEnv(scratch);
1012+
} else {
1013+
writeEnv(drv.env);
1014+
}
9891015
}
9901016

9911017
std::string hashPlaceholder(const OutputNameView outputName)
@@ -1017,6 +1043,17 @@ void BasicDerivation::applyRewrites(const StringMap & rewrites)
10171043
newEnv.emplace(envName, envValue);
10181044
}
10191045
env = std::move(newEnv);
1046+
1047+
if (structuredAttrs) {
1048+
// TODO rewrite the JSON AST properly, rather than dump parse round trip.
1049+
auto [k, jsonS] = structuredAttrs->unparse();
1050+
jsonS = rewriteStrings(jsonS, rewrites);
1051+
StringPairs newEnv;
1052+
newEnv.insert(std::pair{k, std::move(jsonS)});
1053+
auto newStructuredAttrs = StructuredAttrs::tryExtract(newEnv);
1054+
assert(newStructuredAttrs);
1055+
structuredAttrs = std::move(*newStructuredAttrs);
1056+
}
10201057
}
10211058

10221059
static void rewriteDerivation(Store & store, BasicDerivation & drv, const StringMap & rewrites)
@@ -1338,10 +1375,8 @@ nlohmann::json Derivation::toJSON(const StoreDirConfig & store) const
13381375
res["args"] = args;
13391376
res["env"] = env;
13401377

1341-
if (auto it = env.find("__json"); it != env.end()) {
1342-
res["env"].erase("__json");
1343-
res["structuredAttrs"] = nlohmann::json::parse(it->second);
1344-
}
1378+
if (structuredAttrs)
1379+
res["structuredAttrs"] = structuredAttrs->structuredAttrs;
13451380

13461381
return res;
13471382
}
@@ -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
}

0 commit comments

Comments
 (0)