Parse string context elements properly by Ericson2314 · Pull Request #7543 · NixOS/nix

Ericson2314 · 2023-01-03T17:16:23Z

Prior to this change, we had a bunch of ad-hoc string manipulation code scattered around. This made it hard to figure out what data model for string contexts is.

Now, we still store string contexts most of the time as encoded strings --- I was wary of the performance implications of changing that --- but whenever we parse them we do so only through the
NixStringContextElem::parse method, which handles all cases. This creates a data type that is very similar to DerivedPath but:

Represents the funky =<drvpath> case as properly distinct from the others.
Only encodes a single output, no wildcards and no set, for the "built" case.

(I would like to deprecate =<path>, after which we are in spitting distance of DerivedPath and could maybe get away with fewer types, but that is another topic for another day.)

Additionally, App now uses DerivedPath not StorePathWithOutputs for its (simplified) context. This is just better as StorePathWithOutputs is the deprecated old type just for the old CLI.

thufschmitt

Nice cleaning up :)

Left a bunch of small comments, but LGTM overall

src/libexpr/value/context.cc

src/libcmd/installables.hh

src/libexpr/primops.cc

src/libexpr/primops/context.cc

thufschmitt · 2023-01-09T14:27:14Z

src/nix/app.cc

+                [&](const NixStringContextElem::DrvDeep & d) -> DerivedPath {
+                    /* We want all outputs of the drv */
+                    return DerivedPath::Built {
+                        .drvPath = d.drvPath,
+                        .outputs = {},
+                    };


Should we just throw an error here instead?

Or perhaps perform hash scanning?

This would be a breaking change (unfortunately) so I would rather save this for a subsequent PR that tries to constraint DrvDeep.

fricklerhandwerk · 2023-01-10T12:51:26Z

Reviewed in Nix team meeting on 2023-01-09:

@Ericson2314: deliberately gave the types cryptic names, because not sure what the code actually does in detail.
agreement: this PR is the right thing to do
@thufschmitt: we had the discussion about the variant pattern
- @Ericson2314: it's more verbose in the header but more readable at call sites
@roberth: parsing and serialising the type should be called fromString and toString
@fricklerhandwerk: is this syntax completely internal?
- @Ericson2314: we think so
- @thufschmitt: the next step would be to remove it altogether and only leave the data structures
  - one exception: eval cache, where the string context is stored to disk
    - but that one is transient and we can bump the version on an incompatible serialisation format
@thufschmitt: somewhat unrelated: not clear what PathSet & context means in each different case
- @Ericson2314: yes, but we should address this in a different PR

nixos-discourse · 2023-01-10T12:54:11Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-01-09-nix-team-meeting-minutes-22/24577/1

- Add a comment - Put `OutputsSpec` in a different header (First part of NixOS#6815) - Make a few stray uses of it in new code use `DerivedPath` instead.

Prior to this change, we had a bunch of ad-hoc string manipulation code scattered around. This made it hard to figure out what data model for string contexts is. Now, we still store string contexts most of the time as encoded strings --- I was wary of the performance implications of changing that --- but whenever we parse them we do so only through the `NixStringContextElem::parse` method, which handles all cases. This creates a data type that is very similar to `DerivedPath` but: - Represents the funky `=<drvpath>` case as properly distinct from the others. - Only encodes a single output, no wildcards and no set, for the "built" case. (I would like to deprecate `=<path>`, after which we are in spitting distance of `DerivedPath` and could maybe get away with fewer types, but that is another topic for another day.)

Ericson2314 · 2023-01-10T18:27:28Z

I called it to_string matching the convention elsewhere, but I kept parse as parse because that, and not from_string or fromString, is the convention today. I suppose we can deal with that later.

I ended up going with an exception not tests, because this (for now) exposed in the eval cache, and therefore we should have nice errors when reading a corrupted eval cache. Also, as a matter of practicality, I am not sure how to write failing parse tests when errors are caught with assert.

I think this should be ready now!

roberth · 2023-01-10T21:14:14Z

@roberth: parsing and serialising the type should be called fromString and toString

I pronounced it in snake case ;), and I'm also ok with parse.

Ericson2314 · 2023-01-10T22:16:36Z

src/libexpr/tests/value/context.cc

+TEST_F(NixStringContextElemTest, empty_invalid) {
+    EXPECT_THROW(
+        NixStringContextElem::parse(store(), ""),
+        BadNixStringContextElem);
+}
+
+TEST_F(NixStringContextElemTest, single_bang_invalid) {
+    EXPECT_THROW(
+        NixStringContextElem::parse(store(), "!"),
+        BadNixStringContextElem);
+}
+
+TEST_F(NixStringContextElemTest, double_bang_invalid) {
+    EXPECT_THROW(
+        NixStringContextElem::parse(store(), "!!/"),
+        BadStorePath);
+}
+
+TEST_F(NixStringContextElemTest, eq_slash_invalid) {
+    EXPECT_THROW(
+        NixStringContextElem::parse(store(), "=/"),
+        BadStorePath);
+}
+
+TEST_F(NixStringContextElemTest, slash_invalid) {
+    EXPECT_THROW(
+        NixStringContextElem::parse(store(), "/"),
+        BadStorePath);
+}
+
+TEST_F(NixStringContextElemTest, opaque) {
+    std::string_view opaque = "/nix/store/g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x";
+    auto elem = NixStringContextElem::parse(store(), opaque);
+    auto * p = std::get_if<NixStringContextElem::Opaque>(&elem);
+    ASSERT_TRUE(p);
+    ASSERT_EQ(p->path, store().parseStorePath(opaque));
+    ASSERT_EQ(elem.to_string(store()), opaque);
+}
+
+TEST_F(NixStringContextElemTest, drvDeep) {
+    std::string_view drvDeep = "=/nix/store/g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x.drv";
+    auto elem = NixStringContextElem::parse(store(), drvDeep);
+    auto * p = std::get_if<NixStringContextElem::DrvDeep>(&elem);
+    ASSERT_TRUE(p);
+    ASSERT_EQ(p->drvPath, store().parseStorePath(drvDeep.substr(1)));
+    ASSERT_EQ(elem.to_string(store()), drvDeep);
+}
+
+TEST_F(NixStringContextElemTest, built) {
+    std::string_view built = "!foo!/nix/store/g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x.drv";
+    auto elem = NixStringContextElem::parse(store(), built);
+    auto * p = std::get_if<NixStringContextElem::Built>(&elem);
+    ASSERT_TRUE(p);
+    ASSERT_EQ(p->output, "foo");
+    ASSERT_EQ(p->drvPath, store().parseStorePath(built.substr(5)));
+    ASSERT_EQ(elem.to_string(store()), built);
+}


Shiny new tests! :)

thufschmitt

Thanks, that looks good now :)

Motivation `PathSet` is not correct because string contexts have other forms (`Built` and `DrvDeep`) that are not rendered as plain store paths. Instead of wrongly using `PathSet`, or "stringly typed" using `StringSet`, use `std::std<StringContextElem>`. ----- In support of this change, `NixStringContext` is now defined as `std::std<StringContextElem>` not `std:vector<StringContextElem>`. The old definition was just used by a `getContext` method which was only used by the eval cache. In reflection of this, the `typedef` is now moved to the eval cache header, and the function a static function in the eval cache c++ file. Summarizing the previous paragraph: Old: - `value/context.hh`: `NixStringContext = std::vector<StringContextElem>` - `value.hh`: `NixStringContext Value::getContext(...)` New: - `value/context.hh`: `NixStringContext = std::set<StringContextElem>` - `eval-cache.hh`: `NixStringContextExact = std::vector<StringContextElem>` - `eval-cache.rcc`: `NixStringContext getExactContext(const Value &)` This makes `NixStringContext` the common thing, and narrows the scope of the odder and rarely used `NixStringContextExact`. ---- The string representation of string context elements no longer contains the store dir. The diff of `src/libexpr/tests/value/context.cc` should make clear what the new representation is, so we recommend reviewing that file first. This was done for two reasons: Less API churn: `Value::mkString` and friends did not take a `Store` before. But if `NixStringContextElem::{parse, to_string}` *do* take a store (as they did before), then we cannot have the `Value` functions use them (in order to work with the fully-structured `NixStringContext`) without adding that argument. That would have been a lot of churn of threading the store, and this diff is already large enough, so the easier and less invasive thing to do was simply make the element `parse` and `to_string` functions not take the `Store` reference, and the easiest way to do that was to simply drop the store dir. Space usage: Dropping the `/nix/store/` (or similar) from the internal representation will safe space in the heap of the Nix programming being interpreted. If the heap contains many strings with non-trivial contexts, the saving could add up to something significant. ---- The eval cache version is bumped. The eval cache serialization uses `NixStringContextElem::{parse, to_string}`, and since those functions are changed per the above, that means the on-disk representation is also changed. This is simply done by changing the name of the used for the eval cache from `eval-cache-v4` to eval-cache-v5`. ---- To avoid some duplication `EvalCache::mkPathString` is added to abstract over the simple case of turning a store path to a string with just that string in the context. Context This PR picks up where NixOS#7543 left off. That one introduced the fully structured `NixStringContextElem` data type, but kept `PathSet context` as an awkward middle ground between internal `char[][]` interpreter heap string contexts and `NixStringContext` fully parsed string contexts. The infelicity of `PathSet context` was specifically called out during Nix team group review, but it was agreeing that fixing it could be left as future work. This is that future work. A possible follow-up step would be to get rid of the `char[][]` evaluator heap representation, too, but it is not yet clear how to do that. To use `NixStringContextElem` there we would need to get the STL containers to GC pointers in the GC build, and I am not sure how to do that. ---- PR NixOS#7543 effectively is writing the inverse of a `mkPathString`, `mkOutputString`, and one more such function for the `DrvDeep` case. I would like that PR to have property tests ensuring it is actually the inverse as expected. This PR sets things up nicely so that reworking that PR to be in that more elegant and better tested way is possible.

Motivation `PathSet` is not correct because string contexts have other forms (`Built` and `DrvDeep`) that are not rendered as plain store paths. Instead of wrongly using `PathSet`, or "stringly typed" using `StringSet`, use `std::std<StringContextElem>`. ----- In support of this change, `NixStringContext` is now defined as `std::std<StringContextElem>` not `std:vector<StringContextElem>`. The old definition was just used by a `getContext` method which was only used by the eval cache. In reflection of this, the `typedef` is now moved to the eval cache header, and the function a static function in the eval cache c++ file. Summarizing the previous paragraph: Old: - `value/context.hh`: `NixStringContext = std::vector<StringContextElem>` - `value.hh`: `NixStringContext Value::getContext(...)` New: - `value/context.hh`: `NixStringContext = std::set<StringContextElem>` - `eval-cache.hh`: `NixStringContextExact = std::vector<StringContextElem>` - `eval-cache.rcc`: `NixStringContext getExactContext(const Value &)` This makes `NixStringContext` the common thing, and narrows the scope of the odder and rarely used `NixStringContextExact`. ---- The string representation of string context elements no longer contains the store dir. The diff of `src/libexpr/tests/value/context.cc` should make clear what the new representation is, so we recommend reviewing that file first. This was done for two reasons: Less API churn: `Value::mkString` and friends did not take a `Store` before. But if `NixStringContextElem::{parse, to_string}` *do* take a store (as they did before), then we cannot have the `Value` functions use them (in order to work with the fully-structured `NixStringContext`) without adding that argument. That would have been a lot of churn of threading the store, and this diff is already large enough, so the easier and less invasive thing to do was simply make the element `parse` and `to_string` functions not take the `Store` reference, and the easiest way to do that was to simply drop the store dir. Space usage: Dropping the `/nix/store/` (or similar) from the internal representation will safe space in the heap of the Nix programming being interpreted. If the heap contains many strings with non-trivial contexts, the saving could add up to something significant. ---- The eval cache version is bumped. The eval cache serialization uses `NixStringContextElem::{parse, to_string}`, and since those functions are changed per the above, that means the on-disk representation is also changed. This is simply done by changing the name of the used for the eval cache from `eval-cache-v4` to eval-cache-v5`. ---- To avoid some duplication `EvalCache::mkPathString` is added to abstract over the simple case of turning a store path to a string with just that string in the context. Context This PR picks up where NixOS#7543 left off. That one introduced the fully structured `NixStringContextElem` data type, but kept `PathSet context` as an awkward middle ground between internal `char[][]` interpreter heap string contexts and `NixStringContext` fully parsed string contexts. The infelicity of `PathSet context` was specifically called out during Nix team group review, but it was agreeing that fixing it could be left as future work. This is that future work. A possible follow-up step would be to get rid of the `char[][]` evaluator heap representation, too, but it is not yet clear how to do that. To use `NixStringContextElem` there we would need to get the STL containers to GC pointers in the GC build, and I am not sure how to do that. ---- PR NixOS#7543 effectively is writing the inverse of a `mkPathString`, `mkOutputString`, and one more such function for the `DrvDeep` case. I would like that PR to have property tests ensuring it is actually the inverse as expected. This PR sets things up nicely so that reworking that PR to be in that more elegant and better tested way is possible. Co-authored-by: Théophane Hufschmitt <[email protected]>

Motivation `PathSet` is not correct because string contexts have other forms (`Built` and `DrvDeep`) that are not rendered as plain store paths. Instead of wrongly using `PathSet`, or "stringly typed" using `StringSet`, use `std::std<StringContextElem>`. ----- In support of this change, `NixStringContext` is now defined as `std::std<StringContextElem>` not `std:vector<StringContextElem>`. The old definition was just used by a `getContext` method which was only used by the eval cache. It can be deleted altogether since the tyes are now unified and the preexisting `copyContext` function already suffices. Summarizing the previous paragraph: Old: - `value/context.hh`: `NixStringContext = std::vector<StringContextElem>` - `value.hh`: `NixStringContext Value::getContext(...)` - `value.hh`: `copyContext(...)` New: - `value/context.hh`: `NixStringContext = std::set<StringContextElem>` - `value.hh`: `copyContext(...)` ---- The string representation of string context elements no longer contains the store dir. The diff of `src/libexpr/tests/value/context.cc` should make clear what the new representation is, so we recommend reviewing that file first. This was done for two reasons: Less API churn: `Value::mkString` and friends did not take a `Store` before. But if `NixStringContextElem::{parse, to_string}` *do* take a store (as they did before), then we cannot have the `Value` functions use them (in order to work with the fully-structured `NixStringContext`) without adding that argument. That would have been a lot of churn of threading the store, and this diff is already large enough, so the easier and less invasive thing to do was simply make the element `parse` and `to_string` functions not take the `Store` reference, and the easiest way to do that was to simply drop the store dir. Space usage: Dropping the `/nix/store/` (or similar) from the internal representation will safe space in the heap of the Nix programming being interpreted. If the heap contains many strings with non-trivial contexts, the saving could add up to something significant. ---- The eval cache version is bumped. The eval cache serialization uses `NixStringContextElem::{parse, to_string}`, and since those functions are changed per the above, that means the on-disk representation is also changed. This is simply done by changing the name of the used for the eval cache from `eval-cache-v4` to eval-cache-v5`. ---- To avoid some duplication `EvalCache::mkPathString` is added to abstract over the simple case of turning a store path to a string with just that string in the context. Context This PR picks up where NixOS#7543 left off. That one introduced the fully structured `NixStringContextElem` data type, but kept `PathSet context` as an awkward middle ground between internal `char[][]` interpreter heap string contexts and `NixStringContext` fully parsed string contexts. The infelicity of `PathSet context` was specifically called out during Nix team group review, but it was agreeing that fixing it could be left as future work. This is that future work. A possible follow-up step would be to get rid of the `char[][]` evaluator heap representation, too, but it is not yet clear how to do that. To use `NixStringContextElem` there we would need to get the STL containers to GC pointers in the GC build, and I am not sure how to do that. ---- PR NixOS#7543 effectively is writing the inverse of a `mkPathString`, `mkOutputString`, and one more such function for the `DrvDeep` case. I would like that PR to have property tests ensuring it is actually the inverse as expected. This PR sets things up nicely so that reworking that PR to be in that more elegant and better tested way is possible. Co-authored-by: Théophane Hufschmitt <[email protected]>

Motivation `PathSet` is not correct because string contexts have other forms (`Built` and `DrvDeep`) that are not rendered as plain store paths. Instead of wrongly using `PathSet`, or "stringly typed" using `StringSet`, use `std::std<StringContextElem>`. ----- In support of this change, `NixStringContext` is now defined as `std::std<StringContextElem>` not `std:vector<StringContextElem>`. The old definition was just used by a `getContext` method which was only used by the eval cache. It can be deleted altogether since the types are now unified and the preexisting `copyContext` function already suffices. Summarizing the previous paragraph: Old: - `value/context.hh`: `NixStringContext = std::vector<StringContextElem>` - `value.hh`: `NixStringContext Value::getContext(...)` - `value.hh`: `copyContext(...)` New: - `value/context.hh`: `NixStringContext = std::set<StringContextElem>` - `value.hh`: `copyContext(...)` ---- The string representation of string context elements no longer contains the store dir. The diff of `src/libexpr/tests/value/context.cc` should make clear what the new representation is, so we recommend reviewing that file first. This was done for two reasons: Less API churn: `Value::mkString` and friends did not take a `Store` before. But if `NixStringContextElem::{parse, to_string}` *do* take a store (as they did before), then we cannot have the `Value` functions use them (in order to work with the fully-structured `NixStringContext`) without adding that argument. That would have been a lot of churn of threading the store, and this diff is already large enough, so the easier and less invasive thing to do was simply make the element `parse` and `to_string` functions not take the `Store` reference, and the easiest way to do that was to simply drop the store dir. Space usage: Dropping the `/nix/store/` (or similar) from the internal representation will safe space in the heap of the Nix programming being interpreted. If the heap contains many strings with non-trivial contexts, the saving could add up to something significant. ---- The eval cache version is bumped. The eval cache serialization uses `NixStringContextElem::{parse, to_string}`, and since those functions are changed per the above, that means the on-disk representation is also changed. This is simply done by changing the name of the used for the eval cache from `eval-cache-v4` to eval-cache-v5`. ---- To avoid some duplication `EvalCache::mkPathString` is added to abstract over the simple case of turning a store path to a string with just that string in the context. Context This PR picks up where NixOS#7543 left off. That one introduced the fully structured `NixStringContextElem` data type, but kept `PathSet context` as an awkward middle ground between internal `char[][]` interpreter heap string contexts and `NixStringContext` fully parsed string contexts. The infelicity of `PathSet context` was specifically called out during Nix team group review, but it was agreeing that fixing it could be left as future work. This is that future work. A possible follow-up step would be to get rid of the `char[][]` evaluator heap representation, too, but it is not yet clear how to do that. To use `NixStringContextElem` there we would need to get the STL containers to GC pointers in the GC build, and I am not sure how to do that. ---- PR NixOS#7543 effectively is writing the inverse of a `mkPathString`, `mkOutputString`, and one more such function for the `DrvDeep` case. I would like that PR to have property tests ensuring it is actually the inverse as expected. This PR sets things up nicely so that reworking that PR to be in that more elegant and better tested way is possible. Co-authored-by: Théophane Hufschmitt <[email protected]>

Ericson2314 requested review from edolstra and thufschmitt as code owners January 3, 2023 17:16

Ericson2314 mentioned this pull request Jan 3, 2023

InstallableFlake::toDerivedPaths(): Support paths and store paths #7484

Merged

Ericson2314 requested review from roberth and tomberek January 3, 2023 17:46

Ericson2314 mentioned this pull request Jan 6, 2023

Tracking issue for RFC 92: Dynamic derivations #6316

Open

4 tasks

thufschmitt suggested changes Jan 9, 2023

View reviewed changes

Ericson2314 added 2 commits January 10, 2023 11:27

Make clear that StorePathWithOutputs is a deprecated type

da64f02

- Add a comment - Put `OutputsSpec` in a different header (First part of NixOS#6815) - Make a few stray uses of it in new code use `DerivedPath` instead.

Ericson2314 force-pushed the typed-string-context branch from 09fd5ac to 5576d5e Compare January 10, 2023 18:19

Ericson2314 assigned thufschmitt Jan 10, 2023

Ericson2314 commented Jan 10, 2023

View reviewed changes

Ericson2314 mentioned this pull request Jan 10, 2023

OutputSpec for DerivationGoal and DerivedPath, today's OutputSpec -> ExtendedOutputSpec #6815

Merged

thufschmitt approved these changes Jan 11, 2023

View reviewed changes

thufschmitt merged commit a3ba803 into NixOS:master Jan 11, 2023

Ericson2314 deleted the typed-string-context branch January 11, 2023 06:47

Ericson2314 mentioned this pull request Jan 29, 2023

Use std::set<StringContextElem> not PathSet for string contexts #7710

Merged

7 tasks

Ericson2314 added this to the dynamic-derivations stabilisation milestone Jan 15, 2025

Uh oh!

Comments

Conversation

Ericson2314 commented Jan 3, 2023

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thufschmitt Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

roberth Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fricklerhandwerk commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nixos-discourse commented Jan 10, 2023

Uh oh!

Ericson2314 commented Jan 10, 2023

Uh oh!

roberth commented Jan 10, 2023

Uh oh!

Ericson2314 Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Ericson2314 Jan 10, 2023 •

edited

Loading

fricklerhandwerk commented Jan 10, 2023 •

edited

Loading