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

Doctests new build #701

Merged
merged 2 commits into from
Jan 11, 2017
Merged

Doctests new build #701

merged 2 commits into from
Jan 11, 2017

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Jan 11, 2017

No description provided.

@ekmett
Copy link
Owner

ekmett commented Jan 11, 2017

Can you add a preamble to the Setup.lhs like:

#ifndef MIN_VERSION_Cabal
#define MIN_VERSION_Cabal(X,Y,Z) 0
#endif

This way when run on older cabal versions it'll be more robust.

@phadej
Copy link
Collaborator Author

phadej commented Jan 11, 2017

@ekmett, I did. Let's see what travis says. I guess setup-depends: Cabal >= 1.24 is ok, as it really helps if people move to recent cabal.

Tested locally: can run doctests with GHC-7.8.4 and GHC-8.0.1 when using cabal-install-head new-build

@phadej phadej force-pushed the doctests-new-build branch 2 times, most recently from 0aa054d to d66fd5e Compare January 11, 2017 10:30
@ekmett ekmett merged commit 1f31928 into ekmett:master Jan 11, 2017
@phadej phadej deleted the doctests-new-build branch January 11, 2017 11:10
@RyanGlScott
Copy link
Collaborator

Thanks for doing this, @phadej! Since there's a number of other packages that use a similar Setup.hs script (and since we don't yet have cabal doctest), I'd like to adapt this for other packages. Some questions I have after skimming the changes:

  1. Does this really require cabal-install-head as the minimum?
  2. Is there any logic here that's lens-specific that couldn't just be copy-pasted into something like, say, distributive's Setup.hs?

@phadej
Copy link
Collaborator Author

phadej commented Jan 11, 2017

@RyanGlScott For 2: The lens diagram copying code is something none of other packages need: https://github.com/ekmett/lens/blob/master/Setup.lhs#L42-L44

For 1: I'm not sure, it might work with cabal-install-1.24 new-build too, too lazy to check what https://github.com/haskell/cabal/blame/675d4f1625edc6e3d3a68448cbc72a7931a590ea/cabal-install/Distribution/Client/SetupWrapper.hs#L810-L821 looks like.

But anyhow, cabal-install-1.24 new-build is/was a technology preview, and I wouldn't use it if I can pick HEAD (with HVR's ppa repo it's easy, if you are on Ubuntu; I'm also planning to provide nightly OSX builds of cabal, but run out of time setting that up: now I said it publicly, so I guess it will happen sooner ;) )

@RyanGlScott
Copy link
Collaborator

  1. Oh, I didn't read the new-build part of the title. But lens's Travis script isn't currently using new-build, right? It's still using configure/build/test for the moment. So as long as you stick to the old-style build process, does it matter what version of cabal-install you use?

  2. Good point, I'll make sure not to use the diagram-copying part elsewhere.

@bennofs
Copy link
Collaborator

bennofs commented Feb 2, 2017

@phadej does this work with older cabal-install versions? I'd assume it requires a cabal-install that knows about custom-setup, or parsing of the cabal file will fail.

@phadej
Copy link
Collaborator Author

phadej commented Feb 2, 2017

parsing doesn't fail, cabal just says "unknown section". You can see it e.g. from https://travis-ci.org/ekmett/distributive/builds/197339532

@bennofs
Copy link
Collaborator

bennofs commented Feb 2, 2017

Oh cool, I had assumed that this was less backwards-compatible than it actually is. This is nice, thanks!

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.

None yet

4 participants