From 86cd5e907683a8c4b0f6cd75d4879648d8313b98 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 20 Feb 2022 20:06:22 +0000 Subject: [PATCH 1/4] `copyClosureTo`: Use `SubstituteFlag` instead of `bool` This matches Nix (in the same serialization logic in `src/libstore/legacy-ssh-store.cc`) and adds clarity. --- src/hydra-queue-runner/build-remote.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hydra-queue-runner/build-remote.cc b/src/hydra-queue-runner/build-remote.cc index 73f46a53..f41585c8 100644 --- a/src/hydra-queue-runner/build-remote.cc +++ b/src/hydra-queue-runner/build-remote.cc @@ -113,7 +113,7 @@ static void copyClosureTo( Machine::Connection & conn, Store & destStore, const StorePathSet & paths, - bool useSubstitutes = false) + SubstituteFlag useSubstitutes = NoSubstitute) { StorePathSet closure; destStore.computeFSClosure(paths, closure); @@ -266,7 +266,7 @@ static BasicDerivation sendInputs( destStore.computeFSClosure(basicDrv.inputSrcs, closure); copyPaths(destStore, localStore, closure, NoRepair, NoCheckSigs, NoSubstitute); } else { - copyClosureTo(conn, destStore, basicDrv.inputSrcs, true); + copyClosureTo(conn, destStore, basicDrv.inputSrcs, Substitute); } auto now2 = std::chrono::steady_clock::now(); From 6a54ab24e26b4e387c41e6d8958e7a9ec36c8398 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 7 Dec 2023 02:00:22 -0500 Subject: [PATCH 2/4] Use factored-out `BuildResult` serializer For the record, here is the Nix 2.19 version: https://github.com/NixOS/nix/blob/2.19-maintenance/src/libstore/serve-protocol.cc, which is what we would initially use. It is a more complete version of what Hydra has today except for one thing: it always unconditionally sets the start/stop times. I think that is correct at the other end seems to unconditionally measure them, but just to be extra careful, I reproduced the old behavior of falling back on Hydra's own measurements if `startTime` is 0. The only difference is that the fallback `stopTime` is now measured from after the entire `BuildResult` is transferred over the wire, but I think that should be negligible if it is measurable at all. (And remember, this is fallback case I already suspect is dead code.) --- src/hydra-queue-runner/build-remote.cc | 36 ++++++++++---------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/src/hydra-queue-runner/build-remote.cc b/src/hydra-queue-runner/build-remote.cc index 73f46a53..5206ae5a 100644 --- a/src/hydra-queue-runner/build-remote.cc +++ b/src/hydra-queue-runner/build-remote.cc @@ -286,9 +286,6 @@ static BuildResult performBuild( counter & nrStepsBuilding ) { - - BuildResult result; - conn.to << ServeProto::Command::BuildDerivation << localStore.printStorePath(drvPath); writeDerivation(conn.to, localStore, drv); conn.to << options.maxSilentTime << options.buildTimeout; @@ -301,30 +298,25 @@ static BuildResult performBuild( } conn.to.flush(); - result.startTime = time(0); + BuildResult result; + time_t startTime, stopTime; + + startTime = time(0); { MaintainCount mc(nrStepsBuilding); - result.status = (BuildResult::Status)readInt(conn.from); + result = ServeProto::Serialise::read(localStore, conn); } - result.stopTime = time(0); + stopTime = time(0); - - result.errorMsg = readString(conn.from); - if (GET_PROTOCOL_MINOR(conn.remoteVersion) >= 3) { - result.timesBuilt = readInt(conn.from); - result.isNonDeterministic = readInt(conn.from); - auto start = readInt(conn.from); - auto stop = readInt(conn.from); - if (start && start) { - /* Note: this represents the duration of a single - round, rather than all rounds. */ - result.startTime = start; - result.stopTime = stop; - } - } - if (GET_PROTOCOL_MINOR(conn.remoteVersion) >= 6) { - ServeProto::Serialise::read(localStore, conn); + if (!result.startTime) { + // If the builder gave `startTime = 0`, use our measurements + // instead of the builder's. + // + // Note: this represents the duration of a single round, rather + // than all rounds. + result.startTime = startTime; + result.stopTime = stopTime; } return result; From a45a27851b6cfc22113a660df75e556199c20c84 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 7 Dec 2023 12:20:55 -0500 Subject: [PATCH 3/4] flake.lock: Update Nixpkgs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will be required for upgrading Nix beyond 2.19. Flake lock file updates: • Updated input 'nixpkgs': 'github:NixOS/nixpkgs/ef0bc3976340dab9a4e087a0bcff661a8b2e87f3' (2023-06-21) → 'github:NixOS/nixpkgs/e9f06adb793d1cca5384907b3b8a4071d5d7cb19' (2023-12-03) --- flake.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/flake.lock b/flake.lock index 9a9046c8..2871e70a 100644 --- a/flake.lock +++ b/flake.lock @@ -58,11 +58,11 @@ }, "nixpkgs": { "locked": { - "lastModified": 1687379288, - "narHash": "sha256-cSuwfiqYfeVyqzCRkU9AvLTysmEuSal8nh6CYr+xWog=", + "lastModified": 1701615100, + "narHash": "sha256-7VI84NGBvlCTduw2aHLVB62NvCiZUlALLqBe5v684Aw=", "owner": "NixOS", "repo": "nixpkgs", - "rev": "ef0bc3976340dab9a4e087a0bcff661a8b2e87f3", + "rev": "e9f06adb793d1cca5384907b3b8a4071d5d7cb19", "type": "github" }, "original": { From 411e4d0c2458ec5319b8ea88129dff807793f7d7 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 8 Dec 2023 11:30:31 -0500 Subject: [PATCH 4/4] Let tests themselves intentionally leak temp dir (#1320) * Let tests themselves intentionally leak temp dir By default Yath will clean up temporary files, so the result is the same. But `--keep-dirs` can be passed to `yath test` telling Yath to *not* clean them up instead. This is very useful for debugging. * Update t/lib/HydraTestContext.pm Co-authored-by: Cole Helbling --- t/lib/HydraTestContext.pm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/t/lib/HydraTestContext.pm b/t/lib/HydraTestContext.pm index 53eaa0f7..a22c3df1 100644 --- a/t/lib/HydraTestContext.pm +++ b/t/lib/HydraTestContext.pm @@ -39,7 +39,9 @@ use Hydra::Helper::Exec; sub new { my ($class, %opts) = @_; - my $dir = File::Temp->newdir(); + # Cleanup will be managed by yath. By the default it will be cleaned + # up, but can be kept to aid in debugging test failures. + my $dir = File::Temp->newdir(CLEANUP => 0); $ENV{'HYDRA_DATA'} = "$dir/hydra-data"; mkdir $ENV{'HYDRA_DATA'};