Update flake and fix build with mostly async liblixstore #25

Merged
ma27 merged 1 commit from ma27/hydra:flake-update into main 2025-03-12 12:35:11 +00:00
Member

cc @raito @pennae a review would be greatly appreciated (and as always, feel free to point out the weird stuff, I don't do too much C++).

The vast majority of changes was pretty straight-forward: we need an
AsyncIoRoot per thread to resolve promises we get from Lix. The aio
object is created per thread and passed as lvalue ref instead of making
it a field of the State struct since that struct is shared between the
threads.

The biggest change affects the NAR extractor: parseAndCopyDump has been
removed, instead the nar::parse and nar::dump parts are public now.
This won't work with the original parse visitor anymore, so instead the
NAR is parsed once and dumped again to copy it to the binary cache. In
between, the generator is walked through to search for relevant files
(such as build products) and after that the generators are recreated for
AsyncGeneratorInputStream to read again.

cc @raito @pennae a review would be greatly appreciated (and as always, feel free to point out the weird stuff, I don't do too much C++). The vast majority of changes was pretty straight-forward: we need an AsyncIoRoot per thread to resolve promises we get from Lix. The `aio` object is created per thread and passed as lvalue ref instead of making it a field of the State struct since that struct is shared between the threads. The biggest change affects the NAR extractor: parseAndCopyDump has been removed, instead the `nar::parse` and `nar::dump` parts are public now. This won't work with the original parse visitor anymore, so instead the NAR is parsed once and dumped again to copy it to the binary cache. In between, the generator is walked through to search for relevant files (such as build products) and after that the generators are recreated for `AsyncGeneratorInputStream` to read again.
The vast majority of changes was pretty straight-forward: we need an
AsyncIoRoot per thread to resolve promises we get from Lix. The `aio`
object is created per thread and passed as lvalue ref instead of making
it a field of the State struct since that struct is shared between the
threads.

The biggest change affects the NAR extractor: parseAndCopyDump has been
removed, instead the `nar::parse` and `nar::dump` parts are public now.
This won't work with the original parse visitor anymore, so instead the
NAR is parsed once and dumped again to copy it to the binary cache. In
between, the generator is walked through to search for relevant files
(such as build products) and after that the generators are recreated for
`AsyncGeneratorInputStream` to read again.
pennae left a comment
Owner

haven't checked whether aio roots are indeed unique per thread, but kj has runtime checks for that

haven't checked whether aio roots are indeed unique per thread, but kj has runtime checks for that
@ -397,6 +399,7 @@ void State::abortUnsupported()
bool stepFinished = false;
failStep(
aio,
Owner

weird indentation

weird indentation
ma27 marked this conversation as resolved
@ -41,1 +38,3 @@
std::unordered_set<Path> filesToKeep {
static Generator<nar::Entry> discoverHydraDataInNAR(
NarMemberDatas &members,
const Path &&prefix,
Owner

rule of thumb: don't pass rvalue references into coroutines. in this case you can get away with it because generator semantics are in your favor, but in general it'll cause dangling references and the most undebuggable segfaults of that day. (just remove the && in coroutine parameters and it'll be fine)

rule of thumb: don't pass rvalue references into coroutines. in this case you can get away with it because generator semantics are in your favor, but in general it'll cause dangling references and the most undebuggable segfaults of that day. (just remove the `&&` in coroutine parameters and it'll be fine)
ma27 marked this conversation as resolved
@ -47,2 +50,2 @@
NarMemberDatas & members;
Path prefix;
nix::overloaded handlers {
[&](nar::File f) -> Generator<nar::Entry> {
Owner

none of these have to be generators of their own, they can return a nar::Entry alone

none of these have to be generators of their own, they can return a `nar::Entry` alone
ma27 marked this conversation as resolved
@ -75,0 +94,4 @@
std::cref(e->first),
*discoverHydraDataInNAR(
members,
std::move(prefix),
Owner

that's only valid if directories never have more than one entry, otherwise all but the first item of a directory will see prefix == "".

(just don't move anything that doesn't absolutely need moved, it'll make your life a lot easier)

that's only valid if directories never have more than one entry, otherwise all but the first item of a directory will see `prefix == ""`. (just don't move anything that doesn't absolutely *need* moved, it'll make your life a lot easier)
ma27 marked this conversation as resolved
@ -96,0 +127,4 @@
auto items = nar::parse(source);
co_yield nar::dump(*discoverHydraDataInNAR(
members,
std::move(prefix),
Owner

this does not do what you except because prefix is a const-ref. instead it'll copy prefix into a temporary and then pass the reference to that temporary to discoverHydraDataInNAR

this does not do what you except because `prefix` is a const-ref. instead it'll *copy* `prefix` into a temporary and then pass the *reference* to that temporary to `discoverHydraDataInNAR`
ma27 marked this conversation as resolved
@ -96,0 +129,4 @@
members,
std::move(prefix),
"",
std::move(*items.next())
Owner

the move is unnecessary, and calling next on a nar parser only once is not safe. you must always drain the parser fully

the move is unnecessary, and calling `next` on a nar parser only once is not safe. you must always drain the parser fully
ma27 marked this conversation as resolved
@ -96,0 +130,4 @@
std::move(prefix),
"",
std::move(*items.next())
).next());
Owner

same here because the wrapped parser is still a parser

same here because the wrapped parser is still a parser
ma27 marked this conversation as resolved
ma27 force-pushed flake-update from 93bf5c6175 to 5dca3d8cd5 2025-03-05 21:04:13 +00:00 Compare
requested review from pennae 2025-03-12 09:02:13 +00:00
pennae approved these changes 2025-03-12 12:03:43 +00:00
pennae left a comment
Owner

looks like it should be good

looks like it should be good
@ -49,0 +75,4 @@
members.insert_or_assign(
prefix + pathSuffix,
NarMemberData{.type = FSAccessor::Type::tSymlink});
return std::move(sl);
Owner
return sl;

NRVO can skip the move altogether

``` return sl; ``` NRVO can skip the move altogether
ma27 marked this conversation as resolved
ma27 force-pushed flake-update from 5dca3d8cd5 to 48168af261 2025-03-12 12:28:34 +00:00 Compare
ma27 merged commit 4b826e2255 into main 2025-03-12 12:35:11 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: lix-project/hydra#25
No description provided.