Skip to content

Commit

Permalink
Expose some implementation of builtins.derivationStrict for unit te…
Browse files Browse the repository at this point in the history 
…sting

This allows us to avoid needing to generalize `dummy://` into a
`transient://`  at this time.

Setting the global settings in unit tests like this is very ugly, but
the real problem is not changing the settings but having the code being
unit tested depend on global variables at all. NixOS#5638 tracks fixing this.
  • Loading branch information
@Ericson2314
Ericson2314 committed Jun 23, 2024
1 parent 129ed19 commit 826f891
Show file tree
Hide file tree
Showing 9 changed files with 127 additions and 61 deletions.
89 changes: 55 additions & 34 deletions src/libexpr/primops.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1104,10 +1104,9 @@ static void prim_second(EvalState & state, const PosIdx pos, Value * * args, Val
* Derivations
*************************************************************/

static void derivationStrictInternal(
static void derivationStrictInternalBuildResult(
EvalState & state,
const std::string & name,
const Bindings * attrs,
const Derivation & drv,
Value & v);

/* Construct (as a unobservable side effect) a Nix derivation
Expand All @@ -1119,25 +1118,14 @@ static void derivationStrictInternal(
derivation. */
static void prim_derivationStrict(EvalState & state, const PosIdx pos, Value * * args, Value & v)
{
state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.derivationStrict");

auto attrs = args[0]->attrs();

/* Figure out the name first (for stack backtraces). */
auto nameAttr = getAttr(state, state.sName, attrs, "in the attrset passed as argument to builtins.derivationStrict");
auto [attrs, drvName, namePos] = derivationStrictInternalGetNameAndAttrs(state, pos, *args[0]);

std::string drvName;
try {
drvName = state.forceStringNoCtx(*nameAttr->value, pos, "while evaluating the `name` attribute passed to builtins.derivationStrict");
auto resPos = v.determinePos(noPos);
auto drv = derivationStrictInternalReturning(state, drvName, attrs, resPos);
derivationStrictInternalBuildResult(state, drv, v);
} catch (Error & e) {
e.addTrace(state.positions[nameAttr->pos], "while evaluating the derivation attribute 'name'");
throw;
}

try {
derivationStrictInternal(state, drvName, attrs, v);
} catch (Error & e) {
Pos pos = state.positions[nameAttr->pos];
Pos pos = state.positions[namePos];
/*
* Here we make two abuses of the error system
*
Expand All @@ -1163,16 +1151,41 @@ static void prim_derivationStrict(EvalState & state, const PosIdx pos, Value * *
}
}

static void derivationStrictInternal(
std::tuple<const Bindings *, std::string, const PosIdx>
derivationStrictInternalGetNameAndAttrs(
EvalState & state,
const PosIdx pos,
Value & arg)
{
state.forceAttrs(arg, pos, "while evaluating the argument passed to builtins.derivationStrict");

auto attrs = arg.attrs();

/* Figure out the name first (for stack backtraces). */
auto nameAttr = getAttr(state, state.sName, attrs, "in the attrset passed as argument to builtins.derivationStrict");

std::string drvName;
try {
return std::tuple<const Bindings *, std::string, const PosIdx>{
attrs,
state.forceStringNoCtx(*nameAttr->value, pos, "while evaluating the `name` attribute passed to builtins.derivationStrict"),
nameAttr->pos,
};
} catch (Error & e) {
e.addTrace(state.positions[nameAttr->pos], "while evaluating the derivation attribute 'name'");
throw;
}
}

Derivation derivationStrictInternalReturning(
EvalState & state,
const std::string & drvName,
std::string_view drvName,
const Bindings * attrs,
Value & v)
const PosIdx pos)
{
/* Check whether attributes should be passed as a JSON file. */
using nlohmann::json;
std::optional<json> jsonObject;
auto pos = v.determinePos(noPos);
auto attr = attrs->find(state.sStructuredAttrs);
if (attr != attrs->end() &&
state.forceBool(*attr->value, pos,
Expand Down Expand Up @@ -1215,7 +1228,7 @@ static void derivationStrictInternal(
} catch (UsageError &) {
state.error<EvalError>(
"invalid value '%s' for 'outputHashMode' attribute", s
).atPos(v).debugThrow();
).atPos(pos).debugThrow();
}
if (ingestionMethod == TextIngestionMethod {})
experimentalFeatureSettings.require(Xp::DynamicDerivations);
Expand All @@ -1228,7 +1241,7 @@ static void derivationStrictInternal(
for (auto & j : ss) {
if (outputs.find(j) != outputs.end())
state.error<EvalError>("duplicate derivation output '%1%'", j)
.atPos(v)
.atPos(pos)
.debugThrow();
/* !!! Check whether j is a valid attribute
name. */
Expand All @@ -1237,13 +1250,13 @@ static void derivationStrictInternal(
the resulting set (see state.sDrvPath). */
if (j == "drvPath")
state.error<EvalError>("invalid derivation output name 'drvPath'")
.atPos(v)
.atPos(pos)
.debugThrow();
outputs.insert(j);
}
if (outputs.empty())
state.error<EvalError>("derivation cannot have an empty set of outputs")
.atPos(v)
.atPos(pos)
.debugThrow();
};

Expand Down Expand Up @@ -1367,12 +1380,12 @@ static void derivationStrictInternal(
/* Do we have all required attributes? */
if (drv.builder == "")
state.error<EvalError>("required attribute 'builder' missing")
.atPos(v)
.atPos(pos)
.debugThrow();

if (drv.platform == "")
state.error<EvalError>("required attribute 'system' missing")
.atPos(v)
.atPos(pos)
.debugThrow();

/* Check whether the derivation name is valid. */
Expand All @@ -1384,7 +1397,7 @@ static void derivationStrictInternal(
state.error<EvalError>(
"derivation names are allowed to end in '%s' only if they produce a single derivation file",
drvExtension
).atPos(v).debugThrow();
).atPos(pos).debugThrow();
}

if (outputHash) {
Expand All @@ -1395,7 +1408,7 @@ static void derivationStrictInternal(
if (outputs.size() != 1 || *(outputs.begin()) != "out")
state.error<EvalError>(
"multiple outputs are not supported in fixed-output derivations"
).atPos(v).debugThrow();
).atPos(pos).debugThrow();

auto h = newHashAllowEmpty(*outputHash, outputHashAlgo);

Expand All @@ -1415,7 +1428,7 @@ static void derivationStrictInternal(
else if (contentAddressed || isImpure) {
if (contentAddressed && isImpure)
state.error<EvalError>("derivation cannot be both content-addressed and impure")
.atPos(v).debugThrow();
.atPos(pos).debugThrow();

auto ha = outputHashAlgo.value_or(HashAlgorithm::SHA256);
auto method = ingestionMethod.value_or(FileIngestionMethod::Recursive);
Expand Down Expand Up @@ -1459,7 +1472,7 @@ static void derivationStrictInternal(
state.error<AssertionError>(
"derivation produced no hash for output '%s'",
i
).atPos(v).debugThrow();
).atPos(pos).debugThrow();
auto outPath = state.store->makeOutputPath(i, *h, drvName);
drv.env[i] = state.store->printStorePath(outPath);
drv.outputs.insert_or_assign(
Expand All @@ -1477,11 +1490,19 @@ static void derivationStrictInternal(
}
}

return drv;
}

static void derivationStrictInternalBuildResult(
EvalState & state,
const Derivation & drv,
Value & v)
{
/* Write the resulting term into the Nix store directory. */
auto drvPath = writeDerivation(*state.store, drv, state.repair);
auto drvPathS = state.store->printStorePath(drvPath);

printMsg(lvlChatty, "instantiated '%1%' -> '%2%'", drvName, drvPathS);
printMsg(lvlChatty, "instantiated '%1%' -> '%2%'", drv.name, drvPathS);

/* Optimisation, but required in read-only mode! because in that
case we don't actually write store derivations, so we can't
Expand Down
24 changes: 24 additions & 0 deletions src/libexpr/primops.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,28 @@ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v);

void makePositionThunks(EvalState & state, const PosIdx pos, Value & line, Value & column);

struct Derivation;

/**
* Get the `name` field and attribute set of `builtins.derivationStrict`
* arguments
*
* Exposed for unit testing purposes.
*/
std::tuple<const Bindings *, std::string, const PosIdx>
derivationStrictInternalGetNameAndAttrs(
EvalState & state,
const PosIdx pos,
Value & arg);

/**
* @returns The derivation produced by `builtins.derivationStrict`
*
* Exposed for unit testing purposes.
*/
Derivation derivationStrictInternalReturning(
EvalState & state,
std::string_view name,
const Bindings * attrs,
const PosIdx pos);
}
1 change: 1 addition & 0 deletions tests/unit/libexpr-support/tests/libexpr.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ namespace nix {
static void SetUpTestSuite() {
LibStoreTest::SetUpTestSuite();
initGC();
settings.readOnlyMode = true;
evalSettings.nixPath = {};
}

Expand Down
2 changes: 1 addition & 1 deletion tests/unit/libexpr/data/derivation/advanced-attributes-defaults.nix
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
derivation {
{
name = "advanced-attributes-defaults";
system = "my-system";
builder = "/bin/bash";
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/libexpr/data/derivation/advanced-attributes-structured-attrs-defaults.nix
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
derivation {
{
name = "advanced-attributes-structured-attrs-defaults";
system = "my-system";
builder = "/bin/bash";
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/libexpr/data/derivation/advanced-attributes-structured-attrs.nix
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ let
args = ["-c" "echo bar > $out"];
};
in
derivation {
{
inherit system;
name = "advanced-attributes-structured-attrs";
builder = "/bin/bash";
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/libexpr/data/derivation/advanced-attributes.nix
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ let
args = ["-c" "echo bar > $out"];
};
in
derivation {
{
inherit system;
name = "advanced-attributes";
builder = "/bin/bash";
Expand Down
42 changes: 19 additions & 23 deletions tests/unit/libexpr/derivation.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "tests/characterization.hh"
#include "tests/libexpr.hh"
#include "derivations.hh"
#include "primops.hh"

namespace nix {

Expand All @@ -17,29 +18,24 @@ class DerivationTest : public CharacterizationTest, public LibExprTest
}
};

#define EXPR_DRV_TEST(NAME, STEM) \
TEST_F(DerivationTest, Derivation_##NAME) \
{ \
writeTest( \
STEM ".drv", \
[&]() -> Derivation { \
Value v = eval(readFile(goldenMaster(STEM ".nix"))); \
Symbol s = state.symbols.create("drvPath"); \
auto attr = v.attrs() -> get(s); \
state.forceValueDeep(*attr->value); \
NixStringContext context; \
auto storePath = state.coerceToStorePath(attr->pos, *attr->value, context, ""); \
\
return store->readDerivation(storePath); \
}, \
[&](const auto & file) { \
auto s = readFile(file); \
return parseDerivation(*store, std::move(s), STEM); \
}, \
[&](const auto & file, const auto & got) { \
auto s = got.unparse(*store, false); \
return writeFile(file, std::move(s)); \
}); \
#define EXPR_DRV_TEST(NAME, STEM) \
TEST_F(DerivationTest, Derivation_##NAME) \
{ \
writeTest( \
STEM ".drv", \
[&]() -> Derivation { \
Value v = eval(readFile(goldenMaster(STEM ".nix"))); \
auto [attrs, drvName, _namePos] = derivationStrictInternalGetNameAndAttrs(state, noPos, v); \
return derivationStrictInternalReturning(state, drvName, attrs, noPos); \
}, \
[&](const auto & file) { \
auto s = readFile(file); \
return parseDerivation(*store, std::move(s), STEM); \
}, \
[&](const auto & file, const auto & got) { \
auto s = got.unparse(*store, false); \
return writeFile(file, std::move(s)); \
}); \
}

EXPR_DRV_TEST(advancedAttributes, "advanced-attributes");
Expand Down
24 changes: 24 additions & 0 deletions tests/unit/libexpr/nix_api_expr.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ TEST_F(nix_api_expr_test, nix_expr_eval_drv)

TEST_F(nix_api_expr_test, nix_build_drv)
{
// FIXME this is ugly and will prevent running the tests in
// parallel. #5638 tracks
auto old = nix::settings.readOnlyMode;
nix::settings.readOnlyMode = false;

auto expr = R"(derivation { name = "myname";
system = builtins.currentSystem;
builder = "/bin/sh";
Expand Down Expand Up @@ -103,6 +108,9 @@ TEST_F(nix_api_expr_test, nix_build_drv)
// Clean up
nix_store_path_free(drvStorePath);
nix_store_path_free(outStorePath);

// FIXME remove. see above
nix::settings.readOnlyMode = old;
}

TEST_F(nix_api_expr_test, nix_expr_realise_context_bad_value)
Expand All @@ -118,6 +126,11 @@ TEST_F(nix_api_expr_test, nix_expr_realise_context_bad_value)

TEST_F(nix_api_expr_test, nix_expr_realise_context_bad_build)
{
// FIXME this is ugly and will prevent running the tests in
// parallel. #5638 tracks
auto old = nix::settings.readOnlyMode;
nix::settings.readOnlyMode = false;

auto expr = R"(
derivation { name = "letsbuild";
system = builtins.currentSystem;
Expand All @@ -131,10 +144,18 @@ TEST_F(nix_api_expr_test, nix_expr_realise_context_bad_build)
ASSERT_EQ(nullptr, r);
ASSERT_EQ(ctx->last_err_code, NIX_ERR_NIX_ERROR);
ASSERT_THAT(ctx->last_err, testing::Optional(testing::HasSubstr("failed with exit code 1")));

// FIXME remove. see above
nix::settings.readOnlyMode = old;
}

TEST_F(nix_api_expr_test, nix_expr_realise_context)
{
// FIXME this is ugly and will prevent running the tests in
// parallel. #5638 tracks
auto old = nix::settings.readOnlyMode;
nix::settings.readOnlyMode = false;

// TODO (ca-derivations): add a content-addressed derivation output, which produces a placeholder
auto expr = R"(
''
Expand Down Expand Up @@ -189,6 +210,9 @@ TEST_F(nix_api_expr_test, nix_expr_realise_context)
EXPECT_THAT(names[2], testing::StrEq("not-actually-built-yet.drv"));

nix_realised_string_free(r);

// FIXME remove. see above
nix::settings.readOnlyMode = old;
}

const char * SAMPLE_USER_DATA = "whatever";
Expand Down

0 comments on commit 826f891

Please sign in to comment.