-
Notifications
You must be signed in to change notification settings - Fork 701
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
'cabal new-build test:test-suite' doesn't force tests to be enabled. #3923
Comments
Thanks. @grayjay, do you know if the solver will only disable the test if there is NO solution that works with them enabled? If so, I think it's just a matter of giving a better error when a test target is specified. If we do have to make the solver operate differently if a test is requested, we're in a bit more trouble, because then building a test target / not building a test target could lead to dep plan flip flopping. Not a good thing. Re priority, this bug doesn't seem particularly urgent? Just a bad error message in an unlikely scenario. |
I think there was a solution with tests enabled when I first found this bug. I don't remember what I did, though. I just tried modifying the test case above, but then I discovered another bug that can prevent the solver from disabling tests. I'll create an issue for that later. If testing is only a preference, the solver may disable tests when a solution with tests enabled exists. Do you think it would be better to enable them with a hard constraint in new-build? Then the user could specify |
I think that's OK, but there may be unintended consequences I can't see. |
Right, it's important that the target the user picks does not affect the solution. So we have two obvious UI choices:
I think the second option is better. Much of the time you don't need to build the tests/benchmarks, so you don't need to notice or care if they cannot be easily built. Given our current solver error messages we cannot produce a good error message that says that the user can try again with tests/benchmarks disabled (ie we cannot currently determine if the solver failure is due to the test suites or something else). What we're currently missing in option 2, is that we don't check and produce good error messages when a disabled testsuite/benchmark is requested, but this shouldn't be too hard to do. |
@dcoutts, how about the projecs. At work I quite often have local patches to Hackage packages, and then my private code. I do not want these additional modified Hackage packages tests/benchmarks to affect my private build plan, because at some point I'll use a Hackage version. In packages:
- "."
- location: github/
extra-dep: true As a consequence I.e. for my packages I build tests always, but for those Hackage dependencies not so often, and probably in isolation (something like |
@phadej I think if you add |
Right, either you can specify
for those local packages that you're patching, or perhaps alternatively we want to have control over which packages new-build considers to be "local" for the purposes of applying config & defaults. If you define those packages to be non-local then enabling tests for local packages (and other config given on the command line) would not apply to those packages. |
It looks like #4179 added an error message for the case where the user specifies a test target but the solver doesn't enable tests. I tried the original example with current master and got
|
@grayjay wrote:
Yeah, and I find this message not very helpful. I'd rather see the usual output of In particular, I expected |
Am I mistaken that on master we now do @andreasabel: thank you for your feedback. Feel free to try the new behaviour and add comments in newer tickets specifically about them. BTW, note that a lot of recent extra messages (and the recent |
@Mikolaj no, this hasn't been done afaik. Still dealing with the fallout from cabal haddock enabling documentation 🥲 @andreasabel I think there's a separate ticket for per component solving and the vibe there: it's nowhere near. |
@ulysses4ever : Thanks, indeed my concern was about @Mikolaj: I am using This PR would be great to have, but is seems to have stalled: |
You are both right. |
I think that we should fix this issue with the same solution that was being discussed in the latest comments on #7883 (solving for tests by default, similarly to |
If there is a conflict involving the test dependencies, the solver disables the tests. I think that is because tests are only enabled with a preference in new-build. The build fails later with "cabal: Encountered missing dependencies: ...", unless the tests' dependencies are already required by another component.
Solver log with -v3:
The text was updated successfully, but these errors were encountered: