-
Notifications
You must be signed in to change notification settings - Fork 696
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
new-test should imply --enable-tests #5079
Comments
Iirc this was a deliberate design choice by @dcoutts to avoid invocations of PS: Also, when we run the test-suite or benchmarks, we want to test the same configuration that we're working on w/ new-build; and not some different arbitrary one. PS2: The discussion @grayjay linked starting at #3923 (comment) is very relevant |
Given that constraint (which I'm not sure I agree with), I suspect the correct behavior is to default to creating a build plan that includes the tests and benchmarks (even in new-build), then prune those away along with any dependencies that are in the plan exclusively for them if the user hasn't asked for them. And of course one could fall back to the current way of creating an install plan when --disable-tests/--disable-benchmarks is supplied. A stable build plan is nice. Having a build plan at all in the first place is even nicer. |
So we'd need three way flag
I really want to have an explicit flag for default behavior (i.e. "neither And the same for benchmarks. |
@phadej you also need to take care of the dimension on how much to build by default. Currently this is conflated: |
@phadej Perhaps "local packages" instead of "as much as possible"? Although I still think "all tests" is the right default. It would be just as surprising for @hvr We already have the target specification language for specifying what to build, right? And, luckily, the new-* promise is "alpha only, for intrepid users, interface may change", etc. so retrospectively inconsistent behavior like having |
@dmwit I'm not worried much about changing the meaning of what
PS: and also note, that tests/benchmarks can be enabled/disabled at package granularity; it's not a project global setting |
I see. I suppose the problem is roughly this:
Does that sound right? If so, what if
|
c.f.
We have this already.... that's what I referred to as awkward/incovenient way; i.e. my most common use-case is to say something like I also suggest you take a look at the source-code comments in Just to put it out there, what about emulating GHC/Clang's warnings, and have
or alternatively,
|
Just to point out the absurdity of the current situation:
|
@sjakobi what's your mental model there? Are you assuming that the Would you be surprised if
failed to find an install-plan because |
Short observation: most want to solve for everything (incl tests) but build e.g. only lib part. I mentioned it already before that three state of (`--enable-tests`, `--disable-tests`, and no flag) is confusing. TargetSelector should be really a "SolverConfiguratorTargetSelector", targets we solve for are superset of build targets
…Sent from my iPhone
On 20 Mar 2018, at 12.32, Herbert Valerio Riedel ***@***.***> wrote:
@sjakobi what's your mental model there? Are you assuming that the --enable-tests flag is stateful? In your example you passed -w ghc-7.10.3 to both new-build and new-test (if you didn't, new-test could easily pick a different GHC version; would that be absurd as well?), and yet you didn't pass --enable-tests to both.
Would you be surprised if
$ cabal new-run -w ghc-7.10.3 test:tests`
failed to find an install-plan because --enable-tests was left off?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Maybe we should just add to the error something like
|
Adding to the error is certainly the low-effort workaround. But being able to separately specify solver targets and build targets (as suggested by @phadej) -- and then defaulting to a quite wide set of solver targets -- seems like the Right Solution to me. |
@dmwit there's just a minor downside to it: I'd have to keep passing |
@phadej, I would like to implement this flag. How about calling it " |
I just ran into this: |
We just ran into this on a course. I nearly gave up on |
I think we should at least suggest |
PR #6058 improves the error message. @dmwit @simonmar @michaelpj Is this a sufficient improvement? |
haskell/cabal#5079 Signed-off-by: Tycho Andersen <[email protected]>
TL;DR: Subtle changes to a package can result in test suites being silently ignored on CI (that's even true for seemingly non-significant things like changing the name of a package). I'm not planning to get too involved in this discussion, but I'm not convinced that this is merely a UI problem. One issue I encountered is that The implication of this is that subtle changes to a package can result in e.g. CI silently ignoring test suites. This can lead to undetected code breakage down the line. Given these files:
-- cabal.project
packages:
foo1
foo2 -- foo1/Main.hs
{-# LANGUAGE CPP #-}
main :: IO ()
main = putStrLn (">>> " <> CURRENT_COMPONENT_ID) cabal-version: 3.0
-- foo1/foo1.cabal
name: foo1
version: 0.0.0
build-type: Simple
executable foo1
build-depends: base
main-is: Main.hs
test-suite spec
type: exitcode-stdio-1.0
build-depends: base
main-is: Main.hs cabal-version: 3.0
-- foo2/foo2.cabal
name: foo2
version: 0.0.0
build-type: Simple
test-suite spec
type: exitcode-stdio-1.0
build-depends: base
main-is: Main.hs
hs-source-dirs: ../foo1
-- build-tool-depends: foo1:foo1
However, when you uncomment
The seemingly surprising thing here is: The behavior depends on the package names I say seemingly because what I suspect is happening here is that the dependency solver "bleeds through" to the UI and influences what the BUT: This took me quite some time to debug and properly understand. So what I'm puzzled is, do we expect everyone who wants to use |
One more thing, I think At the point you use At this point I'm puzzled if there is a generic way to run tests so that:
I'm pledging USD 10 to the first person who describes a solution that works with |
That is no longer true with |
Hey, that's great news! @phadej please feel free to claim your bounty. |
Donate it to HF (when they open non-corporate donation methods). |
Problem: we had `cabal test all` on CI which apparently sometimes doesn't work due to this issue: haskell/cabal#5079 Solution: add `--enable-tests` flag as suggested in that issue.
Problem: we had `cabal test all` on CI which apparently sometimes doesn't work due to this issue: haskell/cabal#5079 Solution: add `--enable-tests` flag as suggested in that issue.
I just ran into this problem with haskell-hvr/deepseq-generics#12. Having However, I was pessimistic that (*) would make a difference and surprised when it did. How can you In the case of haskell-hvr/deepseq-generics#12 there is just a single test suite, that apparently builds with |
|
|
Yes, I agree the error messages are confusing, too, even though the hint in the last sentence works. I vote for adding the implicit, default |
If I remember correctly, I tried to implement it, but I gave up because the cabal codebase was too complex and confusing for me. Let's give it another shot! |
FYI, the difference in Edit: e.g., if you build with |
Why would not we |
It could happen that there are no build plans with tests enabled but there are with tests disabled |
This idea is similar to the solution that the discussion in #7883 was converging on. By default, |
Is that a big problem? The user can manually disable tests or benchmarks, |
#7883 is a duplicate of this issue, but it has newer discussion of a possible solution, so I'm going to close this one. |
I observe the following behavior. Summary:
cabal new-test
fails to resolve dependencies, whilecabal new-test --enable-tests
succeeds and runs the tests. I think they should both succeed and run the tests.The project itself is quite minimal. Here's
bad-tests.cabal
:Main.hs
has justmain = return ()
, andSetup.hs
is the standardimport Distribution.Simple; main = defaultMain
.The text was updated successfully, but these errors were encountered: