Skip to content
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

Remove dependency on cabal-doctest #38

Closed
wants to merge 1 commit into from

Conversation

tfausak
Copy link

@tfausak tfausak commented Sep 29, 2017

This fixes #37. It also switches to the lts-9.0 resolver because it wouldn't build with nightly-2016-01-08.

@RyanGlScott
Copy link
Collaborator

This will break the doctests, and more importantly, this isn't the solution to the problem you're facing. See my comments here.

@tfausak
Copy link
Author

tfausak commented Sep 29, 2017

This will break the doctests

Then the build might not be configured properly.

screen shot 2017-09-29 at 9 37 13 am

Regardless, the tests do fail for me locally. Fixing them is pretty easy:

diff --git a/tests/doctests.hs b/tests/doctests.hs
index 2d080e7..1a136d7 100644
--- a/tests/doctests.hs
+++ b/tests/doctests.hs
@@ -13,13 +13,8 @@
 -----------------------------------------------------------------------------
 module Main where
 
-import Build_doctests (flags, pkgs, module_sources)
-import Data.Foldable (traverse_)
 import Test.DocTest
 
 main :: IO ()
 main = do
-    traverse_ putStrLn args
-    doctest args
-  where
-    args = flags ++ pkgs ++ module_sources
+    doctest ["src"]

I don't grok cabal-doctest. What does it do?

@RyanGlScott
Copy link
Collaborator

RyanGlScott commented Sep 29, 2017

Indeed, I'm quite surprised that Travis is green there, since cabal test definitely failed here.

In any case, I encourage reading cabal-doctest's README for an overview of what it does. It solves many common pratfalls that can befall you when using doctest at scale. For example, if you have two packages installed that both define a module named Foo and attempt to import Foo somewhere in your project, then running doctest naked on your code will fail, since it won't be able to disambiguate between the two packages that provide Foo. The solution is to pass an explicit package ID an an argument to doctest to resolve the ambiguity, a tiresome task which cabal-doctest automates by gathering the appropriate info during the configure phase of Cabal.

Yes, it's quite annoying that all of this requires a custom Setup script to accomplish. There are plans to make Cabal aware of doctest in the future, so one day we will be able to axe this Setup shim entirely. But until then, we must do what we can.

As I said in #37, the best solution to the particular problem you're facing (using Cabal HEAD) is to bump the upper version bounds on Cabal in cabal-doctest to allow it.

@tfausak
Copy link
Author

tfausak commented Sep 29, 2017

I think the build succeeds because the test script just runs a bunch of commands without checking the exit codes. They would need to be joined by && for the comment above them to be accurate.

I did read cabal-doctest's README, but it wasn't clear to me what it actually does. Thanks for explaining it. For this particular case, there's only a single doctest and it works fine without cabal-doctest. Is cabal-doctest really necessary here?

@RyanGlScott
Copy link
Collaborator

cabal-doctest isn't strictly necessary for testing distributive in isolation on Travis since the only packages we install are precisely its dependencies, so there's no worry of global module conflicts as described above. But there's no guarantee that this will hold if someone attempts to test distributive on their machine, which might have a substantially larger package database. Indeed, this is a problem that Stackage (which maintains an extraordinarily larges package database) has to deal with regularly. See here and here for examples of where this has bitten.

So yes, cabal-doctest is necessary to ensure that the tests will work reliably with different varieties of configurations.

@tfausak tfausak closed this Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot install with Cabal 2.1
2 participants