Add flag to build package passthru.tests

[B4dM4n]

This PR adds a --tests flag to build passthru.tests for all packages, as discussed in #77.

The tests to be build can be filtered with -p, -P and similar, like all other packages.

$ nixpkgs-review pr --tests 303033
...
Link to currently reviewing PR:
https://github.com/NixOS/nixpkgs/pull/303033

1 tests built:
netbird.passthru.tests.netbird

2 packages built:
netbird netbird-ui
...

$ nixpkgs-review pr --tests --package-regex 'netbird.+' 303033
...
1 tests built:
netbird.passthru.tests.netbird

1 package built:
netbird-ui
...

Since they are classified as tests, they can also be enabled individually with -p, without specifying --tests:

$ nixpkgs-review pr -p netbird.passthru.tests.netbird 303033
...
1 tests built:
netbird.passthru.tests.netbird
...

Fixes: #77

[Mic92]

Sorry for the late reply. I think because of the potential large memory consumption of nixos tests, we need to evaluate them isolation as they will otherwise make the build potentially go out of memory, i.e. if you try to have a large package set rebuild.
Therefore I would suggest to evaluate those packages ahead of time i.e. using nix-instantiate and add them to the final nix build. Of course a better design would be some sort of pipe-lining where already evaluated packages gets build as the rest of the evaluation is still in progress. But this probably requires bigger refactorings and could implemented at a later point similar to nix-fast-build. When using nix-instantiate also add a gcroot with --add-root to make sure that derivations are not garbage collected before hand.

[lucasew]

Maybe at first we could do this way but warn about this possible problem and then address it in a follow up PR

[Mic92]

I don't think it's a good idea to just merge it like that as going out of memory can also causes the users system to crash in some cases i.e. crashing their wayland compositor.

[Eveeifyeve]

Sorry for the late reply. I think because of the potential large memory consumption of nixos tests, we need to evaluate them isolation as they will otherwise make the build potentially go out of memory, i.e. if you try to have a large package set rebuild. Therefore I would suggest to evaluate those packages ahead of time i.e. using nix-instantiate and add them to the final nix build. Of course a better design would be some sort of pipe-lining where already evaluated packages gets build as the rest of the evaluation is still in progress. But this probably requires bigger refactorings and could implemented at a later point similar to nix-fast-build. When using nix-instantiate also add a gcroot with --add-root to make sure that derivations are not garbage collected before hand.

Maybe at first we could do this way but warn about this possible problem and then address it in a follow up PR

I don't think it's a good idea to just merge it like that as going out of memory can also causes the users system to crash in some cases i.e. crashing their wayland compositor.

I 100% agree with this because if someone for example goes to review using nixpkgs-review and then go wtf happened to my wayland compositor they will think it's their compositor being shit so they will try to switch and it could still cause issues then nixpkgs-review will have to be fixed in a timely manner.

So I would suggest doing what mic92 has suggested, I personally will be putting a block on this pr until this is solved.

[Eveeifyeve]

As indicated in suggestion comment, please fix the memory corruption bugs via mic92 suggestion and additionally solve the conflicts.

[corngood]

There are some very memory hungry normal packages you can accidentally hit in a review run and we don't insist on nixpkgs-review having a special job scheduler to handle that well.

I agree with this. I've been locally running something similar to this PR for quite a while, and I've had more problems with packages than with tests. I just use -j2 for everything.

[B4dM4n]

Personally, I never had any problems with memory consumption while evaluating packages/tests, because I only run nixpkgs-review on machines which can do a local package evaluation via nix-env -qaP, which itself currently requires around 22GB of memory. So everything that consumes less just works fine.

But I get that most people probably only review PR's with ofborg evaluation, so the memory consumption in this case is (usually) much lower.

When performing local evaluation, I currently observe the following memory consumption:

• 22GB per nix-env -qaP invocation
• 4GB usage by the nixpkgs-review python process after the nix-env -qaP runs. Not sure what is causing that, maybe the produced XML output is kept in memory?

Afterwards the numbers heavily depend on the number of changed packages:

• nix eval potential unbounded memory usage (which could rapidly grow when evaluating tests)
• nix build potentially also has unbounded memory usage, because it's evaluating the same attributes again

To make the memory consumption a bit more manageable, I tried to implement local valuation with nix-eval-jobs in #473, which comes with its own issues regarding speed/performance, so it's probably not happening soon (nix-eval-jobs would need a "lazy eval" mode like nix-env -qaP has, to be viable).

Based on my experience from that PR, I reworked this PR to only use nix-eval-jobs to evaluate the changed packages and tests with a configurable memory "limit":

master...B4dM4n:nixpkgs-review:build-passthru-tests-v2

It also removes the potential huge and permanent memory consumption of nix build, by passing it the wanted derivations directly, instead of evaluating the attributes again. There are currently no gcroots created for the derivations, but I will add that soon.

I could also try to rewrite the current PR, to invoke one nix-instantiate command per test attribute to limit the memory usage, but this seems rather wasteful.

Which route would you prefer?

[Eveeifyeve]

Personally, I never had any problems with memory consumption while evaluating packages/tests, because I only run nixpkgs-review on machines which can do a local package evaluation via nix-env -qaP, which itself currently requires around 22GB of memory. So everything that consumes less just works fine.

But I get that most people probably only review PR's with ofborg evaluation, so the memory consumption in this case is (usually) much lower.

When performing local evaluation, I currently observe the following memory consumption:

* 22GB per `nix-env -qaP` invocation

* 4GB usage by the `nixpkgs-review` python process after the `nix-env -qaP` runs. Not sure what is causing that, maybe the produced XML output is kept in memory?

Afterwards the numbers heavily depend on the number of changed packages:

* `nix eval` potential unbounded memory usage (which could rapidly grow when evaluating tests)

* `nix build` potentially also has unbounded memory usage, because it's evaluating the same attributes again

To make the memory consumption a bit more manageable, I tried to implement local valuation with nix-eval-jobs in #473, which comes with its own issues regarding speed/performance, so it's probably not happening soon (nix-eval-jobs would need a "lazy eval" mode like nix-env -qaP has, to be viable).

Based on my experience from that PR, I reworked this PR to only use nix-eval-jobs to evaluate the changed packages and tests with a configurable memory "limit":

master...B4dM4n:nixpkgs-review:build-passthru-tests-v2

It also removes the potential huge and permanent memory consumption of nix build, by passing it the wanted derivations directly, instead of evaluating the attributes again. There are currently no gcroots created for the derivations, but I will add that soon.

I could also try to rewrite the current PR, to invoke one nix-instantiate command per test attribute to limit the memory usage, but this seems rather wasteful.

Which route would you prefer?

nix-instantiate then nix-build because you can have large evals that chug a lot of memory and it prepares the build so build doesn't have to do eval then build which uses double of the memory than it can just build and worry's about the memory building the package.

[B4dM4n]

nix-instantiate then nix-build because you can have large evals that chug a lot of memory and it prepares the build so build doesn't have to do eval then build which uses double of the memory than it can just build and worry's about the memory building the package.

This is exactly what the new branch does. nix-eval-jobs implicitly instantiates all derivations and the resulting derivation paths are fed directly into into nix build, so it doesn't have to evaluate them again.

nix-eval-jobs is an optimized way to do run nix-instantiate for each attribute separately. It can perform multiple instantiations in the same process, but creates a new process when a given memory limit is exceeded. And it can spawn multiple workers processes to run instantiations in parallel, speeding up the process.

[Eveeifyeve]

nix-instantiate then nix-build because you can have large evals that chug a lot of memory and it prepares the build so build doesn't have to do eval then build which uses double of the memory than it can just build and worry's about the memory building the package.

This is exactly what the new branch does. nix-eval-jobs implicitly instantiates all derivations and the resulting derivation paths are fed directly into into nix build, so it doesn't have to evaluate them again.

nix-eval-jobs is an optimized way to do run nix-instantiate for each attribute separately. It can perform multiple instantiations in the same process, but creates a new process when a given memory limit is exceeded. And it can spawn multiple workers processes to run instantiations in parallel, speeding up the process.

Yes, you could use nix-eval-jobs which is lovely that you have created pr #473, however if we were to switch your pr will blocking this pr to merge and as said by @Mic92 #397 (comment) at a later point these implementations can be implemented.

[B4dM4n]

There is nothing blocking

master...B4dM4n:nixpkgs-review:build-passthru-tests-v2

from merging. It's a rewrite of this PR, implementing the requested memory limit. It has nothing todo with #473 and is totally independent.

It's just using nix-eval-jobs to parallelize instantiation (optional, default is 1 worker), while also providing the option to limit memory consumption (defaults to 4GB, which is also the nix-eval-jobs default).