NixOS / cabal2nix

Generate Nix build instructions from a Cabal file
https://haskell4nix.readthedocs.io
Other
355 stars 154 forks source link

Explicitly list testTargets #617

Open TeofilC opened 5 months ago

TeofilC commented 5 months ago

testTarget is a space separated list of test-suites.

We explicitly list all test suites. This allows us to implement logic in the nixpkgs builder to run test suites in separate passthru.test derivations. See: https://github.com/NixOS/nixpkgs/pull/305958

Additionally it makes it easier to filter out test suites we don't want.

I've chosen to avoid changing the type of testTarget from a String to a [String] in order to avoid a breaking change. I think this would be a good change to make but I'm not sure how it would work in practice? Maybe we could add a new testTargets field in the generic builder and use that to transition?

sternenseemann commented 5 months ago

If we're going to do this, I'd add testTargets to the nixpkgs builder and some extra logic to keep backwards compatibility with the old interface. I've been meaning to do this anyways.

W.r.t. this change, I'm not sure if this is the best course of action yet. It seems good in principle, however it is going to massively increase the size of hackage-packages.nix which is a bit of a problem.

Skipping tests could in principle be implemented in the builder without having data about the names of all test suites at eval time, so I'm curious about the second use case you mention. Can you maybe share the code for a proof of concept for that?

maralorn commented 5 months ago

If we're going to do this, I'd add testTargets to the nixpkgs builder and some extra logic to keep backwards compatibility with the old interface. I've been meaning to do this anyways.

W.r.t. this change, I'm not sure if this is the best course of action yet. It seems good in principle, however it is going to massively increase the size of hackage-packages.nix which is a bit of a problem.

Skipping tests could in principle be implemented in the builder without having data about the names of all test suites at eval time, so I'm curious about the second use case you mention. Can you maybe share the code for a proof of concept for that?

I share the concern that hard coding the list of tests makes the derivations bigger and less flexible. (e.g. sometimes it is useful to use the same derivation with a different source, the more specific we make the derivation the less flexible it will be.)

Maybe we can introduce a list of disabled tests in the builder where we query cabal for the list of available tests and filter out disabled ones at build time and not at eval time?

TeofilC commented 5 months ago

Thanks for taking a look folks!

If we're going to do this, I'd add testTargets to the nixpkgs builder and some extra logic to keep backwards compatibility with the old interface. I've been meaning to do this anyways.

Cool I might make an MR to do this refactoring.

I share the concern that hard coding the list of tests makes the derivations bigger and less flexible

This is a very good point! The disabled test stuff is just a nice extra. My main aim with this is to enable passthru.tests style test suites. Take a look at https://github.com/NixOS/nixpkgs/pull/305958. I think that, in its current form, this requires us to have the list of test suites at eval time. That's because it creates a passthru.tests._ derivation for each test suite.

I think there's two ways to proceed: 1) put this behind a flag, so nixpkgs isn't burdened by this but end-users can enable it. 2) Change the nixpkgs PR so that it creates just 1 derivation for ALL the test suites, so we don't require knowing at eval time the names of the test suites. 3) Only emit this if there's more than 1 test suite. I feel like most packages just have 1, so this might be a nice compromise to avoid exploding the size while still having separate derivations for each?

I think we could also mix (1) and (2), where if testTargets is empty we just create one derivation or something like that.

What do you all think?