Use `nix-eval-jobs` and delete `hydra-eval-jobs` #1421

Ericson2314 · 2024-11-13T21:50:02Z

nix-eval-jobs was forked from hydra-eval-jobs originally, for others to use. That's great! But maintaining two copies is no good. But we use that.

Original author is @delroth, for Lix's Hydra, thank you!

Draft because some tests fail. I am not sure why. Maybe there are fixes on the Lix branch that can be cherry-picked to fix? Not sure!

zowoq · 2024-11-13T23:06:59Z

src/script/hydra-eval-jobset

-                "--flake", $flakeRef,
-                "--gc-roots-dir", getGCRootsDir,
-                "--max-jobs", 1);
+        @cmd = ("nix-eval-jobs", "--flake", $flakeRef . '#hydraJobs');


Uses hydraJobs here and the original hydra-eval-jobs has hydraJobs and checks:

hydra/src/hydra-eval-jobs/hydra-eval-jobs.cc

Lines 148 to 152 in f974891

auto aHydraJobs = vOutputs->attrs()->get(state.symbols.create("hydraJobs"));

if (!aHydraJobs)

aHydraJobs = vOutputs->attrs()->get(state.symbols.create("checks"));

if (!aHydraJobs)

throw Error("flake '%s' does not provide any Hydra jobs or checks", flakeRef);

Ah found more commits that are relevant.

The Lix commits don't address this issue, they only have support for hydraJobs.

I think it would be important to keep the checks supports

@Mic92 Do you have a preference for how we do this?

Could we add an nix eval before that to figure out which one exists?

nix eval --raw --impure --expr "let f = builtins.getFlake \"$flakeRef\"; in if f ? hydraJobs then \"hydraJobs\" else \"checks\""

Alternative is fixing: nix-community/nix-eval-jobs#332
Than we can just pass in what we want.

OK added a commit that ought to do this, but we should have some tests.

dasJ · 2024-11-14T10:51:18Z

A relevant thing to consider here is whether we want an official NixOS project to be dependant on a Nix Community project. As of now (afaik), the NixOS GitHub namespace is mostly self-contained. I see these solutions:

Close this PR
Migrate nix-eval-jobs to the NixOS namespace (or even nix repo?)
Just live with the fact

I personally prefer the latter two options over the first one since it would get rid of code duplication and remove a component from Hydra that only a few people are proficient in and that gets a lot less maintenance than nix-eval-jobs.

Maybe that's a question for the new SC?

Mic92 · 2024-11-14T11:10:05Z

Happy to move the project to the NixOS org if I can keep maintaining it there. We would loose aarch64-linux builds because those runners are currently sponsored by namespace in nix-community. I can attach my buildbot-nix installation to this org to compensate.

Mic92 · 2024-11-14T11:17:36Z

Maybe that's a question for the new SC?

That's more a technical question for the Hydra/Nix team, unless we feel to overwhelmed what to do and need to escalate.

Ericson2314 · 2024-11-15T16:17:43Z

OK undrafting because we're back on nix-eval-jobs main.

@dasJ I am fine resolving the nix-community thing later, because Hydra is a funny project --- it's official because history and because hydra.nixos.org, not because it is up to the quality we generally expect for 3rd-party use.

Mindavi

The perl bits look generally ok to me but not my expertise. Maybe put somewhere a bit clearer that the constituents are not supported anymore. That's not used on hydra.nixos.org, right?

src/script/hydra-eval-jobset

dasJ

As stated before, I do find this move to be a good idea. Here are some things I noticed while reading the code

flake.nix

dasJ · 2024-11-16T20:56:20Z

src/script/hydra-eval-jobset

-                "--flake", $flakeRef,
-                "--gc-roots-dir", getGCRootsDir,
-                "--max-jobs", 1);
+        @cmd = ("nix-eval-jobs", "--flake", $flakeRef . '#hydraJobs');


I think it would be important to keep the checks supports

src/script/hydra-eval-jobset

dasJ · 2024-11-16T21:03:40Z

src/script/hydra-eval-jobset

@@ -709,36 +744,20 @@ sub checkJobsetWrapped {
        return;
    }

-    die "Jobset contains a job with an empty name. Make sure the jobset evaluates to an attrset of jobs.\n"


Is this a relevant check? Kind of surprising to see it gone

@Ma27 Do you know about this? Maybe this is something that should be restored in the new streaming version? Or maybe its fine?

If I had to guess, this probably just got removed since $jobs got replaced by an iterator?
But yeah, seems reasonalbe to add it back.

src/script/hydra-eval-jobset

dasJ · 2024-11-17T08:17:45Z

flake.nix

@@ -8,14 +8,20 @@
  inputs.nix.inputs.nixpkgs.follows = "nixpkgs";
  inputs.nix.inputs.libgit2.follows = "libgit2";

+  inputs.nix-eval-jobs.url = "github:nix-community/nix-eval-jobs";
+  inputs.nix-eval-jobs.inputs.nixpkgs.follows = "nixpkgs";


Something I forgot in the last review: I think we should also force the nix version of hydra into the package, otherwise we might depend on 2 nix versions

That's actually a good one and could be a problem if hydra or nix-eval-jobs doesn't keep up with nix releases.

I can maintain release branches for different nix versions again if this is needed.

I don't think the literal Nix derivation is forced to be the same, but the version is the same now.

(cherry picked from commit 684cc50)

Ericson2314 · 2024-12-10T15:35:55Z

OK thanks to the fix in nix-eval-jobs, this works now!

incrementally ingest eval results nix-eval-jobs streams output, unlike hydra-eval-jobs. Now that we've migrated, we can use this to: 1. Use less RAM by avoiding buffering a whole eval's worth of metadata into a Perl string and an array of JSON objects. 2. Make evals latency a bit lower by allowing the queue runner to start ingesting builds faster. Also use the newly-restored constituents support in `nix-eval-jobs` Note, we pass --workers and --max-memory-size to n-e-j Lost in the h-e-j -> n-e-j migration, causing evaluation to always be single threaded and limited to 4GiB RAM. Follow the config settings like h-e-j used to do (via C++ code). (cherry picked from commit 6d4ccff) (cherry picked from commit b0e9b4b) (cherry picked from commit cdfc5c81e8037d3e4818a3e459d0804b2c157ea9) (cherry picked from commit 4b107e6) Co-Authored-By: Maximilian Bosch <[email protected]>

(cherry picked from commit ed7c587)

Mic92 · 2024-12-11T06:42:38Z

src/script/hydra-eval-jobset

+            "or flake.checks" .
+            "or throw \"flake '$flakeRef' does not provide any Hydra jobs or checks\"";
+
+        @cmd = ("nix-eval-jobs", "--flake", "--expr", $nix_expr);


Those are mutual exclusive afaik.

Suggested change

@cmd = ("nix-eval-jobs", "--flake", "--expr", $nix_expr);

@cmd = ("nix-eval-jobs", "--expr", $nix_expr);

Mic92 · 2024-12-11T06:44:04Z

src/script/hydra-eval-jobset

-                "--max-jobs", 1);
+        my $nix_expr =
+            "let " .
+            "flake = builtins.getFlake (toString \"$flakeRef\"); " .


Afaik builtins.getFlake will be impure doing this. Also we would need to think about escaping "$flakeRef" than more.
There is a work around... https://github.com/nix-community/nixos-anywhere/blob/9ba099b2ead073e0801b863c880be03a981f2dd1/terraform/nix-build/nix-build.sh#L38

nixos-discourse · 2024-12-11T21:21:37Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-12-09-nix-team-meeting-minutes-201/57280/1

nixos-discourse · 2024-12-11T21:22:39Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-12-13-nix-team-meeting-minutes-202/57281/1

Ericson2314 marked this pull request as draft November 13, 2024 21:50

Ericson2314 mentioned this pull request Nov 13, 2024

autotools -> meson #1422

Merged

zowoq reviewed Nov 13, 2024

View reviewed changes

Ericson2314 mentioned this pull request Nov 13, 2024

improve compatibility if ca derivations are present nix-community/nix-eval-jobs#330

Merged

This comment was marked as resolved.

Sign in to view

Ericson2314 mentioned this pull request Nov 14, 2024

Ensure that we still create map kv pairs for floating CA outputs nix-community/nix-eval-jobs#336

Merged

Ericson2314 mentioned this pull request Nov 14, 2024

PackageInfo::queryOutputs should not trip up on placeholders NixOS/nix#11885

Open

Ericson2314 force-pushed the nix-eval-jobs branch from 498b283 to d0cfbed Compare November 14, 2024 18:21

This comment was marked as resolved.

Sign in to view

Ericson2314 mentioned this pull request Nov 14, 2024

Make fallback logic for CA derivations more flexible nix-community/nix-eval-jobs#337

Merged

Ericson2314 marked this pull request as ready for review November 14, 2024 18:28

Ericson2314 marked this pull request as draft November 14, 2024 18:28

This comment was marked as resolved.

Sign in to view

Ericson2314 marked this pull request as ready for review November 15, 2024 15:57

Ericson2314 force-pushed the nix-eval-jobs branch from d0cfbed to 8a49ecb Compare November 15, 2024 15:57

Mindavi approved these changes Nov 15, 2024

View reviewed changes

Mic92 reviewed Nov 16, 2024

View reviewed changes

src/script/hydra-eval-jobset Show resolved Hide resolved

dasJ requested changes Nov 16, 2024

View reviewed changes

dasJ reviewed Nov 17, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

Ericson2314 marked this pull request as draft November 17, 2024 21:24

Ericson2314 force-pushed the nix-eval-jobs branch 2 times, most recently from a3e3fe9 to d2227af Compare November 17, 2024 23:26

Ericson2314 force-pushed the nix-eval-jobs branch from d2227af to d97956d Compare November 25, 2024 16:08

This comment was marked as resolved.

Sign in to view

Ericson2314 assigned Ma27 Nov 29, 2024

This comment was marked as resolved.

Sign in to view

Ma27 removed their assignment Dec 9, 2024

flake: add nix-eval-jobs as input

11a6a34

(cherry picked from commit 684cc50)

Ericson2314 force-pushed the nix-eval-jobs branch from d97956d to cc5f805 Compare December 10, 2024 15:34

Ericson2314 marked this pull request as ready for review December 10, 2024 15:35

delroth and others added 2 commits December 10, 2024 11:19

hydra-eval-jobs: remove, replaced by nix-eval-jobs

890e693

(cherry picked from commit ed7c587)

Ericson2314 force-pushed the nix-eval-jobs branch from cc5f805 to 890e693 Compare December 10, 2024 16:20

Ericson2314 marked this pull request as draft December 10, 2024 16:20

Ericson2314 added 2 commits December 10, 2024 11:31

Render the nix-eval-jobs version too

68aadc4

nix-eval-jobs should check hydraJobs and then checks with flakes

5833e12

Mic92 reviewed Dec 11, 2024

View reviewed changes

	auto aHydraJobs = vOutputs->attrs()->get(state.symbols.create("hydraJobs"));
	if (!aHydraJobs)
	aHydraJobs = vOutputs->attrs()->get(state.symbols.create("checks"));
	if (!aHydraJobs)
	throw Error("flake '%s' does not provide any Hydra jobs or checks", flakeRef);

	@cmd = ("nix-eval-jobs", "--flake", "--expr", $nix_expr);
	@cmd = ("nix-eval-jobs", "--expr", $nix_expr);

Use nix-eval-jobs and delete hydra-eval-jobs #1421

Are you sure you want to change the base?

Use nix-eval-jobs and delete hydra-eval-jobs #1421

Conversation

Ericson2314 commented Nov 13, 2024

zowoq Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zowoq Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

dasJ commented Nov 14, 2024 • edited Loading

Mic92 commented Nov 14, 2024 • edited Loading

Mic92 commented Nov 14, 2024

This comment was marked as resolved.

This comment was marked as resolved.

Ericson2314 commented Nov 15, 2024

Mindavi left a comment

Choose a reason for hiding this comment

dasJ left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Ericson2314 commented Dec 10, 2024

Choose a reason for hiding this comment

Mic92 Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

nixos-discourse commented Dec 11, 2024

nixos-discourse commented Dec 11, 2024

Use `nix-eval-jobs` and delete `hydra-eval-jobs` #1421

Use `nix-eval-jobs` and delete `hydra-eval-jobs` #1421

zowoq Nov 13, 2024 •

edited

Loading

zowoq Nov 14, 2024 •

edited

Loading

Mic92 Dec 10, 2024 •

edited

Loading

dasJ commented Nov 14, 2024 •

edited

Loading

Mic92 commented Nov 14, 2024 •

edited

Loading

dasJ left a comment •

edited

Loading

Mic92 Dec 11, 2024 •

edited

Loading