Split the buildRemote function#1180
Conversation
thufschmitt
force-pushed
the
split-buildRemote
branch
from
5153735 to
5a7d176
Compare
|
Need to read not on my phone, but I like this incrementalism. Good first steps before other cleanups! |
There was a problem hiding this comment.
Looks good, though I am reminded of this post by John Carmack on splitting big functions: http://number-none.com/blow/john_carmack_on_inlined_code.html. I.e. having a lot of small functions obfuscates the control flow.
|
@edolstra I think it's instructive per the top of that linked page that John Carmack eventually changed his mind :). These functions are are basically the ones to dedup with Nix per the very cautious incremental plan in #1164 ensuring we have no regressions, before finally getting Hydra to use the Store abstraction properly as the last step. I therefore like this a lot. |
thufschmitt
force-pushed
the
split-buildRemote
branch
from
ee2b9cc to
6689816
Compare
Thanks for the reference, that’s indeed a very valid point (and a very interesting read :) ) I think we’re fine(ish) here, because most of these functions are conceptually pure (for a very broad definition of pure), or at least idempotent. |
|
Mh, just noticed an issue with this, we’re sending the original derivation to the remote builders and not the basic one. Will fix asap |
OK, fixed in c592293 |
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
thufschmitt
force-pushed
the
split-buildRemote
branch
from
c592293 to
6e571e2
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/tweag-nix-dev-update-27/18433/1 |
|
I would still like to do this. @edolstra did start a more aggressive rewrite to go directly to using the store abstraction (#1200). But that PR is still draft, and to me that adds evidence to my theory that this approach (there is no disagreement on the end goal!) is better:
We are all pressed for time, and no one wants to break Hydra / hydra.nixos,org, so I think this is the prudent way forward. Please, let's do this. |
dasJ
left a comment
dasJ
left a comment
There was a problem hiding this comment.
The change looks good itself apart from the naming issue and the conflict that arose
| return {std::move(logFile), std::move(logFD)}; | ||
| } | ||
|
|
||
| void handshake(Machine::Connection & conn, unsigned int repeats) |
There was a problem hiding this comment.
I think it would either be better to rename the function (nixServeV1Handshake or something like that) or move it into a namespace/class to make its use more obvious
There was a problem hiding this comment.
Yup' fair point. I've moved all that new logic to its own namespace to make things clearer
| throw Error("machine ‘%1%’ does not support repeating a build; please upgrade it to Nix 1.12", conn.machine->sshName); | ||
| } | ||
|
|
||
| BasicDerivation sendInputs( |
Just don't pollute the global one
|
I'm not super in favor of this PR, since it is just a refactoring but it will make #1200 harder to merge. |
|
master...obsidiansystems:hydra:split-buildRemote I fixed merge conflicts in this one here, but I could not yet due to limited internet. |
|
@dasJ Is the new namespacing sufficient to you to clarify the identifiers? |
|
Yeah I'm happy with it. Could you resolve the conflict? I will pick this PR into my downstream Hydra then and run it for a couple of days to make sure nothing odd happens (although I don't expect a lot of impact) |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/content-addressed-nix-call-for-testers/12881/220 |
| auto narSize = readLongLong(conn.from); | ||
| auto narHash = Hash::parseAny(readString(conn.from), htSHA256); | ||
| auto ca = parseContentAddressOpt(readString(conn.from)); | ||
| readStrings<StringSet>(conn.from); // sigs |
There was a problem hiding this comment.
is the result of sigs discarded intentionally?
There was a problem hiding this comment.
It was already done that way. After we upgrade to Nix 2.19 we can deduplicate this code better.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/content-addressed-nix-call-for-testers/12881/221 |
6 participants
In an attempt to understand everything that’s going on here − and to make it easier to eventually simplify it − extract a handful of auxiliary functions from the
State::buildRemotemethod.I (hopefully) didn’t substantially change any logic yet, just moved things out of this method, so the behavior should remain exactly the same.