-
Notifications
You must be signed in to change notification settings - Fork 247
support cabal-doctest #427
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
support cabal-doctest #427
Conversation
@cdepillabout https://github.com/cdepillabout/cabal-doctests-test gives me an empty repository? |
@angerman Sorry about that. I've pushed the https://github.com/cdepillabout/cabal-doctests-test repo, so it should now be on github. I have this PR working with commit e07ce94 in |
@cdepillabout great! Could I bother you to bother you to explain the problem (how you see it) and the solution to solve it (as taken in this PR) in a bit more detail? I'm still left a bit puzzled after reading the PR description. |
Yes definitely. This current PR is still completely hacky, I just wanted to throw something up to show that I am working on it, since there seem to be people other than us who want this working (#388). I will add a much better explanation when I have something that isn't completely hacky. |
ad9386f
to
c227bcf
Compare
acdfa9e
to
58ec878
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR should be ready to be reviewed.
This PR starts to add support for running tests using cabal-doctest
.
In this comment, I will explain a little how doctest
and cabal-doctest
work, as well as how this PR tries to support cabal-doctest
.
doctest
doctest
is a library for writing runnable examples directly in Haddocks.
Here's an example of writing doctests:
-- | Add two numbers.
--
-- >>> addTwoNums 3 5
-- 8
addTwoNums :: Int -> Int -> Int
addTwoNums a b = a + b
The doctest
package exposes a doctest
function. This function takes a list of GHCi options.
Effectively, it searches through the list of modules/sources provided to it, looking for doctests in haddocks. It uses the GHCi api to load the module containing the doctest, and then runs the doctests.
It is generally used as a Cabal test suite (similar to a set of hspec tests, tasty tests, etc). Here is an example:
https://github.com/sol/doctest/blob/master/example/test/doctests.hs
cabal-doctest
The big problem with doctest
is that you manually have to figure out what flags to pass doctest
(because it internally passes the flags to GHCi). This is possible for simple packages, like the example above, but it becomes prohibitively difficult when you have a complicated package (lots of dependencies, required GHC options, extensions specified in the .cabal file, etc).
This is compounded by the fact that stack
and cabal-install
(and nixpkgs
and haskell.nix
) all run cabal tests in slightly different environments, with slightly different package databases available.
As a way to work around these problems, phadej came up with the cabal-doctest
package.
cabal-doctest
uses a custom Setup.hs
script to figure out exactly what flags need to be passed to doctest
when it runs.
Here's an example of a package using cabal-doctest
:
https://github.com/phadej/cabal-doctest/tree/master/simple-example
See, for example, the Setup.hs
script:
https://github.com/phadej/cabal-doctest/blob/master/simple-example/Setup.hs#L13
And the corresponding test:
When the code in Setup.hs
runs, it auto-gens a module with the arguments that need to be passed to doctest
.
Here's an example of these arguments for the test that has been added in this PR:
-i
-i/build/cabal-doctests/dist/build/autogen
-i/build/cabal-doctests/dist/build
-i/build/cabal-doctests
-package-env=-
-hide-all-packages
-clear-package-db
-package-db=/nix/store/cbb4w3pxpn8rd8yfzfx4ygmyldvg9gfs-cabal-doctests-test-0.1.0.0-test-doctests-config/package.conf.d
-package-db=dist/package.conf.inplace
-optP-include
-optPdist/build/autogen/cabal_macros.h
-package-id=aeson-1.4.6.0-A6G20UcWPKt2VtuRP8GY1a
-package-id=base-4.12.0.0
-package=cabal-doctests-test-0.1.0.0
-package-id=doctest-0.16.2-508StU9N2bgCYYJI9HQFOO
Lib
Paths_cabal_doctests_test
This specifies a couple different necessary things:
- a few package databases that need to used to find all required packages
- directories to find autogen'd Haskell files
- a list of modules we actually want to run
doctest
on
cabal-doctest
enables doctests to work in almost all environments, regardless of whether you build with stack
, cabal
, or normal nixpkgs
.
cabal-doctest
+ haskell.nix
Unfortunately, tests using cabal-doctest
didn't work with haskell.nix
. There were three main reasons for this:
-
cabal-doctest
is only able to auto-gen the correct modules when runningsetup configure all
. However, haskell.nix callssetup configure
separately for each component.This PR fixes
comp-builder
so that when building components where theisDoctest
option is set totrue
, we run something likesetup configure
instead ofsetup configure test:test_name
. This allows the doctest-specific modules to be auto-generated correctly.This comment from phadej basically states that
setup configure
doesn't really work for separate components withbuild-type: Custom
: Work better with Haskell.nix? ulidtko/cabal-doctest#57 (comment). So I think callingsetup configure
instead ofsetup configure test:test_name
is a reasonable workaround for when theisDoctest
option is set totrue
. -
doctests need to be executed in an environment with the
dist
directory available from where they were built. I did this by saving thedist/
directory into a separate output, but only whenisDoctest
is true.This is slightly hacky, but I couldn't see a way around it.
-
When running
setup configure
for the components whereisDoctest
is true, we actually need to pass configure options for building all the components.This is the hackiest part of this PR. I was able to get this working by passing in the
all
component to thecomp-builder
, and using it to generate the configure options whenisDoctest
is true.This appears to work, but is somewhat hacky.
Ideally, this would instead work by setting the things like
depends
,includes
, etc for a component whereisDoctest
is true to the same value asdepends
,includes
, etc for theall
component.Originally, I tried (and failed) to implement similarly to how the
all
component is defined:haskell.nix/modules/package.nix
Lines 309 to 332 in 5145fc9
# This has one quirk. Manually setting options on the all component # will be considered a conflict. This is almost always fine; most # settings should be modified in either the package options, or an # individual component's options. When this isn't sufficient, # mkForce is a reasonable workaround. # # An alternative solution to mkForce for many of the options where # this is relevant would be to switch from the bool type to # something like an anyBool type, which would merge definitions by # returning true if any is true. config.components.all = let allComps = haskellLib.getAllComponents config; in lib.mkMerge ( builtins.map (c: # Exclude attributes that are likely to have conflicting definitions # (a common use case for `all` is in `shellFor` and it only has an # install phase). builtins.removeAttrs c ["preCheck" "postCheck" "keepSource"] ) allComps ) // { # If any one of the components needs us to keep the source # then keep it for the `all` component keepSource = lib.foldl' (x: comp: x || comp.keepSource) false allComps; }; pseudo-code:
{ config.components.tests = let doctestComps = haskellLib.getAllDoctestComponents config; in map (comp: comp // { depends = config.components.all.depends; }) doctestComps; }
However, something like this won't work because of the nixos module system. I'm not sure of a good way around this. Maybe there is a clever way of writing this using mkMerge...?
Alternatively,
cabal-to-nix
could be modified in order to spit out the correctdepends
,includes
,gcc-options
, etc for components whereisDoctest
is true. Unfortunately, I'm not sure how to tellcabal-to-nix
to do something like this. Is it even possible to pass package options likeisDoctest
directly tocabal-to-nix
??
If you guys have any good ideas for how to deal with the last point, I'd be very interested in fixing it up.
The current implementation in this PR isn't absolutely terrible, but passing in the all
component just for getting setup
options is not very nice.
Also, it appears that phadej is working towards an actual cabal doctest
command, so hopefully we'll be able to hook into that magic when available:
builder/comp-builder.nix
Outdated
@@ -1,6 +1,7 @@ | |||
{ stdenv, buildPackages, ghc, lib, pkgconfig, gobject-introspection ? null, haskellLib, makeConfigFiles, ghcForComponent, hsPkgs, runCommand, libffi, gmp }: | |||
|
|||
{ componentId | |||
{ allComponent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the hackiness.
builder/comp-builder.nix
Outdated
if isDoctest | ||
then | ||
makeConfigFiles { | ||
inherit (package) identifier; | ||
component = allComponent; | ||
inherit fullName flags; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of the hackiness.
builder/comp-builder.nix
Outdated
'' + (lib.optionalString isDoctest '' | ||
cp -r dist/ $dist/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running the doctests, they need access to the dist/
directory that was created from setup configure
(and maybe setup build
?). We save it here in a separate output.
builder/hspkg-builder.nix
Outdated
buildComp = allComponent: componentId: component: comp-builder { | ||
inherit allComponent componentId component package name src flags setup cabalFile cabal-generator patches revision | ||
shellHook | ||
; | ||
}; | ||
|
||
in rec { | ||
components = haskellLib.applyComponents buildComp config; | ||
components = haskellLib.applyComponents (buildComp config.components.all) config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the beginning of the hackiness. I explicitly pass the all
component to comp-builder
.
lib/check.nix
Outdated
# cabal-doctest assumes we are running tests in a directory with the same | ||
# name as when we built the test. | ||
this_dir_name=$(pwd) | ||
src_basename="$(basename "${drv.cleanSrc}")" | ||
cd ../ | ||
mv "$this_dir_name" "$src_basename" | ||
cd "$src_basename" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another small hack, but it shouldn't really affect anything important.
The reason this is needed is because cabal-doctest
figures out include flags relative to the directory we used to build the component.
For example, here are the flags cabal-doctest
has figured out for the test that has been added in this PR:
-i
-i/build/cabal-doctests/dist/build/autogen
-i/build/cabal-doctests/dist/build
-i/build/cabal-doctests
-package-env=-
-hide-all-packages
-clear-package-db
-package-db=/nix/store/cbb4w3pxpn8rd8yfzfx4ygmyldvg9gfs-cabal-doctests-test-0.1.0.0-test-doctests-config/package.conf.d
-package-db=dist/package.conf.inplace
-optP-include
-optPdist/build/autogen/cabal_macros.h
-package-id=aeson-1.4.6.0-A6G20UcWPKt2VtuRP8GY1a
-package-id=base-4.12.0.0
-package=cabal-doctests-test-0.1.0.0
-package-id=doctest-0.16.2-508StU9N2bgCYYJI9HQFOO
Lib
Paths_cabal_doctests_test
You can see that the first three flags are as follows:
-i/build/cabal-doctests/dist/build/autogen
-i/build/cabal-doctests/dist/build
-i/build/cabal-doctests
The default directory name is /build/cabal-doctests-test-0.1.0.0-test-doctests
, so it must be changed to /build/cabal-doctests/
to match what the doctests are expecting.
lib/default.nix
Outdated
else | ||
if component.isDoctest | ||
# TODO: Why is this needed??? | ||
then "--enable-tests" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed so that all the other components actually get configured when we run setup configure
for the doctests. The doctests need this in order to find the library in the package (as well as things like auto-generated modules)
58ec878
to
f7dc472
Compare
I should add that this PR contains a test that exercises some features of $ nix build -f ./test cabal-doctests You can try running just the relevant $ nix build -f ./test cabal-doctests.run.passthru.packages.cabal-doctests-test.checks.doctests |
@cdepillabout sorry for the lengthy wait for the review. Conceptually I'm ok with this.
|
@angerman Sorry I haven't been more responsive on this. I had planned on fixing this up more but I ended up getting somewhat busy at work.
Yeah, my original intention was to add more documentation after it was decided whether something like this would actually be merged.
Yes, I completely agree with this. The use of I'd be happy with supporting |
In a way I am not sure using |
Thanks for fixing this up.
Does this mean that this PR would potentially be accepted as-is, assuming I go back and add some docs? I was mostly worried that this approach wouldn't be accepted, since it is kinda weird explicitly passing the |
tryBuild failed: |
bors try |
tryBuild succeeded: |
@ocharles should be working now. Tests at least are passing. Breaking one of the doctests does seem to produce a test failure. I'm not really sure if it is doing the right thing in all cases. Adding
I suspect this means that the modules in |
@hamishmack Almost there! The only thing I get now is:
This is when running the check. So I think related to what you see, but for some reason it causes a crash for me, and a warning for you. Hmm. I'm on IRC if you want to chat there |
Another one of our libraries fails in a more interesting way:
No idea what that's about! I'll try and dig into this more soon. |
@hamishmack I found a solution to my problem. In my
I think the problem is the
Note this last entry has no ID, but because of our sym-linking, it'll use the already built library component. Does that make sense? If so, maybe this is something we want to roll into this PR. For some reason, of all 4 packages we use |
# (not just the one we are building). | ||
# Enable for tests in packages that use cabal-doctest. | ||
( haskellLib.isTest componentId && | ||
lib.any (x: x.identifier.name or "" == "cabal-doctest") package.setup-depends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think package.setup-depends
is wrong here. If a single test-suite
depends on cabal-doctest
then setup-depends
will contain cabal-doctest
. But if another test-suite
is defined that doesn't depend on cabal-doctest
, then this test-suite
will also build all components. I think this is wrong. Can we look at just what this component depends on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's my humble suggestion:
lib.any (x: x.identifier.name or "" == "cabal-doctest") package.setup-depends | |
lib.any (x: x.identifier.name or "" == "cabal-doctest") package.setup-depends && | |
lib.any (x: x.identifier.name or """ == "doctest") component.depends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumb suggestion: what if we just made people set this themselves? This is only a problem for test components using doctest
, so is it totally unreasonable to say "if you use doctest, you should set this option on the corresponding test component"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could, but I like the mantra of "if you do cabal X
and it works, then it will work with Haskell.nix". That will no longer hold for this case now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling I'm about to say something I've said in the past, but... why does this work in cabal itself? I vaguely recall that cabal turns off per-component builds if you have a custom setup - should we do the same thing? Maybe that would work more simply than trying to detect cabal-doctest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That one is outside my knowledge, I'm afraid!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we do the same thing?
I think this change is a step in that direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it works well enough to use for all custom setup packages yet. In particular I think the component filtering will not work.
@@ -45,7 +53,7 @@ let self = | |||
, doHoogle ? component.doHoogle # Also build a hoogle index | |||
, hyperlinkSource ? component.doHyperlinkSource # Link documentation to the source code | |||
, quickjump ? component.doQuickjump # Generate an index for interactive documentation navigation | |||
, keepSource ? component.keepSource # Build from `source` output in the store then delete `dist` | |||
, keepSource ? component.keepSource || configureAllComponents # Build from `source` output in the store then delete `dist` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
# If configureAllComponents is set we should not specify the component | ||
# and Setup will attempt to configure them all. | ||
if configureAllComponents | ||
then ["--enable-tests" "--enable-benchmarks"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this respect package/component settings of whether to enable tests/benchmarks?
$SETUP_HS copy ${lib.concatStringsSep " " ( | ||
setupInstallFlags | ||
++ lib.optional configureAllComponents | ||
(haskellLib.componentTarget componentId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just do this unconditionally? Does it hurt us to setup copy
a specific component even if we just configured for that component?
# (not just the one we are building). | ||
# Enable for tests in packages that use cabal-doctest. | ||
( haskellLib.isTest componentId && | ||
lib.any (x: x.identifier.name or "" == "cabal-doctest") package.setup-depends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling I'm about to say something I've said in the past, but... why does this work in cabal itself? I vaguely recall that cabal turns off per-component builds if you have a custom setup - should we do the same thing? Maybe that would work more simply than trying to detect cabal-doctest
?
# to avoid confusion. It is not mapped by `builder/hspkg-builder.nix` to anything | ||
# you can build. Instead it is used internally when `configureAllComponents` | ||
# is set or for tests whe on `cabal-doctest` is in the `setup-depends` of the package. | ||
allComponent = mkOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The villain returns :p
Is this ever going to be merged? It works great, but is very tedious fixing all the merge conflicts! |
# Conflicts: # builder/comp-builder.nix
Almost completely unrelated to this PR, but there is now a new doctest-like program called Unlike I heard about it in cdepillabout/pretty-simple#82 (comment), which points to some discussion about it in ekmett/lens#958. I haven't actually played around with it, so I'm not sure how it will interact with haskell.nix. |
bors try |
tryTimed out. |
Hydra bors build did pass in the end. I'm going to merge this. It still has some things that could be improved, but it will help with other issues (for instance #1077). |
* Add test. * wip * Fix merge regression * Combine drv, drv.source and drv.dist for doctest * Skip cabal-doctests test when cross compiling * Add the --no-magic flag to the .cabal file for the cabal-doctests test. This appears to be necessary on OSX. The --no-magic flag stops the doctest executable from expanding path arguments, trying to locate the package db, etc. This shouldn't be necessary with cabal-doctest, since all the necessary options to pass to doctest are computed when running the Setup.hs script. See input-output-hk#427 (comment). * Fix cabal-doctest support * Skip cabal-doctest test plan cross compiling * More fixes for cabal-doctest * Skip cabal-doctest tests when cross compiling Co-authored-by: Moritz Angermann <[email protected]> Co-authored-by: Hamish Mackenzie <[email protected]> Co-authored-by: Hamish Mackenzie <[email protected]>
This would give preliminary support for running
cabal-doctest
tests. This would go some way towards fixing #388.This is currently building a example project using
cabal-doctest
. The tests in this project are able to be successfully built, and the tests are successfully run.https://github.com/cdepillabout/cabal-doctests-test
However, the code in this PR is still very hacky. I need to spend some more time cleaning up this code and getting it ready for a review. I also plan on testing it out on our codebase at work, which is hopefully complicated enough to reveal any rough edges.
The basic approach I am taking is to compile cabal-doctest tests as if they were
all
components (so that they have access everything they need), save the resultingdist/
directory, and then make it available again when actually running the tests.I'm also planning on adding a few actual tests here to haskell.nix for this functionality.