lix-project/hydra
3
0
Fork 6
Code Issues 2 Pull requests Packages Projects Releases Wiki Activity

Update Lix to latest revision #8

Closed
ma27 wants to merge 5 commits from ma27:lix-update into main
pull from: ma27:lix-update
merge into: lix-project:main
Conversation 4 Commits 5 Files changed 4 +43 -40
ma27 commented 2024-09-26 13:03:44 +00:00
Member
Copy link

cc @delroth @leo60228

cc @jade who authored the related Lix change. Would be nice if you could double-check if I didn't mess anything up in e92ac734e6 :)

cc @delroth @leo60228 cc @jade who authored the related Lix change. Would be nice if you could double-check if I didn't mess anything up in e92ac734e6 :)
ma27 added 2 commits 2024-09-26 13:03:49 +00:00 
Flake lock file updates:

• Updated input 'lix':
    'git+https://git.lix.systems/lix-project/lix?ref=refs/heads/main&rev=02eb07cfd539c34c080cb1baf042e5e780c1fcc2' (2024-09-01)
  → 'git+https://git.lix.systems/lix-project/lix?ref=refs/heads/main&rev=31954b51367737defbae87fba053b068897416fb' (2024-09-26)
• Updated input 'nixpkgs':
    'github:NixOS/nixpkgs/6e99f2a27d600612004fbd2c3282d614bfee6421' (2024-08-30)
  → 'github:NixOS/nixpkgs/759537f06e6999e141588ff1c9be7f3a5c060106' (2024-09-25)
Since ca1dc3f70bf98e2424b7b2666ee2180675b67451, the NAR parser has moved
the preallocate & receive steps into the file handle class to remove the
assumption that only one file can be handled at a time.
jade reviewed 2024-09-26 17:03:23 +00:00
jade left a comment
Owner
Copy link

i mean there's really nothing wrong with this code but there's a couple things you could make a bit better

i mean there's really nothing wrong with this code but there's a couple things you could make a bit better
src/hydra-queue-runner/nar-extractor.cc
@ -12,0 +18,4 @@
public:
MyFileHandle(NarMemberData * memberData, uint64_t size) : memberData(memberData)
{
expectedSize = size;
jade commented 2024-09-26 16:58:24 +00:00
Owner
Copy link

should be in the initializer above rather than in the function itself

should be in the initializer above rather than in the function itself
ma27 marked this conversation as resolved
src/hydra-queue-runner/nar-extractor.cc
@ -12,0 +19,4 @@
MyFileHandle(NarMemberData * memberData, uint64_t size) : memberData(memberData)
{
expectedSize = size;
hashSink = std::make_unique<HashSink>(HashType::SHA256);
jade commented 2024-09-26 16:59:14 +00:00
Owner
Copy link

does this actually have to be behind a pointer? only reason i can see that it would is being big but i don't think it's big.

edit: oh, it might be the reset() maybe? so that it becomes null and crashes if you put extra data in it?

does this actually have to be behind a pointer? only reason i can see that it would is being big but i don't think it's big. edit: oh, it might be the reset() maybe? so that it becomes null and crashes if you put extra data in it?
ma27 commented 2024-10-06 17:45:13 +00:00
Author
Member
Copy link

I guess we can leave that as-is then, correct @jade?

I guess we can leave that as-is then, correct @jade?
jade commented 2024-10-06 19:03:12 +00:00
Owner
Copy link

yeah you can leave it alone, it's fine.

yeah you can leave it alone, it's fine.
jade marked this conversation as resolved
src/hydra-queue-runner/nar-extractor.cc Outdated
@ -32,2 +60,3 @@
std::unique_ptr<FileHandle> createRegularFile(const Path & path, uint64_t size, bool executable) override
{
curMember = &members.insert_or_assign(prefix + path, NarMemberData {
auto memberData = &members.insert_or_assign(prefix + path, NarMemberData {
jade commented 2024-09-26 17:01:51 +00:00
Owner
Copy link

probably should be auto * right?

or you could refactor it into being a reference now that the field doesn't need to be reassigned; either way.

probably should be `auto *` right? or you could refactor it into being a reference now that the field doesn't need to be reassigned; either way.
ma27 marked this conversation as resolved
ma27 commented 2024-09-28 10:48:29 +00:00
Author
Member
Copy link

i mean there's really nothing wrong with this code but there's a couple things you could make a bit better

To be clear, that's also helpful for me!

I mostly do C++ because I'm interested in hacking on Lix & Hydra, so getting input on how to do things better is very valuable to me. Thank you!

I'll try to get back to you this weekend for that.

> i mean there's really nothing wrong with this code but there's a couple things you could make a bit better To be clear, that's also helpful for me! I mostly do C++ because I'm interested in hacking on Lix & Hydra, so getting input on how to do things better is very valuable to me. Thank you! I'll try to get back to you this weekend for that.
ma27 added 1 commit 2024-10-06 17:44:46 +00:00 
* Turn `NarMemberData` in `MyFileHandle` into a reference.
* `expectedSize` now uses an initializer.
* Used `std::make_unique` instead of `std::unique_ptr(new ...)` as
  suggested by clangd.
jade commented 2024-10-06 19:05:59 +00:00
Owner
Copy link

(ah fyi the reason ours was using the constructor and new instead of make_unique is that it was a private constructor and so the template couldn't call it. not the case here; doesn't strictly need to be the case because it's unrealistic anyone would call that constructor when they shouldn't, so you can just leave it as-is)

(ah fyi the reason ours was using the constructor and new instead of make_unique is that it was a private constructor and so the template couldn't call it. not the case here; doesn't strictly need to be the case because it's unrealistic anyone would call that constructor when they shouldn't, so you can just leave it as-is)
👍 1
ma27 commented 2024-10-07 15:41:58 +00:00
Author
Member
Copy link

cc @vriska @yu-re-ka @raito

cc @vriska @yu-re-ka @raito
ma27 added 2 commits 2024-10-07 16:07:24 +00:00 
Flake lock file updates:

• Updated input 'lix':
    'git+https://git.lix.systems/lix-project/lix?ref=refs/heads/main&rev=31954b51367737defbae87fba053b068897416fb' (2024-09-26)
  → 'git+https://git.lix.systems/lix-project/lix?ref=refs/heads/main&rev=ed9b7f4f84fd60ad8618645cc1bae2d686ff0db6' (2024-10-05)
• Updated input 'nixpkgs':
    'github:NixOS/nixpkgs/759537f06e6999e141588ff1c9be7f3a5c060106' (2024-09-25)
  → 'github:NixOS/nixpkgs/ecbc1ca8ffd6aea8372ad16be9ebbb39889e55b6' (2024-10-06)
The Finally part is a destructor, so using `ignoreExceptionInDestructor`
seems to be the correct choice here.
jade approved these changes 2024-10-07 16:53:22 +00:00
yu-re-ka commented 2024-10-07 17:24:35 +00:00
Member
Copy link

Thanks, I pushed these changes to the main branch with some rearrangements of the commit (squashed the "address review-comments" into the previous commit etc)

Thanks, I pushed these changes to the main branch with some rearrangements of the commit (squashed the "address review-comments" into the previous commit etc)
yu-re-ka closed this pull request 2024-10-07 17:24:35 +00:00
ma27 deleted branch lix-update 2024-10-07 17:38:55 +00:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
jade
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 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#8
No description provided.
Delete branch "ma27:lix-update"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?