nixos/tests: respect meta.hydraPlatforms#503721
Conversation
f21ada2 to
56521c1
Compare
jfly
left a comment
There was a problem hiding this comment.
A couple of comments I trust you to resolve without another review from me.
56521c1 to
d4b38bb
Compare
E.g. nspawn tests are not supported on the official NixOS infrastructure currently.
d4b38bb to
ec2c751
Compare
|
Done! |
| in | ||
| lib.optionalAttrs (builtins.elem system (getPlatforms driver)) ( | ||
| if attrNamesOnly then | ||
| hydraJob driver |
There was a problem hiding this comment.
This is a material change. Previously we were passing config.test. Was this intentional?
There was a problem hiding this comment.
Yes, the attribute is supposed to contain all drivers here.
I split this into its own commit such that it's clear when looking at the history or bisecting.
There was a problem hiding this comment.
I appreciate the separate commit, but I'm still confused about if this is a correct change. I just don't have the context, sorry.
There was a problem hiding this comment.
My reasoning is fairly simple: this generates the jobs inside a allDrivers attribute and the if/else only decides whether an attribute for system is added. To me it seems very wrong that this also switches the job from a test-driver to a test.
cc @wolfgangwalther who originally added this.
There was a problem hiding this comment.
Looks like that was a copy&paste mistake in my commit e3f583c.
There was a problem hiding this comment.
callTest needs to be burned down, and be replaced by a simpler solution. (which will in turn be easier to do after #386873)
I've worked on stuff involving callTest before and I consistently fail to reason about the multiple ways in which it is called, in which it is defined, and how those all interact.
I would review this, but it'd be a worthless review, or it'd be a research project.
I suggest throwing the spaghetti at the wall with a lot of force for the time being.
There was a problem hiding this comment.
I'm interpreting this as a decline to review (which is OK - don't get me wrong!) and not as blocking this patch: while I understand the argument of prefering a totally different solution, this approach solves a real problem of a test-driver option being flat-out ignored when we actually need this option now after the introduction of the nspawn backend.
There was a problem hiding this comment.
My ranting was meant to be informative. I fully support proceeding with this.
jfly
left a comment
There was a problem hiding this comment.
Lgtm, modulo my lack of understanding about the "always add config.driver to allDrivers" commit.
wolfgangwalther
left a comment
There was a problem hiding this comment.
Only looked at the first commit.
Motivation is to not build nspawn tests on hydra.nixos.org for the time being.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.