These VM requests are only emitted by code gated behind the impure feature.
To prevent warning from popping up when building without default
features, allow these to be unused if `impure` is not enabled.
Change-Id: Id871a5215e9a0f09aa78edecdd111369ee7ffe34
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11722
Reviewed-by: Connor Brewster <cbrewster@hey.com>
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
These use StdIO, which is only available if the impure feature is
enabled.
Change-Id: I18d8e191a7eba6ba5bd59f43631973eaa796c7bb
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11721
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: tazjin <tazjin@tvl.su>
These are only needed when building with the impure feature enabled.
This removes some warnings when building with --no-default-features.
Change-Id: I3139d9133d4846aeb1b1b5f3830c0d078d047292
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11720
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
cl/11712 simultaneously introduced this check and made it unnecessary,
since NixString::context should never return `Some` for empty contexts
now.
Change-Id: I41a655ff33910e8326cbb7d7526eb91bd19e9585
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11713
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Both `Some(NixContext::new())` and `None` represent empty contexts,
but the former trips up `NixString::has_context`, and seems likely
to trip up other things.
We could hide the difference in the accessors, but we don't really
*want* the distinction to exist, since heap-allocating a null value
is pretty much always a mistake.
Change-Id: Ie84d26fb0d4b59e68354891ba13bde3bae40ab6e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11712
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
In places where we want to extend context with that from another
NixString, use take_context() to split it off, then call .extend(),
making use of IntoIterator to avoid a bunch of clones.
Change-Id: I2460141a3ed776c64c36132b2203b6a1d710b922
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11705
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: edef <edef@edef.eu>
XmlEmitter gains a NixContext field, and `write_typed_value` extends it
with all context elements present in the passed value.
Once all serialization is done, a into_context() function returns the
collected context, so we can construct a NixString with context.
Tests for this live in tvix-glue, as we use builtins.derivation, which
is not present in the tvix-eval crate.
Fixes b/398.
Change-Id: I85feaaa17b753885f8a017a54e419ec4e602af21
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11704
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: Alyssa Ross <hi@alyssa.is>
This is a slightly less annoying version of `join`, which does not
consume self. It's more consistent with HashSet::extend().
Change-Id: Ifd0872da36fe8e7b2aa6948674cb8e4023abe9d7
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11703
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: edef <edef@edef.eu>
In order to be compatible with the nix XML generator, it’s easier to
generate the XML directly, instead of going through a library which we
have to bend to do what we need.
Removes dependency on `xml-rs`, which came with a full XML parser that
we didn’t use. Only takes a tiny bit of code for the XML escaping,
somewhat simplified.
I add a little escaping value, to make sure we have the same behaviour
as nix proper.
Interestingly enough, we never need to escape XML attribute names,
because the `builtins.toXML` format encodes user-defined values as
attribute keys only. So we only escape attribute values.
Fixes: https://b.tvl.fyi/issues/399
Change-Id: If4d407d324864b3bb9aa3160e2ec6889f7727127
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11697
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: Profpatsch <mail@profpatsch.de>
Have this return the same values as builtins.unsafeGetAttrsPos, rather
than returning a CatchableErrorKind, which crashes the CLI if it bubbles
up.
The environment we're in doesn't allow emitting a warning, as we don't
have `co` in scope, but that's probably OK as a stopgap solution.
Alternative to cl/11665.
Change-Id: I5b2c2530842547c93b6533ed9601ee9b2923b1bf
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11685
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Prior to this, some tests would not build
or would fail in an obscure way.
Change-Id: I68587cc7592492ebfd71ca02fc7ccc9ff7c0196f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11544
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
This caught me by accident in an earlier revision of cl/11500 -
I had a `NixString`, wanted to return it as a `String`, so I was naively
calling `s.into()`.
That unfortunately gave me the `Display` implementation of `NixString`,
which quotes strings, causing an annoying error further up the stack.
NixStrings are bytes, we can keep the impl From<NixString> for BString,
but having a `.into()` suddenly do quoting is more than unexpected.
Change-Id: I5434ba94bfe6c493d0a57e68225ecc22daa4b948
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11505
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: tazjin <tazjin@tvl.su>
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
It was returning `{ line, col, file }` instead of `{ line, column, file
}`.
Note this is not a regression in Nix, Nix always had `column` since
2013.
Change-Id: I4871a201b32324dff0432526d0449806a535828f
Signed-off-by: Ryan Lahfa <tvl@lahfa.xyz>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11409
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
`toJSON` transform a Nix structure into a JSON string.
For each context in that Nix structure, the JSON string must possess it.
Thus, it is necessary to take the union of all contexts and attach it to
the final structure.
Unfortunately, the return type of `into_json` is a serde's JSON object,
not a string. Therefore, it is not possible to reuse `NixString`
machinery.
Context tests are reinforced as Nix does not test those behaviors.
Fixes b/393.
Change-Id: I5afdbc4e18dd70469192c1aa657d1049ba330149
Signed-off-by: Ryan Lahfa <tvl@lahfa.xyz>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11266
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
We don't have it in all Cargo.toml files either.
Change-Id: Ifcb333a39603a7e402ab0068e0f0f1b5af769e13
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11388
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
Reviewed-by: Brian Olsen <me@griff.name>
Autosubmit: flokli <flokli@flokli.de>
Right now `builtins.hashFile` always reads the entire file into memory
before hashing, which is not ideal for large files. This replaces
`read_to_string` with `open_file` which allows calculating the hash of
the file without buffering it entirely into memory. Other callers can
continue to buffer into memory if they choose, but they still use the
`open_file` VM request and then call `read_to_string` or `read_to_end`
on the `std::io::Reader`.
Fixes b/380
Change-Id: Ifa1c8324bcee8f751604b0b449feab875c632fda
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11236
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
`appendContext s ctx` will just append a user-crafted context attrs to `s`.
The most important part of this builtin is to perform all the relevant invariant validations
to avoid letting the user craft invalid contexts which can never be built, e.g. invalid store paths,
inexistent derivations, etc.
This version is incomplete and full of TODOs, but passes all the Nix's
context strings tests, so we turn them on.
Change-Id: I625dc5e7c4f5b784f078b390f04b0ee5a8d65a7c
Signed-off-by: Ryan Lahfa <tvl@lahfa.xyz>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11263
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This builtin only transforms any `NixContextElement::Derivation` into the trivial `NixContextElement::Plain`.
This is a forgetful functor on derivation-deep context strings.
The test coverage of this change is done in cl/11264.
Change-Id: Icd00778c97766be6db8a6bdabaa59e9724353ec5
Signed-off-by: Ryan Lahfa <tvl@lahfa.xyz>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11262
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Nixpkgs make use of it and this is a 2.3 builtin that was missed.
Change-Id: If36d748eed4a5f777344556c0802392c76c8d53c
Signed-off-by: Ryan Lahfa <tvl@lahfa.xyz>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11260
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
The accompanying test case shows that we need to bubble up the catchable
error from the equality check if one is created.
Change-Id: Ic9929a57aa7653c8aa5a72d1711cf3264798c731
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11159
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
It is pretty pointless to force the function argument if we are going to
use a suspended call later since forcing the function may fail in ways
that are not covered by Catchables (non-recoverable errors, infinite
recursions). From this, it kind of seems as if using #[catch] is never
correct and should be replaced by #[lazy]. Also we should probably try
to come up with more test cases for stuff where laziness gets us out of
the jam as an equivalent to the catchable tests for nonrecoverable
errors.
Fixes b/386.
Change-Id: Ia926df4ac1b440ec430403ab7b40924a0c97221b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11153
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
When we added the Nix language test suite in cl/6126, we excluded the
whole tvix tests folder from Nix formatting. This is unintentional, as
we probably want *our* tests to be formatted correctly.
Change-Id: I2b66d79e30fae17e75d5a1f8c44e279886091c5f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11154
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
The primary addition is the `builtin_hashFile` function which attempts
value-path coercion and on success reads the file to a nix_string.
Similar to the `builtins_hashString` implementation, the hash is then
derived using the `hash_nix_string` function in the introduced hash
module which has extracted hashing functionality initially specified in
cl/11005.
Change-Id: I5e3ef0317223af99488ebb79efc2fb49b4cbc1b6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11007
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Implements md5, sha1, sha256 and sha512 using the related crates from
the RustCrypto hashes project (https://github.com/RustCrypto/hashes)
Change-Id: I00730dea44ec9ef85309edc27addab0ae88814b8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11005
Tested-by: BuildkiteCI
Reviewed-by: aspen <root@gws.fyi>
Previously, Nix strings were represented as a Box (within Value)
pointing to a tuple of an optional context, and another Box pointing to
the actual string allocation itself. This is pretty inefficient, both in
terms of memory usage (we use 48 whole bytes for a None context!) and in
terms of the extra indirection required to get at the actual data. It
was necessary, however, because with native Rust DSTs if we had
something like `struct NixString(Option<NixContext>, BStr)` we could
only pass around *fat* pointers to that value (with the length in the
pointer) and that'd make Value need to be bigger (which is a waste of
both memory and cache space, since that memory would be unused for all
other Values).
Instead, this commit implements *manual* allocation of a packed string
representation, with the length *in the allocation* as a field past the
context. This requires a big old pile of unsafe Rust, but the payoff is
clear:
hello outpath time: [882.18 ms 897.16 ms 911.23 ms]
change: [-15.143% -13.819% -12.500%] (p = 0.00 < 0.05)
Performance has improved.
Fortunately this change can be localized entirely within
value/string.rs, since we were abstracting things out nicely.
Change-Id: Ibf56dd16c9c503884f64facbb7f0ac596463efb6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10852
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Autosubmit: aspen <root@gws.fyi>
With our values using bstr now, we're not restricted to only reading
files that contain valid UTF-8.
Update our `read_to_string` function to `read_to_end`
(named like `std::io::Read::read_to_end`), and have it return a Vec<u8>.
Change-Id: I87f0291dc855a132689576559c891d66c30ddf2b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11003
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: Pádraic Ó Mhuiris <patrick.morris.310@gmail.com>
Reviewed-by: flokli <flokli@flokli.de>
One last place where we needed to wrap the forcing of the element of a
list in `try_value!`. This fixes a previously `notyetpassing` test
Change-Id: I8827a3e39630e6959013b70bdfa9cbcb93f4e91c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10789
Autosubmit: aspen <root@gws.fyi>
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
(Re-)Adds an error variant that wraps a catchable error kind, which is
used for returning the result of an evaluation.
Previously this would return the internal catchable value, which would
lead to panics if users tried to use these. Somehow this was missed; I
think we need error output tests.
Change-Id: Id6e24aa2ce4ea4358a29b2e1cf4a6749986baf8c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10991
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
The previous behaviour (enabling `import`, but not allowing e.g.
`<nixpkgs/lib>` to resolve) was very confusing.
Now imports from NIX_PATH become enabled by default, unless the user
already overrode that behaviour with something else by setting
`Evaluation::nix_path` manually.
Change-Id: Iad970beb633d9887be4b185b01e6f5858d81bea3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10993
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
There's some code path where fancy_format_str()->Error::Display, which
recurses forever and overflows the stack. This was introduced in a
previous commit today.
Change-Id: I87a59492099f6c138c752478901b9aa614bb57cc
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10990
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: Peter Kolloch <info@eigenvalue.net>
Makes use of the SourceCode field now being stored directly in
errors (see parent CL). With this change, the default `Display`
implementation can now format errors correctly, and there is no need
to keep a `SourceCode` around just for error formatting.
Updates dependent crates (CLI, serde, tvixbolt) to use this correctly.
Change-Id: Iddc5d7a6b4bab391f30a999e4c68aca34304c059
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10987
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
With this change it's no longer necessary to track the SourceCode
struct separately from the evaluation for error reporting: It's just
stored directly in the errors.
This also ends up resolving an issue in compiler::bindings, where we
cloned the Arc containing file references way too often. In fact those
clones probably compensate for all additional SourceCode clones during
error construction now.
Change-Id: Ice93bf161e61f8ea3d48103435e20c53e6aa8c3a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10986
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This makes some of the work of configuring an arbitrary I/O handler
easier.
Change-Id: I158db3235fe83df6e709578ed515e0e028c20086
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10959
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Now that I've done a ton of things to make sure Value is small on the
stack (16 bytes, which is a perfectly reasonable size for a programming
language Value enum), add a test asserting it stays that way.
These size improvements have a measurable impact, too - here's the
`hello outpath` benchmark compared between canon (as of r/7495) and this
commit:
hello outpath time: [990.56 ms 995.83 ms 1.0070 s]
change: [-7.1397% -6.1302% -5.1651%] (p = 0.00 < 0.05)
Performance has improved.
Change-Id: If99a0976eab28eb5e516fcd2f4a0e068145af23e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10799
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
Autosubmit: aspen <root@gws.fyi>
Reviewed-by: sterni <sternenseemann@systemli.org>
This is now the only enum variant for Value that is larger than 8
bytes (it's 16 bytes), so boxing it (especially since it's not
perf-critical) allows us to get the Value size down to only 16 bytes!
Change-Id: I98598e2b762944448bef982e8ff7da6d6683c4aa
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10798
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Autosubmit: aspen <root@gws.fyi>
This reverts commit d3d41552cf.
This was well-intentioned, but now the boxed Path values are actually
the *largest* Value enum variants, at 16 bytes (because they're
fat-pointers, with a len) instead of 8 bytes like all the other values.
Having the double reference is a reasonable price to pay (it seems; more
benchmarks may end up disagreeing) for a smaller Value repr.
Change-Id: I0d3e84f646c8f5ffd0b7259c4e456637eea360f7
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10797
Tested-by: BuildkiteCI
Autosubmit: aspen <root@gws.fyi>
Reviewed-by: sterni <sternenseemann@systemli.org>
Storing a full BString here incurs the extra overhead of the capacity
for the inner byte-vector, which we basically never use as Nix strings
are immutable (and we don't do any mutation / sharing analysis).
Switching to a Box<BStr> cuts us from 72 bytes to 64 bytes per
string (and there are a lot of strings!)
Change-Id: I11f34c14a08fa02759f260b1c78b2a2b981714e4
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10794
Autosubmit: aspen <root@gws.fyi>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
These strings are allocated once and never changed, so they don't need
the additional overhead of a capacity given by String - instead, we can
use Box<str> and save on 16 bytes for each of these, *and* for each
Value since this is currently the largest Value variant.
Change-Id: I3e5cb070fe6c5bf82114c92d04f6bae775663a7e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10796
Autosubmit: aspen <root@gws.fyi>
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
serde_json::Value is pretty large, and is contributing (albeit not
exclusively) to the large size of the Value repr. Putting it in a box
is *especially* cheap (since it's rarely used) and allows us
to (eventually) cut down on the size of Value.
Change-Id: I005a802d8527b639beb4e938e3320b11ffa1ef23
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10795
Reviewed-by: sterni <sternenseemann@systemli.org>
Autosubmit: aspen <root@gws.fyi>
Tested-by: BuildkiteCI
Adds a bunch (notably certain overlapping) tests for catchable situations.
This should cover many scenarios, argument is catchable, element in argument is catchable, function returns
catchable in the middle of the processing, etc.
Co-authored-by: Aspen Smith <root@gws.fyi>
Change-Id: Icd722cf8dbc91a24f45cd540a328711e5826f76c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10621
Reviewed-by: aspen <root@gws.fyi>
Tested-by: BuildkiteCI
Correctly propagate the case where the *key* of an attrset is a
Value::Catchable (eg { "${builtins.throw "c"}" = "b"; }) in
`NixAttrs::construct`, by converting the return type to
`Result<Result<Self, CatchableErrorKind>, ErrorKind>` (ugh!!) and
correctly handling that everywhere (including an `expect` in the
Deserialize impl for NixAttrs, since afaict this is impossible to hit
when deserializing from stuff like JSON).
Change-Id: Ic4bc611fbfdab27c0bd8a40759689a87c4004a17
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10786
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
In order to correctly propagate errors in the comparator passed to
builtins.sort, we need to do all the sorting in a context where we can
short-circuit return `Value`s (because catchables are Values on the `Ok`
side of the Result , not `Err`s). Unfortunately this means we have
to *inline* the List `sort_by` implementation into the builtin_sort
function - fortunately this is the only place that was called so this is
relatively low cost. This does that, and adds the requisite `try_value!`
invocation to allow us to propagate comparator errors here.
As before, this doesn't include tests, primarily since those are coming
in the next commit.
Change-Id: I8453c3aa2cd82299eae89828e2a2bb118da4cd48
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10754
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Nix doesn't propagate errors for the function argument to some builtins,
like genList and map:
❯ nix repl
Welcome to Nix version 2.3.17. Type :? for help.
nix-repl> (builtins.tryEval (builtins.genList (builtins.throw "a") 10)).success
true
nix-repl> (builtins.tryEval (builtins.map (builtins.throw "a") [ "" ])).success
true
Note that this is untested as of this particular commit, only because a
big test suite covering all sorts of catchable error propagation issues
is coming next
Change-Id: I48c8eb390a541204b1a6d438c753fa1ca9b3877e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10753
Autosubmit: aspen <root@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Propagate catchables that we get from forcing thunks in builtins in a
few more places using the new try_value! macro
Change-Id: I95fd41a231f877ff153f4adbabd944372d4cc7eb
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10738
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
Rather than explicitly checking for Value::Catchable in all builtins,
make the #[builtin] proc macro insert this for all strict arguments by
default, with support for a #[catch] attribute on the argument to
disable this behavior. That attribute hasn't actually been *used*
anywhere here, primarily because the tests pass without it, even for
those builtins which weren't previously checking for Value::Catchable -
if some time passes without this being used I might get rid of support
for it entirely.
There's also a `try_value` macro in builtins directly for the places
where builtins were eg forcing something, then explicitly propagating a
catchable value.
Change-Id: Ie22037b9d3e305e3bdb682d105fe467bd90d53e9
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10732
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
NixString is *quite* large - like 80 bytes - because of the extra
capacity value for BString and because of the context. We want to keep
Value small since we're passing it around a lot, so let's box the
NixString inside Value::String to save on some memory, and make cloning
ostensibly a little cheaper
Change-Id: I343c8b4e7f61dc3dcbbaba4382efb3b3e5bbabb2
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10729
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
In the compiler, skip emitting an OpForce if the last op was an
OpConstant for a non-thunk constant. This gives a small (~1% on my
machine) perf boost, eg when evaluating hello.outPath:
❯ hyperfine \
"./before --no-warnings -E '(import <nixpkgs> {}).hello.outPath'" \
"./after --no-warnings -E '(import <nixpkgs> {}).hello.outPath'"
Benchmark 1: ./before --no-warnings -E '(import <nixpkgs> {}).hello.outPath'
Time (mean ± σ): 1.151 s ± 0.022 s [User: 1.003 s, System: 0.151 s]
Range (min … max): 1.123 s … 1.184 s 10 runs
Benchmark 2: ./after --no-warnings -E '(import <nixpkgs> {}).hello.outPath'
Time (mean ± σ): 1.140 s ± 0.022 s [User: 0.989 s, System: 0.152 s]
Range (min … max): 1.115 s … 1.175 s 10 runs
Summary
./after --no-warnings -E '(import <nixpkgs> {}).hello.outPath' ran
1.01 ± 0.03 times faster than ./before --no-warnings -E '(import <nixpkgs> {}).hello.outPath'
Change-Id: I2105fd431d4bad699087907e16c789418e9a4062
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10714
Reviewed-by: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
PathBuf internally contains a heap pointer (an OsString), so we were in
effect double-boxing here. Removing the extra layer by making
Tvix::Value represented by a Box<Path> rather than a Box<PathBuf> saves
us an indirection, while still avoiding the extra memory overhead of the
capacity which was the reason we were boxing PathBuf in the first place.
Change-Id: I8c185b9d4646161d1921917f83e87421496a3e24
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10725
Reviewed-by: sterni <sternenseemann@systemli.org>
Autosubmit: aspen <root@gws.fyi>
Tested-by: BuildkiteCI
C++ nix uses C-style zero-terminated char pointers to represent strings
internally - however, up to this point, tvix has used Rust `String` and
`str` for string values. Since those are required to be valid utf-8, we
haven't been able to properly represent all the string values that Nix
supports.
To fix that, this change converts the internal representation of the
NixString struct from `Box<str>` to `BString`, from the `bstr` crate -
this is a wrapper around a `Vec<u8>` with extra functions for treating
that byte vector as a "morally string-like" value, which is basically
exactly what we need.
Since this changes a pretty fundamental assumption about a pretty core
type, there are a *lot* of changes in a lot of places to make this work,
but I've tried to keep the general philosophy and intent of most of the
code in most places intact. Most notably, there's nothing that's been
done to make the derivation stuff in //tvix/glue work with non-utf8
strings everywhere, instead opting to just convert to String/str when
passing things into that - there *might* be something to be done there,
but I don't know what the rules should be and I don't want to figure
them out in this change.
To deal with OS-native paths in a way that also works in WASM for
tvixbolt, this also adds a dependency on the "os_str_bytes" crate.
Fixes: b/189
Fixes: b/337
Change-Id: I5e6eb29c62f47dd91af954f5e12bfc3d186f5526
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10200
Reviewed-by: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Reviewed-by: sterni <sternenseemann@systemli.org>
Autosubmit: aspen <root@gws.fyi>
Tested-by: BuildkiteCI
Add a new --trace-runtime-timing flag (probably a better bikeshed for
this) that enables capturing the time, relative to the last event, of
each event recorded with the tracing observer.
This probably isn't *super* useful yet, but I'd like to start here in
adding new profiling tools to the VM, specifically based on the runtime
observer
Change-Id: Id7f12077291c39bf3eef42ab6744bfba53687a65
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10713
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
In case a .exp.xml file is provided alongside the test, compare its
output with the desired state.
Also, add some function .exp.xml that were presumably moved out of the
way back to src/tests/nix_tests, as they now produce the correct XML
output.
Change-Id: Ibd8123f3e6ed7bae3a44407d2284a2b2c8ce9a28
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10687
Reviewed-by: sterni <sternenseemann@systemli.org>
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
These need to be preserved at least for builtins.toXML.
Also, we incorrectly only wrote an <attrspat> in case ellipsis was true,
but that's not the case.
Change-Id: I6bff9c47c2922f878d5c43e48280cda9c9ddb692
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10686
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: aspen <root@gws.fyi>
At least toXML wants to get these out in a sorted fashion.
Change-Id: I6373d7488fff7c40dc2ddeeecd03ba537c92c4af
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10685
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
It's debateable on whether the serialization code should be exposed a
bit more prominently or not.
Change-Id: Iff7a28f884b1490b12b145dfdadbedacb84fd387
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10684
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
This relies on TEST_VAR=foo being set to "foo".
Nix does this in tests/functional/lang.sh, we do it in the test suite.
Change-Id: I7ffa9ed27124530b7758aeadf07c79477656f34f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10683
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
There's no `expected_failures` feature, we run them unconditionally.
Change-Id: Ibe1c93497e040d0d5b6cbfcaa043027814c191f1
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10681
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
Our docs say it's still todo, but that's wrong, it landed in cl/7835
and cl/7962.
Change-Id: Ice374ae47ab653146be18c02dce27145e7488397
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10674
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: sterni <sternenseemann@systemli.org>
- Adjust to ecl 23.9.9 release
- Regenerate go protos after protoc-gen-go update
- Drop dhall fork which hasn't kept up with 1.42.*
- Address new clippy warnings:
- Variant naming of Error::ValidationError
- Simplify .try_into().unwrap()
- Drop unnecessary identity function
- Test module must be last in file
- Drop unused `pub use`
- Update agenix to 0.15.0. Current master has a installCheckPhase that
doesn't work with C++ Nix 2.3.*:
a23aa271be (commitcomment-137185861)
Change-Id: Ic29eef20d6fd1362ce1031364a5ca6b4edf195bd
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10615
Reviewed-by: aspen <root@gws.fyi>
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Have a Evaluation::new() function that's used to set up the Evaluation
struct initially - which is also used by both new_pure and new_impure
internally.
It's generic over the exact type of IO, making it easier to instantiate
Evaluation with non-tvix-eval EvalIO implementations, that might not be
in a Box.
Change-Id: Ibf728da24aca59639c5b6df58d00ae98c99a63f5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10640
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Don't restrict to a Box<dyn EvalIO>.
There's still one or two places where we do restrict, this will be
solved by b/262.
Change-Id: Ic8d927d6ea81fa12d90b1e4352f35ffaafbd1adf
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10639
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
`throw (throw "a")` should work and propagate the internal throw.
Before this commit, it didn't work.
Change-Id: Id5d46f74e484dba99e912ad9fa211f3bf1617bac
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10600
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
`elem` did not catch the list being a catchable.
This surfaced during Nixpkgs evaluation.
Change-Id: Icf19b94e914e35a435c4412d769ee63ba59ab7b0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10599
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This is an additional test suite on the top of the Nix ones
for context strings matters.
It already smoked out multiple mistakes and potential bugs and non-deterministic result from the evaluator.
It uses a similar technology as the one in the tvix-eval albeit we instantiate a fully fledged evaluator
with in-memory store.
We copy the files instead of symlinking them because crates are built in
isolation, so symlinks cannot work.
Change-Id: I63ae225ce4f83c6e2c8ccd60d779c2f8eb9d08fb
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10619
Autosubmit: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Previously, we were assembling very naively an attribute set composed of context we saw.
But it was forgetting that `"${drv}${drv.drvPath}"` would contain 2 contexts with the same key, but
with different values, one with `outputs = [ "out" ];` and `allOutputs = true;`.
Following this reasoning and comparing with what Nix does, we ought to merge underlying values systematically.
Hence, I bring `itertools` to perform a group by on the key and merge everything on the fly, it's not
beautiful but it's the best I could find, notice that I don't use
`group_by` but I talk about group by, that is, because `group_by` is a
`group_by_consecutive`, see
https://github.com/rust-itertools/itertools/issues/374.
Initially, I tried to do it without a `into_grouping_map_by`, it was akin to assemble the final `NixAttrs` directly,
it was less readable and harder to pull out because we don't have a lot of in-place mutable functions on
our data structures.
Change-Id: I9933c9bd88ffe04de50dda14f21879b60d8b8cd4
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10620
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Yes, `hasContext e` should work where `e` is a contextful strings, otherwise, it is really useless.
Change-Id: I5eb071fc257217d6e8a63fe519132ebd98186696
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10617
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
`test-generator` has not been updated in the past 2 years.
`rstest` has not been updated in the past 5 months.
This is an improvement in the maintenance state… I guess?
We get also new features, it changes the name of the tests with numbers too.
Change-Id: I5376104c7704f525dba7524da78daa09867cc669
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10623
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
We want to handle bottoms in a consistent fashion. Previously this was
handled by repetitive is_catchable checks, which were not consistently
present.
Change-Id: I9614c479cc6297d1f64efba22b620a26e2a96802
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10485
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
A bunch of operations in Tvix are not aware of catchable values
and does not propagate them.
In the meantime, as we wait for a better solution, we just offer this
commit for moving the needle.
Change-Id: Ic3f0e1550126b0847b597dfc1402c35e0eeef469
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10473
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Allow other crates (like tvix-glue) to look at a Value in JSON, which is
used by the structured attrs feature.
Change-Id: Iba02ace6e11a74c3f9b19dcbef4b008b76dec046
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10602
Reviewed-by: tazjin <tazjin@tvl.su>
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
The error message is misleading. The errors we return can happen both
during serialization or deserialization, though the messages suggested
the latter only.
Change-Id: I2dafe17ec78ee75cab5937a3a81540fda3175eac
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10603
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
cl/10475 updated the API of tvix_eval::Evaluation, but didn't update
this benchmark to use that new API.
Also, fixes the docstring to no longer specify that there is a "given
snippet".
Change-Id: Ibb8285731849dbeec814e2585bbaa36f22368afe
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10542
Autosubmit: aspen <root@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Nix does something like:
```cpp
NixStringContext context;
const auto str = state.forceString(*args[1], context, pos, "while evaluating the second argument passed to builtins.split");
```
And then do nothing with that context, therefore, we follow them and
make `split` aware of the context but still do nothing with it.
Change-Id: I4fee1936600ce86d99d00893ca3f64013213935b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10428
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: raitobezarius <tvl@lahfa.xyz>
This removes the reference tracking and uses instead the context for
references and returns some.
Change-Id: Ic359ca6b903b63f1a9c679c566004c617b792442
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10435
Autosubmit: raitobezarius <tvl@lahfa.xyz>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
In the past reference tracking system, `tvix-io` glue was appending
plain paths in the known path state.
Now, we make up for this by just making contextful coercion of file
imports.
Change-Id: Ieb9b04dd83302c77909252d5f7733857ac3cf8fd
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10443
Tested-by: BuildkiteCI
Autosubmit: raitobezarius <tvl@lahfa.xyz>
Reviewed-by: tazjin <tazjin@tvl.su>
String with contexts are always coerced to a string with the same
context.
Change-Id: I224814febd9cad196bb28876793e76bed564dc72
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10440
Tested-by: BuildkiteCI
Autosubmit: raitobezarius <tvl@lahfa.xyz>
Reviewed-by: sterni <sternenseemann@systemli.org>
Otherwise, you just fail because they are not... contextless strings!
Change-Id: I0b8f63a18cd89c3841b613d41c12ec4ee336f953
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10442
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
`match` silently ignore the input context and do not propagate
it and successful matches.
The why is unclear but nixpkgs does rely implicitly on this behavior
because dynamic attribute selection cannot be done with contextful
strings.
Change-Id: I5167fa9b2c2db8ecab0c2fb3e9895c9cfce6eeb2
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10441
Autosubmit: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
And it also preserve the original context if it exists.
Change-Id: I904f7c13b7f003a267aace6301723780fccaafb7
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10434
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: raitobezarius <tvl@lahfa.xyz>
We just perform union of contexts of every pieces.
Change-Id: Ief925c1818cd8bbec0503e9c625b0630feebfdda
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10432
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: raitobezarius <tvl@lahfa.xyz>
`substring` has a very funny behavior when it comes to empty strings,
it propagates the context too, this is used in nixpkgs to attach context
to strings without using any builtin: `lib.addContextFrom`.
Change-Id: Id655356799b3485f7519b3d1914c630f9d8416c3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10448
Autosubmit: raitobezarius <tvl@lahfa.xyz>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
