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

Adds cabal doctest #4480

Merged
merged 10 commits into from May 5, 2017

Conversation

Projects
None yet
9 participants
@angerman
Copy link
Collaborator

angerman commented Apr 30, 2017

This adds cabal doctest logic, modelled after cabal haddock.

Resolves #2327

angerman added some commits Apr 29, 2017

Add `doctest’ command.
This adds the `doctest` command to cabal.  It does however not yet work as the driver
is baiscally a stub.

This is therfore only the first step towards #2327.
Adds doctest execution logic
This is a rather rough cut. But it allows to run `cabal doctest` already.
@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Apr 30, 2017

@angerman, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts, @23Skidoo and @ezyang to be potential reviewers.

@angerman

This comment has been minimized.

Copy link
Collaborator

angerman commented Apr 30, 2017

Note, that this PR is likely incomplete, and I'd like to get some feedback early on about the code so far. I'm certain there's likely some logic missing for full doctest support. Please also note that this PR depends on sol/doctest#156

@angerman

This comment has been minimized.

Copy link
Collaborator

angerman commented Apr 30, 2017

However, the current state of affairs is that:

distributive-0.5.2 $ ../cabal/dist-newstyle/build/cabal-install-2.1.0.0/build/cabal/cabal doctest --with-doctest=../doctest/dist/build/doctest/doctest
Preprocessing library for distributive-0.5.2..
Examples: 1  Tried: 1  Errors: 0  Failures: 0

already works.

@angerman

This comment has been minimized.

Copy link
Collaborator

angerman commented Apr 30, 2017

Pinging @phadej as well, who implemented https://github.com/phadej/cabal-doctest and might know the innards of doctest wrt cabal much better than I do.

@@ -309,6 +311,16 @@ hscolourProgram = (simpleProgram "hscolour") {
_ -> ""
}

doctestProgram :: Program
doctestProgram = (simpleProgram "doctest") {

This comment has been minimized.

@phadej

phadej Apr 30, 2017

Collaborator

Ok for now; but we should compile doctest against used ghc (and in fact also haddock)

This comment has been minimized.

@phadej

phadej Apr 30, 2017

Collaborator

I'd leave a todo.

This comment has been minimized.

@angerman

angerman Apr 30, 2017

Collaborator

Could (and more likely should?) cabal-install depend on doctest and haddock then, and use their binaries installed alongside cabal, unless explicitly overwritten?

This comment has been minimized.

@angerman

angerman Apr 30, 2017

Collaborator

On second thought, that might actually not be sufficient, as cabal-install could be compiled by a different ghc than the one it is used with, unless I'm mistaken.

This comment has been minimized.

@ezyang

ezyang Apr 30, 2017

Contributor

@angerman That's correct. In general, we don't have this problem for haddock because it is distributed with GHC, so we can use the same detection code that finds the correct ghc-pkg for a ghc to also get the correct haddock. For now, I'm OK with requiring people to do some footwork to get the correct doctest if they're using a different ghc, but we should test and make sure we get a reasonable error message in this case (similar to what happens when ghc and ghc-pkg versions mismatch.)

This comment has been minimized.

@ezyang

ezyang Apr 30, 2017

Contributor

I think teaching cabal-install how to compile correct versions of haddock and doctest is worthwhile, and fits into the new-build model (we need a simulated build-tools-depends) but it is definitely out of scope for this ticket. Maybe we should make a ticket for it.

Happy CI; Added comment.
This hopefully makes the CI happy.  Also added the comment regarding
the need to have doctest and haddock be built against the same ghc.
@ezyang
Copy link
Contributor

ezyang left a comment

This looks totally reasonable, but of course, the important work of passing the correct -package flags has not been done yet ;)

We'll need tests eventually, which is going to require updates to test suite (since you will need to feed it path to correct doctest.) I can advise when we get there.

@@ -290,6 +292,21 @@ hscolourAction hooks flags args = do
(getBuildConfig hooks verbosity distPref)
hooks flags' args

doctestAcion :: UserHooks -> DoctestFlags -> Args -> IO ()

This comment has been minimized.

@ezyang

ezyang Apr 30, 2017

Contributor

s/Acion/Action/

@@ -309,6 +311,16 @@ hscolourProgram = (simpleProgram "hscolour") {
_ -> ""
}

doctestProgram :: Program
doctestProgram = (simpleProgram "doctest") {

This comment has been minimized.

@ezyang

ezyang Apr 30, 2017

Contributor

@angerman That's correct. In general, we don't have this problem for haddock because it is distributed with GHC, so we can use the same detection code that finds the correct ghc-pkg for a ghc to also get the correct haddock. For now, I'm OK with requiring people to do some footwork to get the correct doctest if they're using a different ghc, but we should test and make sure we get a reasonable error message in this case (similar to what happens when ghc and ghc-pkg versions mismatch.)

@@ -309,6 +311,16 @@ hscolourProgram = (simpleProgram "hscolour") {
_ -> ""
}

doctestProgram :: Program
doctestProgram = (simpleProgram "doctest") {

This comment has been minimized.

@ezyang

ezyang Apr 30, 2017

Contributor

I think teaching cabal-install how to compile correct versions of haddock and doctest is worthwhile, and fits into the new-build model (we need a simulated build-tools-depends) but it is definitely out of scope for this ticket. Maybe we should make a ticket for it.

doctestProgram = (simpleProgram "doctest") {
programFindLocation = \v p -> findProgramOnSearchPath v p "doctest"
, programFindVersion = findProgramVersion "--version" $ \str ->
-- "doctest version 0.11.2"

This comment has been minimized.

@ezyang

ezyang Apr 30, 2017

Contributor

Hmm... we kind of really want the GHC version too, so you can check for compatibility. Not sure how to fix the API to make this possible.

@@ -761,6 +763,14 @@ haddockAction haddockFlags extraArgs globalFlags = do
createTarGzFile dest docDir name
notice verbosity $ "Documentation tarball created: " ++ dest

doctestAcion :: DoctestFlags -> [String] -> Action

This comment has been minimized.

@ezyang

ezyang Apr 30, 2017

Contributor

s/Acion/Action/

k (["--no-magic"], argTargets args)

-- -----------------------------------------------------------------------------
-- TODO: move somewhere else (this is copied from Haddock.hs!)

This comment has been minimized.

@ezyang

ezyang Apr 30, 2017

Contributor

Yes please do :)

angerman added some commits May 1, 2017

Adds package discovery and flags
This basically reuses cabals logic for computing the command line arguments.
@ezyang
Copy link
Contributor

ezyang left a comment

This is so sweet. Do you want to merge this now (perhaps as a "hidden" command) and then keep fixing bugs as they come up?

Re: -Wall, this is butting up with the "how do we specify doctest-specific packages/parameters" problem, because what you really wanted was to say -Wall for library build, but no -Wall for doctest. Do you think this problem is a show-stopper? If it is we could add a hack to solve this particular problem (e.g., a flag which enables some sort of ghc-options filtering; NOT turned on by default), while trying to hash out a design for the rest of the parameters.

-> BuildInfo
-> IO DoctestArgs
mkDoctestArgs verbosity tmp lbi clbi inFiles bi = do
let vanillaOpts = (componentGhcOptions normal lbi bi clbi (buildDir lbi))

This comment has been minimized.

@ezyang

ezyang May 1, 2017

Contributor

Since there is modest duplication here with existing code (which is not easy to remove, so don't) I'd like forward and backward pointers between the code, as a reminder that if someone changes one you might have to change the other.

then return vanillaOpts
else if withSharedLib lbi
then return sharedOpts
else die' verbosity $ "Must have canilla or shared lirbaries "

This comment has been minimized.

@ezyang

ezyang May 1, 2017

Contributor

s/canilla/vanilla/

@angerman

This comment has been minimized.

Copy link
Collaborator

angerman commented May 1, 2017

I'm fine with this a non-hidden command. Someone would need to find it anyway. And more visibility means more bug reports :-)

I'm perfectly fine with -Wall. If you want to compile your code, with -Wall why shouldn't the tests run with something else? Point being ghc-options: could contain arbitrary information. I'm not a fan of some magic argument drops from ghc-options:. If this turns out to be a real issue, I'd rather add cabal doctest [more ghc options], so that one would pass -Wno-xyz and -package there. I just don't know how to grab arbitrary arguments there and just feed them into doctest. Pointers?

We'll have to wait for sol/doctest#156 before merging this though. Especially as the version bump will likely lead to 0.12, and we'd need to adapt the version here then.

@angerman

This comment has been minimized.

Copy link
Collaborator

angerman commented May 2, 2017

We'll now ignore ghc-options:, and provide a doctest-ghc-options: field for when you need to specify ghc options when running cabal doctest. See ekmett/distributive#30.

@phadej

This comment has been minimized.

Copy link
Collaborator

phadej commented May 2, 2017

What is the scope of this PR (I.e. what should be possible after this). Some feature creep is already happening there; without discussing the design.

I'm against /just/ introducing doctest-ghc-options, the own doctest stanza feels to be better long-term goal. Adding fields which are soon removed isn't good for users.

If the lightweight support isn't possible (even simple cases doesn't work out) then lets's merge what we have to do proper RFC. @RyanGlScott will hopefully comment about problems he run into with Emmett packages

@RyanGlScott

This comment has been minimized.

Copy link
Member

RyanGlScott commented May 2, 2017

I think I would would need to a see a test case to gauge how well I like the proposed implementation of this feature, since I can't tell from the code what this will end up looking like in practice.

Also, I'd prefer the name doctest-options (instead of doctest-ghc-options) for congruity with haddock-options.

@angerman

This comment has been minimized.

Copy link
Collaborator

angerman commented May 3, 2017

@phadej, @RyanGlScott: this PR is mostly here to get this ball rolling, #2327 has been started over two years ago, and I believe there needs to be some progress. My motivation might be different from others, as I'm primarily focused on getting rid of build-type: Custom, which in my eyes is an escape hatch, and should rarely be needed. It also complicates cross compilation. Of course we can try to teach cabal about different compilers (host, target, ...; c.f. #1493), which will eventually lead to full support for build-type: Custom.

In any case, do I believe that it should be as trivial as cabal doctest to run doctest over the library(s) and executable(s) defined in the .cabal file.

After giving the doctest-ghc-options: some thought, I'll scrap them again, in favor of just passing whatever is passed to cabal doctest to further down to doctest, which in turn would pass those on to ghci, in addition to the flags that cabal computes for the relevant component. The proper way seems to not pass ghc-options: when running cabal doctest.

So what would this change for a package?

  • No more build-type: Custom just to get doctest.
  • No more custom Setup.hs, for anything doctest related.
  • instead of running cabal test doctests, just run cabal doctest.
@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented May 3, 2017

Note that haddock-options isn't actually a thing at the Cabal level. It works from command line because haddock is part of the program database, and all programs in the program db get support for --foo-options automatically. This means, with the implementation today, --doctest-options is already a thing and can let you feed some custom doctest options as necessary.

@angerman

This comment has been minimized.

Copy link
Collaborator

angerman commented May 3, 2017

So with the latest changes this is what we get (e.g. for distributive):
(these all assume that doctest is installed with PR sol/doctest#156)

distributive $ cabal doctest --help
Run doctest tests.

Usage: cabal doctest [FLAGS]

Requires the program doctest, version 0.12.

Flags for doctest:
 -h --help                 Show this help text
 -v --verbose[=n]          Control verbosity (n is 0--3, default verbosity
                           level is 1)
    --builddir=DIR         The directory where Cabal puts generated build files
                           (default dist)
    --with-doctest=PATH    give the path to doctest
    --PROG-option=OPT      give an extra option to PROG (no need to quote
                           options containing spaces)
    --doctest-options=OPTS give extra options to doctest
distributive $ cabal doctest
Preprocessing library for distributive-0.5.2..
Examples: 1  Tried: 1  Errors: 0  Failures: 0
distributive $ cabal doctest --doctest-options=-Wall
Preprocessing library for distributive-0.5.2..
### Failure in src/Data/Distributive.hs:84: expression `distribute [(+1),(+2)] 1'
expected: [2,3]
 but got:
          <interactive>:23:1: warning: [-Wtype-defaults]
              • Defaulting the following constraints to type ‘Integer’
                  (Show t0) arising from a use of ‘print’ at <interactive>:23:1-24
                  (Num t0) arising from a use of ‘it’ at <interactive>:23:1-24
              • In a stmt of an interactive GHCi command: print it
          [2,3]
Examples: 1  Tried: 1  Errors: 0  Failures: 1

@RyanGlScott would this work as a test case for you?

Fix -package-db for inplace package dbs; require doctest 0.11.3 and up.
The inplace package-db was not passed by default, as I had initially assumed.
We also no encode the requirement for doctest 0.11.3, which sports the
--no-magic flag.

@angerman angerman merged commit 6beff4e into haskell:master May 5, 2017

12 checks passed

ci/sake-bot/linux-7.10.3 Downstream Travis
Details
ci/sake-bot/linux-7.4.2 Downstream Travis
Details
ci/sake-bot/linux-7.6.3 Downstream Travis
Details
ci/sake-bot/linux-7.8.4 Downstream Travis
Details
ci/sake-bot/linux-8.0.2 Downstream Travis
Details
ci/sake-bot/linux-8.0.2-fdebug-expensive-assertions Downstream Travis
Details
ci/sake-bot/linux-8.0.2-parsec Downstream Travis
Details
ci/sake-bot/osx-7.10.3 Downstream Travis
Details
ci/sake-bot/osx-7.8.4 Downstream Travis
Details
ci/sake-bot/osx-8.0.2 Downstream Travis
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 5, 2017

This seems to lack documentation and a changelog note. OK for now, since it's expected to be buggy.

@sol

This comment has been minimized.

Copy link
Member

sol commented on Cabal/Distribution/Simple/Program/Builtin.hs in 435fd96 Jul 28, 2018

@angerman I think this also changed the semantics of the build-tools field. Was that by intention?

This comment has been minimized.

Copy link
Collaborator

angerman replied Jul 28, 2018

Most certainly not.

This comment has been minimized.

Copy link
Member

sol replied Jul 28, 2018

But then again, this may not be specific to your change. Here is how the list of build-in build tools got extended in the past:

1.14.0:
hpc

1.22.4:
ghcjs
ghcjs-pkg
haskell-suite
haskell-suite-pkg

2.0.0:
runghc
doctest

I'm not convinced yet that this always was on purpose and I wouldn't be surprised if this were side effects of other changes.

As I understand it, since 2.0 arbitrary build tools are accepted anyway, where the observable difference between a built-in build tool and an unknown build tool is:

  • The version specified for an unknown build tool is silently ignored
  • Cabal does not error out if an unknown build tool is not on the PATH

@ezyang @peti this last change I'm mentioning, was this primarily motivated by nix, so that nix can install system tools without the need of a custom setup script?

@hvr

This comment has been minimized.

Copy link
Member

hvr commented Jul 28, 2018

@sol, I'm a bit confused, are you saying that the current behaviour doesn't match

http://cabal.readthedocs.io/en/latest/developing-packages.html#buildtoolsmap

(i.e. as tools like runghc and doctest are not not documented being part of that legacy mapping?)

The version specified for an unknown build tool is silently ignored

we could warn that a version was specified for unknown build-tools; but other than that we have little choice but to ignore it for tools for which we don't know how to infer the tool's version

Cabal does not error out if an unknown build tool is not on the PATH

if this is in fact the case, this sounds like a bug/regression to me

cc @Ericson2314

@sol

This comment has been minimized.

Copy link
Member

sol commented Jul 29, 2018

I'm a bit confused, are you saying that the current behaviour doesn't match

http://cabal.readthedocs.io/en/latest/developing-packages.html#buildtoolsmap

I don't think the documentation is wrong. As I understand it there is merely some undocumented behavior.

As an example, here are two things that you can't infer from the documentation:

  • You can specify a version constraint for doctest and Cabal >= 2 will fail if that version constraint is not met by a pre-installed doctest executable
  • You can list ghc under build-tools and Cabal < 2 will not error out due to an unknown build tool

we could warn that a version was specified for unknown build-tools

I'm still not sure if that is desirable. As mentioned earlier, I could imagine that the version constraint is used by third party tools (specifically cabal2nix) to install system build tools. @peti do you have any input on this?

we have little choice but to ignore it for tools for which we don't know how to infer the tool's version

Yes, I'm not suggesting to change anything here.

Cabal does not error out if an unknown build tool is not on the PATH

if this is in fact the case, this sounds like a bug/regression to me

I opened #5464 for that.

@peti

This comment has been minimized.

Copy link
Collaborator

peti commented Jul 29, 2018

I could imagine that the version constraint is used by third party tools (specifically cabal2nix) to install system build tools.

We currently don't use that version information in Nix. When we convert a Cabal file into a Nix expression, all dependencies become inputs of the build function and whoever calls that function must provide in whatever versions they deem appropriate. So the build expression itself does not know which version are legal or not; it simply tries to build whatever configuration the upper layer sets up.

Now, this might change at some point in the future, but I don't think it will be any time soon.

Maybe it's a good idea to have Cabal warn about the fact unsupported tools cannot be version-checked Cabal. The problem I see is that cabal check warnings are considered errors by many CI setups, so if that warning were implemented we'd essentially force many people to remove the version information, which doesn't feel like a good idea. If you know the version constraints, then by all means put them in the Cabal file! All we need is people to be aware that they aren't enforced (yet) when they do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment