Skip to content

Use cabal-doctest only during CI #365

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

Merged
merged 2 commits into from
Jan 15, 2019

Conversation

hvr
Copy link
Member

@hvr hvr commented Jan 11, 2019

Proof of concept for using build-type:custom only during CI

@hvr hvr requested a review from kazu-yamamoto January 11, 2019 23:59
@hvr hvr mentioned this pull request Jan 12, 2019
@kazu-yamamoto
Copy link
Collaborator

You should also take care of AppVeyor. Can the shell script run on Windows?
Or should we implement and use a Haskell script?

@hvr
Copy link
Member Author

hvr commented Jan 12, 2019

@kazu-yamamoto I intentionally used a shell script so we can run it via bash (you'll notice there's already one bash call in the appveyor script) on Appveyor too

PS: I'm on it!

@hvr hvr force-pushed the pr/custom-doctest-only-ci branch from 608daf7 to aa2f001 Compare January 12, 2019 13:16
Moreover, this removes the superflous manual
`cabal install hspec-discover cabal-doctest`
invocations.
@hvr hvr force-pushed the pr/custom-doctest-only-ci branch from aa2f001 to d9154d4 Compare January 12, 2019 13:28
@hvr
Copy link
Member Author

hvr commented Jan 12, 2019

@kazu-yamamoto done; you should take a look now. I also noticed there was some non-idiomatic manual installation lib:cabal-doctest which made no sense (as new-build wouldn't be able to see it anyway)

kazu-yamamoto
kazu-yamamoto previously approved these changes Jan 12, 2019
Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@kazu-yamamoto
Copy link
Collaborator

Q1) Don't we need a default Setup.hs?
Q2) On macOS, bash enable_doctest.sh results in:

sed: 1: "network.cabal": extra characters at the end of n command
sed: 1: "network.cabal": extra characters at the end of n command
sed: 1: "network.cabal": extra characters at the end of n command

@hvr
Copy link
Member Author

hvr commented Jan 12, 2019

Q1) Don't we need a default Setup.hs?

They're only needed for compliant source-tarballs -- and cabal sdist will include a default Setup.hs automatically; in other words, it's redundant (and somewhat error-prone as there's no checks in place to verify that for build-type:{Simple,Configure} the Setup.hs contents is consistent) to place an explicit default Setup.hs into Git. The only case where you need to provide an explicit one is for build-type:Custom.

Q2) On macOS, bash enable_doctest.sh results in:

Right... the sed invocations assumed GNU sed; but on macOS you have BSD sed which lacks the some of the extensions I used...

Do you need the script to support BSD? If so, I can find an alternative way to perform the transformations.

@kazu-yamamoto
Copy link
Collaborator

I would suggest to prepare network.cabal.doctest and copy it to network.cabal instead using sed.

@kazu-yamamoto
Copy link
Collaborator

@hvr
Copy link
Member Author

hvr commented Jan 12, 2019

I would suggest to prepare network.cabal.doctest and copy it to network.cabal instead using sed.

the problem with that approach is that you need to keep the two files manually synced up which can become annoying to do.

Also, I've tweaked the sed invocation a bit to be POSIX compatible.

@llelf
Copy link
Member

llelf commented Jan 12, 2019

@hvr s/bash/sh/ then? :)

@hvr
Copy link
Member Author

hvr commented Jan 12, 2019

@llelf the script is already a #!/bin/sh script...? :-)

@llelf
Copy link
Member

llelf commented Jan 12, 2019

@hvr yes, but you run it with bash ;)
Think of those poor *BSD systems!

@kazu-yamamoto kazu-yamamoto self-requested a review January 15, 2019 05:13
Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

kazu-yamamoto added a commit to kazu-yamamoto/network that referenced this pull request Jan 15, 2019
@kazu-yamamoto kazu-yamamoto merged commit 6e62265 into haskell:master Jan 15, 2019
@kazu-yamamoto
Copy link
Collaborator

Merged.
Thank you for your contribution!

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.

3 participants