soenkehahn / getopt-generics

Create command line interfaces with ease
Other
41 stars 3 forks source link

getopt-generics-0.6.2 cannot compile its test suite #23

Closed peti closed 9 years ago

peti commented 9 years ago

Citing from http://hydra.cryp.to/build/781955/log/raw:

Preprocessing test suite 'spec' for getopt-generics-0.6.2...

src/System/Console/GetOpt/Generics.hs:10:14: Warning:
    -XOverlappingInstances is deprecated: instead use per-instance pragmas OVERLAPPING/OVERLAPPABLE/OVERLAPS
ghc: could not execute: markdown-unlit

I suppose markdown-unlit needs to be declared as a build-tool: dependency of the test suite.

jkarni commented 9 years ago

@peti Thanks! Just adding it to the build-tool section of the cabal file isn't enough. Nix then fetches the package and makes the executable available, but:

[nix-shell:...$ ./Setup.hs configure --enable-tests
Configuring getopt-generics-0.6.2...
Setup.hs: Unknown build tool markdown-unlit

I think the Setup.hs file also has to be updated, but I don't as of yet know how.

soenkehahn commented 9 years ago

I've added markdown-unlit to the build-dependencies of the test-suite in this branch: https://github.com/zalora/getopt-generics/tree/fix-test-suite. Does that work for nix?

peti commented 9 years ago

Well, that change would fix the build error. Note that markdown-unlit belongs into the build-tools stanza, though, because it's not really a library dependency of your package. IMHO, it would be cleaner to list it there.

jkarni commented 9 years ago

Adding it to the build-tools is precisely what breaks it for me. Without it it works, with it I get the error above.

peti commented 9 years ago

Oh, sorry. Somehow I didn't see your earlier message. The Setup.hs file has to look like this:

module Main ( main ) where

import Distribution.Simple
import Distribution.Simple.Program

main :: IO ()
main = defaultMainWithHooks simpleUserHooks
       { hookedPrograms = [ simpleProgram "markdown-unlit" ]
       }

Note that build-type must be switched to custom, too, if it isn't already.

sol commented 9 years ago

What I know is that listing it as a dependency works with cabal, sandboxes, stackage, nix and possibly in other setups too. We use the same approach for hspec-discover and I'm not aware of any issues with that approach.

Is there any evidence that listing it as a build-tool or having a custom Setup.lhs has any tangible and practically relevant advantages that outweigh the costs?

Sent from my iPhone

On 24 Apr 2015, at 5:27 pm, Peter Simons notifications@github.com wrote:

Oh, sorry. Somehow I didn't see your earlier message. The Setup.hs file has to look like this:

module Main ( main ) where

import Distribution.Simple import Distribution.Simple.Program

main :: IO () main = defaultMainWithHooks simpleUserHooks { hookedPrograms = [ simpleProgram "markdown-unlit" ] } Note that build-type must be switched to custom, too, if it isn't already.

— Reply to this email directly or view it on GitHub.

peti commented 9 years ago

@sol, are there any issues with using the correct stanza, build-tools, which exists precisely for a case like this one?

In Nix, for instance, build dependencies are propagated. If B depends on A, and A depends on markdown-unlit, then B will get markdown-unlit as a build input -- even though it doesn't need it. Build tools, however, are local to the build and are not propagated. From the point of view of Nix, it's definitely preferable to declare the type of the dependency correctly. I'm not sure why you seem to resist doing that?

jkarni commented 9 years ago

https://github.com/zalora/getopt-generics/tree/build-tool-fix compiles under nix, with markdown-unlit only in the build-tools (if that's how we want to do it).

peti commented 9 years ago

Yes, this looks perfect to me. Thank you for the quick fix. (I'm not sure about the empty line at https://github.com/zalora/getopt-generics/blob/build-tool-fix/getopt-generics.cabal#L62, BTW. Does cabal check not complain about that? Anyway, just wondering ...)

jkarni commented 9 years ago

@peti the whitespace just separates out the library from the test dependencies. And cabal check seems perfectly happy with it.

sol commented 9 years ago

@peti build-tools are not automatically installed by cabal. Specifying things as a regular dependency instead may seem odd, but it has the advantage that they are installed on

cabal install --only-dependencies --enable-tests

This may not be relevant for Nix, but it is relevant if you use vanilla cabal.

For getopt-generics it's @soenkehahn's call. But for my projects, unless there is a strong case not to or unless somebody can point out a better approach, I will continue to list build tools as regular dependencies in test-suite sections.

Out of curiosity (I'm not into the details of Nix), markdown-unlit is only a test dependency. Do test dependencies propagate in the way you described? And if they propagate, what are the practical implications (note that markdown-unlit only (!) depends on base)?

peti commented 9 years ago

build-tools are not automatically installed by cabal.

Personally, I don't consider that to be a problem worth sacrificing correct semantics. Your mileage may vary, though.

markdown-unlit is only a test dependency. Do test dependencies propagate [in Nix] the way you described?

No, test dependencies are local to the build.

soenkehahn commented 9 years ago

What I care about most is that the package works, both with nix and with stand-alone cabal. And putting markdown-unlit in the build-tools does only help in the nix case. With stand-alone cabal you would still be required to manually make sure markdown-unlit is installed. Whereas putting it in the build-depends section as a library dependency (of the test-suite) makes the package work out of the box both with nix and with stand-alone cabal. I'm going to go with that.

The whole thing is a bit of a messy problem I think. And it shouldn't be. All I wanted to do is to have a README file that contains Haskell snippets that would be typechecked. Ideally that README would show up as the github README but also on hackage as part of the documentation. (Which it doesn't currently. Well, there is a link that I manually inserted, but it's not elegant.) Maybe we can come up with a cleaner solution at some point. But I'll close this for now.

soenkehahn commented 9 years ago

Fixed through f9ba878be8c4718c3960e02270a3683f4364d92d.

soenkehahn commented 9 years ago

Released to hackage as 0.6.3.

peti commented 9 years ago

Thank you very much for the fix: http://hydra.cryp.to/eval/386480.

soenkehahn commented 9 years ago

@peti: Nice! Thanks for reporting. And for packaging. :)