Scrap ParsedDerivation for parts

Only a much smaller `StructuredAttrs` remains, the rest is is now moved
to `DerivationOptions`.

This gets us quite close to `std::optional<StructuredAttrs>` and
`DerivationOptions` being included in `Derivation` as fields.

Author: Ericson2314

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

@@ -81,10 +81,11 @@ TEST_F(DerivationAdvancedAttrsTest, Derivation_advancedAttributes_defaults)
 
         auto drvPath = writeDerivation(*store, got, NoRepair, true);
 
-        ParsedDerivation parsedDrv(got.env);
-        DerivationOptions options = DerivationOptions::fromParsedDerivation(*store, parsedDrv);
+        auto parsedDrv = StructuredAttrs::tryParse(got.env);
+        DerivationOptions options =
+            DerivationOptions::fromStructuredAttrs(*store, got.env, parsedDrv ? &*parsedDrv : nullptr);
 
-        EXPECT_TRUE(!parsedDrv.hasStructuredAttrs());
+        EXPECT_TRUE(!parsedDrv);
 
         EXPECT_EQ(options.additionalSandboxProfile, "");
         EXPECT_EQ(options.noChroot, false);
@@ -116,12 +117,13 @@ TEST_F(DerivationAdvancedAttrsTest, Derivation_advancedAttributes)
 
         auto drvPath = writeDerivation(*store, got, NoRepair, true);
 
-        ParsedDerivation parsedDrv(got.env);
-        DerivationOptions options = DerivationOptions::fromParsedDerivation(*store, parsedDrv);
+        auto parsedDrv = StructuredAttrs::tryParse(got.env);
+        DerivationOptions options =
+            DerivationOptions::fromStructuredAttrs(*store, got.env, parsedDrv ? &*parsedDrv : nullptr);
 
         StringSet systemFeatures{"rainbow", "uid-range"};
 
-        EXPECT_TRUE(!parsedDrv.hasStructuredAttrs());
+        EXPECT_TRUE(!parsedDrv);
 
         EXPECT_EQ(options.additionalSandboxProfile, "sandcastle");
         EXPECT_EQ(options.noChroot, true);
@@ -157,10 +159,11 @@ TEST_F(DerivationAdvancedAttrsTest, Derivation_advancedAttributes_structuredAttr
 
         auto drvPath = writeDerivation(*store, got, NoRepair, true);
 
-        ParsedDerivation parsedDrv(got.env);
-        DerivationOptions options = DerivationOptions::fromParsedDerivation(*store, parsedDrv);
+        auto parsedDrv = StructuredAttrs::tryParse(got.env);
+        DerivationOptions options =
+            DerivationOptions::fromStructuredAttrs(*store, got.env, parsedDrv ? &*parsedDrv : nullptr);
 
-        EXPECT_TRUE(parsedDrv.hasStructuredAttrs());
+        EXPECT_TRUE(parsedDrv);
 
         EXPECT_EQ(options.additionalSandboxProfile, "");
         EXPECT_EQ(options.noChroot, false);
@@ -191,12 +194,13 @@ TEST_F(DerivationAdvancedAttrsTest, Derivation_advancedAttributes_structuredAttr
 
         auto drvPath = writeDerivation(*store, got, NoRepair, true);
 
-        ParsedDerivation parsedDrv(got.env);
-        DerivationOptions options = DerivationOptions::fromParsedDerivation(*store, parsedDrv);
+        auto parsedDrv = StructuredAttrs::tryParse(got.env);
+        DerivationOptions options =
+            DerivationOptions::fromStructuredAttrs(*store, got.env, parsedDrv ? &*parsedDrv : nullptr);
 
         StringSet systemFeatures{"rainbow", "uid-range"};
 
-        EXPECT_TRUE(parsedDrv.hasStructuredAttrs());
+        EXPECT_TRUE(parsedDrv);
 
         EXPECT_EQ(options.additionalSandboxProfile, "sandcastle");
         EXPECT_EQ(options.noChroot, true);

src/libstore/build/derivation-goal.cc

@@ -180,10 +180,12 @@ Goal::Co DerivationGoal::haveDerivation()
 {
     trace("have derivation");
 
-    parsedDrv = std::make_unique<ParsedDerivation>(drv->env);
+    if (auto parsedOpt = StructuredAttrs::tryParse(drv->env)) {
+        parsedDrv = std::make_unique<StructuredAttrs>(*parsedOpt);
+    }
     try {
         drvOptions = std::make_unique<DerivationOptions>(
-            DerivationOptions::fromParsedDerivation(worker.store, *parsedDrv));
+            DerivationOptions::fromStructuredAttrs(worker.store, drv->env, parsedDrv.get()));
     } catch (Error & e) {
         e.addTrace({}, "while parsing derivation '%s'", worker.store.printStorePath(drvPath));
         throw;

src/libstore/build/derivation-goal.hh

@@ -2,6 +2,7 @@
 ///@file
 
 #include "parsed-derivations.hh"
+#include "derivations.hh"
 #include "derivation-options.hh"
 #ifndef _WIN32
 #  include "user-lock.hh"
@@ -150,7 +151,7 @@ struct DerivationGoal : public Goal
      */
     std::unique_ptr<Derivation> drv;
 
-    std::unique_ptr<ParsedDerivation> parsedDrv;
+    std::unique_ptr<StructuredAttrs> parsedDrv;
     std::unique_ptr<DerivationOptions> drvOptions;
 
     /**

src/libstore/derivation-options.cc

@@ -1,6 +1,8 @@
 #include "derivation-options.hh"
 #include "json-utils.hh"
 #include "parsed-derivations.hh"
+#include "derivations.hh"
+#include "store-api.hh"
 #include "types.hh"
 #include "util.hh"
 
@@ -11,49 +13,122 @@
 
 namespace nix {
 
+static std::optional<std::string>
+getStringAttr(const StringMap & env, const StructuredAttrs * parsed, const std::string & name)
+{
+    if (parsed) {
+        auto i = parsed->structuredAttrs.find(name);
+        if (i == parsed->structuredAttrs.end())
+            return {};
+        else {
+            if (!i->is_string())
+                throw Error("attribute '%s' of must be a string", name);
+            return i->get<std::string>();
+        }
+    } else {
+        auto i = env.find(name);
+        if (i == env.end())
+            return {};
+        else
+            return i->second;
+    }
+}
+
+static bool getBoolAttr(const StringMap & env, const StructuredAttrs * parsed, const std::string & name, bool def)
+{
+    if (parsed) {
+        auto i = parsed->structuredAttrs.find(name);
+        if (i == parsed->structuredAttrs.end())
+            return def;
+        else {
+            if (!i->is_boolean())
+                throw Error("attribute '%s' must be a Boolean", name);
+            return i->get<bool>();
+        }
+    } else {
+        auto i = env.find(name);
+        if (i == env.end())
+            return def;
+        else
+            return i->second == "1";
+    }
+}
+
+static std::optional<Strings>
+getStringsAttr(const StringMap & env, const StructuredAttrs * parsed, const std::string & name)
+{
+    if (parsed) {
+        auto i = parsed->structuredAttrs.find(name);
+        if (i == parsed->structuredAttrs.end())
+            return {};
+        else {
+            if (!i->is_array())
+                throw Error("attribute '%s' must be a list of strings", name);
+            Strings res;
+            for (auto j = i->begin(); j != i->end(); ++j) {
+                if (!j->is_string())
+                    throw Error("attribute '%s' must be a list of strings", name);
+                res.push_back(j->get<std::string>());
+            }
+            return res;
+        }
+    } else {
+        auto i = env.find(name);
+        if (i == env.end())
+            return {};
+        else
+            return tokenizeString<Strings>(i->second);
+    }
+}
+
+static std::optional<StringSet>
+getStringSetAttr(const StringMap & env, const StructuredAttrs * parsed, const std::string & name)
+{
+    auto ss = getStringsAttr(env, parsed, name);
+    return ss ? (std::optional{StringSet{ss->begin(), ss->end()}}) : (std::optional<StringSet>{});
+}
+
 using OutputChecks = DerivationOptions::OutputChecks;
 
 using OutputChecksVariant = std::variant<OutputChecks, std::map<std::string, OutputChecks>>;
 
-DerivationOptions
-DerivationOptions::fromParsedDerivation(const StoreDirConfig & store, const ParsedDerivation & parsed, bool shouldWarn)
+DerivationOptions DerivationOptions::fromStructuredAttrs(
+    const StoreDirConfig & store, const StringMap & env, const StructuredAttrs * parsed, bool shouldWarn)
 {
     DerivationOptions defaults = {};
 
-    auto structuredAttrs = parsed.structuredAttrs.get();
-
-    if (shouldWarn && structuredAttrs) {
-        if (get(*structuredAttrs, "allowedReferences")) {
+    if (shouldWarn && parsed) {
+        if (get(parsed->structuredAttrs, "allowedReferences")) {
             warn(
                 "'structuredAttrs' disables the effect of the top-level attribute 'allowedReferences'; use 'outputChecks' instead");
         }
-        if (get(*structuredAttrs, "allowedRequisites")) {
+        if (get(parsed->structuredAttrs, "allowedRequisites")) {
             warn(
                 "'structuredAttrs' disables the effect of the top-level attribute 'allowedRequisites'; use 'outputChecks' instead");
         }
-        if (get(*structuredAttrs, "disallowedRequisites")) {
+        if (get(parsed->structuredAttrs, "disallowedRequisites")) {
             warn(
                 "'structuredAttrs' disables the effect of the top-level attribute 'disallowedRequisites'; use 'outputChecks' instead");
         }
-        if (get(*structuredAttrs, "disallowedReferences")) {
+        if (get(parsed->structuredAttrs, "disallowedReferences")) {
             warn(
                 "'structuredAttrs' disables the effect of the top-level attribute 'disallowedReferences'; use 'outputChecks' instead");
         }
-        if (get(*structuredAttrs, "maxSize")) {
+        if (get(parsed->structuredAttrs, "maxSize")) {
             warn(
                 "'structuredAttrs' disables the effect of the top-level attribute 'maxSize'; use 'outputChecks' instead");
         }
-        if (get(*structuredAttrs, "maxClosureSize")) {
+        if (get(parsed->structuredAttrs, "maxClosureSize")) {
             warn(
                 "'structuredAttrs' disables the effect of the top-level attribute 'maxClosureSize'; use 'outputChecks' instead");
         }
     }
 
     return {
         .outputChecks = [&]() -> OutputChecksVariant {
-            if (auto structuredAttrs = parsed.structuredAttrs.get()) {
+            if (parsed) {
                 std::map<std::string, OutputChecks> res;
-                if (auto outputChecks = get(*structuredAttrs, "outputChecks")) {
+                if (auto outputChecks = get(parsed->structuredAttrs, "outputChecks")) {
                     for (auto & [outputName, output] : getObject(*outputChecks)) {
                         OutputChecks checks;
 
@@ -91,19 +166,19 @@ DerivationOptions::fromParsedDerivation(const StoreDirConfig & store, const Pars
                 return OutputChecks{
                     // legacy non-structured-attributes case
                     .ignoreSelfRefs = true,
-                    .allowedReferences = parsed.getStringSetAttr("allowedReferences"),
-                    .disallowedReferences = parsed.getStringSetAttr("disallowedReferences").value_or(StringSet{}),
-                    .allowedRequisites = parsed.getStringSetAttr("allowedRequisites"),
-                    .disallowedRequisites = parsed.getStringSetAttr("disallowedRequisites").value_or(StringSet{}),
+                    .allowedReferences = getStringSetAttr(env, parsed, "allowedReferences"),
+                    .disallowedReferences = getStringSetAttr(env, parsed, "disallowedReferences").value_or(StringSet{}),
+                    .allowedRequisites = getStringSetAttr(env, parsed, "allowedRequisites"),
+                    .disallowedRequisites = getStringSetAttr(env, parsed, "disallowedRequisites").value_or(StringSet{}),
                 };
             }
         }(),
         .unsafeDiscardReferences =
             [&] {
                 std::map<std::string, bool> res;
 
-                if (auto structuredAttrs = parsed.structuredAttrs.get()) {
-                    if (auto udr = get(*structuredAttrs, "unsafeDiscardReferences")) {
+                if (parsed) {
+                    if (auto udr = get(parsed->structuredAttrs, "unsafeDiscardReferences")) {
                         for (auto & [outputName, output] : getObject(*udr)) {
                             if (!output.is_boolean())
                                 throw Error("attribute 'unsafeDiscardReferences.\"%s\"' must be a Boolean", outputName);
@@ -117,8 +192,8 @@ DerivationOptions::fromParsedDerivation(const StoreDirConfig & store, const Pars
         .passAsFile =
             [&] {
                 StringSet res;
-                if (auto * passAsFileString = get(parsed.env, "passAsFile")) {
-                    if (parsed.hasStructuredAttrs()) {
+                if (auto * passAsFileString = get(env, "passAsFile")) {
+                    if (parsed) {
                         if (shouldWarn) {
                             warn(
                                 "'structuredAttrs' disables the effect of the top-level attribute 'passAsFile'; because all JSON is always passed via file");
@@ -133,8 +208,8 @@ DerivationOptions::fromParsedDerivation(const StoreDirConfig & store, const Pars
             [&] {
                 std::map<std::string, StorePathSet> ret;
 
-                if (auto structuredAttrs = parsed.structuredAttrs.get()) {
-                    auto e = optionalValueAt(*structuredAttrs, "exportReferencesGraph");
+                if (parsed) {
+                    auto e = optionalValueAt(parsed->structuredAttrs, "exportReferencesGraph");
                     if (!e || !e->is_object())
                         return ret;
                     for (auto & [key, storePathsJson] : getObject(*e)) {
@@ -144,10 +219,10 @@ DerivationOptions::fromParsedDerivation(const StoreDirConfig & store, const Pars
                         ret.insert_or_assign(key, std::move(storePaths));
                     }
                 } else {
-                    auto s = getOr(parsed.env, "exportReferencesGraph", "");
+                    auto s = getOr(env, "exportReferencesGraph", "");
                     Strings ss = tokenizeString<Strings>(s);
                     if (ss.size() % 2 != 0)
-                        throw BuildError("odd number of tokens in 'exportReferencesGraph': '%1%'", s);
+                        throw Error("odd number of tokens in 'exportReferencesGraph': '%1%'", s);
                     for (Strings::iterator i = ss.begin(); i != ss.end();) {
                         auto fileName = std::move(*i++);
                         static std::regex regex("[A-Za-z_][A-Za-z0-9_.-]*");
@@ -163,15 +238,15 @@ DerivationOptions::fromParsedDerivation(const StoreDirConfig & store, const Pars
                 return ret;
             }(),
         .additionalSandboxProfile =
-            parsed.getStringAttr("__sandboxProfile").value_or(defaults.additionalSandboxProfile),
-        .noChroot = parsed.getBoolAttr("__noChroot", defaults.noChroot),
-        .impureHostDeps = parsed.getStringSetAttr("__impureHostDeps").value_or(defaults.impureHostDeps),
-        .impureEnvVars = parsed.getStringSetAttr("impureEnvVars").value_or(defaults.impureEnvVars),
-        .allowLocalNetworking = parsed.getBoolAttr("__darwinAllowLocalNetworking", defaults.allowLocalNetworking),
+            getStringAttr(env, parsed, "__sandboxProfile").value_or(defaults.additionalSandboxProfile),
+        .noChroot = getBoolAttr(env, parsed, "__noChroot", defaults.noChroot),
+        .impureHostDeps = getStringSetAttr(env, parsed, "__impureHostDeps").value_or(defaults.impureHostDeps),
+        .impureEnvVars = getStringSetAttr(env, parsed, "impureEnvVars").value_or(defaults.impureEnvVars),
+        .allowLocalNetworking = getBoolAttr(env, parsed, "__darwinAllowLocalNetworking", defaults.allowLocalNetworking),
         .requiredSystemFeatures =
-            parsed.getStringSetAttr("requiredSystemFeatures").value_or(defaults.requiredSystemFeatures),
-        .preferLocalBuild = parsed.getBoolAttr("preferLocalBuild", defaults.preferLocalBuild),
-        .allowSubstitutes = parsed.getBoolAttr("allowSubstitutes", defaults.allowSubstitutes),
+            getStringSetAttr(env, parsed, "requiredSystemFeatures").value_or(defaults.requiredSystemFeatures),
+        .preferLocalBuild = getBoolAttr(env, parsed, "preferLocalBuild", defaults.preferLocalBuild),
+        .allowSubstitutes = getBoolAttr(env, parsed, "allowSubstitutes", defaults.allowSubstitutes),
     };
 }
 

src/libstore/derivation-options.hh

@@ -14,16 +14,16 @@ namespace nix {
 
 class Store;
 struct BasicDerivation;
-class ParsedDerivation;
+struct StructuredAttrs;
 
 /**
  * This represents all the special options on a `Derivation`.
  *
  * Currently, these options are parsed from the environment variables
- * with the aid of `ParsedDerivation`.
+ * with the aid of `StructuredAttrs`.
  *
  * The first goal of this data type is to make sure that no other code
- * uses `ParsedDerivation` to ad-hoc parse some additional options. That
+ * uses `StructuredAttrs` to ad-hoc parse some additional options. That
  * ensures this data type is up to date and fully correct.
  *
  * The second goal of this data type is to allow an alternative to
@@ -174,8 +174,8 @@ struct DerivationOptions
      * (e.g. JSON) but is necessary for supporing old formats (e.g.
      * ATerm).
      */
-    static DerivationOptions
-    fromParsedDerivation(const StoreDirConfig & store, const ParsedDerivation & parsed, bool shouldWarn = true);
+    static DerivationOptions fromStructuredAttrs(
+        const StoreDirConfig & store, const StringMap & env, const StructuredAttrs * parsed, bool shouldWarn = true);
 
     /**
      * @param drv Must be the same derivation we parsed this from. In

src/libstore/misc.cc

@@ -222,10 +222,13 @@ void Store::queryMissing(const std::vector<DerivedPath> & targets,
             if (knownOutputPaths && invalid.empty()) return;
 
             auto drv = make_ref<Derivation>(derivationFromPath(drvPath));
-            ParsedDerivation parsedDrv(drv->env);
+            auto parsedDrv = StructuredAttrs::tryParse(drv->env);
             DerivationOptions drvOptions;
             try {
-                drvOptions = DerivationOptions::fromParsedDerivation(*this, parsedDrv);
+                drvOptions = DerivationOptions::fromStructuredAttrs(
+                    *this,
+                    drv->env,
+                    parsedDrv ? &*parsedDrv : nullptr);
             } catch (Error & e) {
                 e.addTrace({}, "while parsing derivation '%s'", printStorePath(drvPath));
                 throw;