Add a missing dependency on hspec-discover

[kirelagin]

No description provided.

[qrilka]

@kirelagin could you share why do you need to have an explicit dependency here? The only build tool that requires it (but probably shouldn't?) that I know has an issue about this problem input-output-hk/nix-tools#38

[kirelagin]

Personally I need this for my nix-based builds as well. But, regardless of my specific case, specifying dependencies properly is the right thing to do.

I am not quite sure what the state of underspecified dependencies currently with Cabal2 is, but my understanding is that a package with a missing build-tool dependency on hspec-discover will not build with cabal new-build today and it’s been this way for a while now, at least that’s how I read this comment.

[qrilka]

@kirelagin I'm not saying that you shouldn't be explicit about dependencies but that doesn't look like the way current Haskell tooling works: the current safe-decimal runs tests fine on 1) Stack; 2) Cabal v1 commands; 3) Cabal v2 commands; 4) generic-builder.nix from nixpkgs (which uses 2) under the hood)
And also Stackage contains hundreds of packages using hspec as a dependency and hspec-discover executable comes as a transitive dependency

[kirelagin]

runs tests fine on <...>

Ok, that’s good to know! I have to admit that I am not very familiar with the way Cabal handles this case of underspecified dependencies. I suspect it adds binaries of all transitive dependencies to PATH, however I couldn’t find this behaviour documented anywhere, but I did find discussions where people seem to agree that this practice is very bad.

There is a workaround in cabal2nix that adds hspec-discover (and some other tools) to build-tool-depends if it finds it in test-depends and it is aimed precisely at those packages that incorrectly specify this tool dependency as a library dependency, but nothing for allowing binaries from transitive, not declared explicitly dependencies.

[kirelagin]

Not to be overly persistent or sound preachy (although, I still do not understand if you are against merging this PR or not), but are you sure about it working with Cabal2? I tried it and what I see on my own machine seems to be consistent with what is suggested in the comment I linked above:

$ cabal new-test
Build profile: -w ghc-8.6.3 -O1
In order, the following will be built (use -v for more details):
 - hspec-discover-2.7.0 (lib) (requires download & build)
 - hspec-expectations-0.8.2 (lib) (requires download & build)
 - quickcheck-io-0.2.0 (lib) (requires download & build)
 - setenv-0.1.1.3 (lib) (requires download & build)
 - hspec-core-2.7.0 (lib) (requires download & build)
 - hspec-2.7.0 (lib) (requires download & build)
 - safe-decimal-0.1.0.0 (test:tests) (first run)
Downloading  hspec-discover-2.7.0
Downloaded   hspec-discover-2.7.0
Downloading  setenv-0.1.1.3
Starting     hspec-discover-2.7.0 (lib)
Downloaded   setenv-0.1.1.3
Downloading  hspec-expectations-0.8.2
Starting     setenv-0.1.1.3 (lib)
Downloaded   hspec-expectations-0.8.2
Downloading  quickcheck-io-0.2.0
Starting     hspec-expectations-0.8.2 (lib)
Downloaded   quickcheck-io-0.2.0
Downloading  hspec-core-2.7.0
Starting     quickcheck-io-0.2.0 (lib)
Building     hspec-discover-2.7.0 (lib)
Downloaded   hspec-core-2.7.0
Downloading  hspec-2.7.0
Downloaded   hspec-2.7.0
Building     setenv-0.1.1.3 (lib)
Building     hspec-expectations-0.8.2 (lib)
Building     quickcheck-io-0.2.0 (lib)
Installing   setenv-0.1.1.3 (lib)
Completed    setenv-0.1.1.3 (lib)
Installing   quickcheck-io-0.2.0 (lib)
Installing   hspec-discover-2.7.0 (lib)
Installing   hspec-expectations-0.8.2 (lib)
Completed    hspec-discover-2.7.0 (lib)
Completed    quickcheck-io-0.2.0 (lib)
Completed    hspec-expectations-0.8.2 (lib)
Starting     hspec-core-2.7.0 (lib)
Building     hspec-core-2.7.0 (lib)
Installing   hspec-core-2.7.0 (lib)
Completed    hspec-core-2.7.0 (lib)
Starting     hspec-2.7.0 (lib)
Building     hspec-2.7.0 (lib)
Installing   hspec-2.7.0 (lib)
Completed    hspec-2.7.0 (lib)
Configuring test suite 'tests' for safe-decimal-0.1.0.0..
Preprocessing test suite 'tests' for safe-decimal-0.1.0.0..
Building test suite 'tests' for safe-decimal-0.1.0.0..
ghc: could not execute: hspec-discover

[kirelagin]

generic-builder.nix from nixpkgs (which uses 2) under the hood)

Also I would like to point out that (unfortunately) generic Haskell builder does not use cabal-install under the hood, it uses Setup.hs and manages all dependencies in GHC package db (and in PATH) itself. I don’t quite remember whether the commit enabling strictDeps by default was reverted or not, but binaries from unspecified dependencies (modulo the workaround for tool dependencies incorrectly specified as library dependencies that I mentioned before) will definitely not be available with strictDeps = true.

[llelf]

+1 for @kirelagin
I can confirm that it doesn't work right now.
build-tool-depends is the right way.

[qrilka]

@kirelagin I guess what I saw is just cabal new-test not using pure PATH, it appears that I have hspec-discover installed system-wide on this machine, overall I agree that explicit is better than implicit and the only thing which bothers me a bit is that a lot of packages rely on hspec-discover being available from hspec. Probably it's time to start migration to explicit dependencies

[lehins]

Thank you guys for the discussion and @kirelagin thank you for the PR.