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

Relax upper bounds #179

Merged
merged 5 commits into from
Jan 28, 2017
Merged

Relax upper bounds #179

merged 5 commits into from
Jan 28, 2017

Conversation

phadej
Copy link
Contributor

@phadej phadej commented Jan 13, 2017

Is it ok to add released packages to Stackage, so we get pinged when bounds are restrictive? hackage-security is anyhow there, as it's depended by cabal-install.

@ezyang
Copy link
Contributor

ezyang commented Jan 16, 2017

Hmm, is there a way to do this patch with less CPP?

@phadej
Copy link
Contributor Author

phadej commented Jan 16, 2017

If it's ok to have http-client >= 0.5

@ezyang
Copy link
Contributor

ezyang commented Jan 16, 2017

I don't really have the final say for a package like this, but my preference is that we create a Compat package that centralizes compatibility wrappers around API differences between the old and new versions.

@phadej
Copy link
Contributor Author

phadej commented Jan 16, 2017

It'S not realised example package, so i'd rather not over-engineer. I'll be ok with CPP or tighter lower bound, otherwise the value of an example degrares

@ezyang
Copy link
Contributor

ezyang commented Jan 16, 2017

Oops, I didn't realize it was an example package. In that case, updating the version bound to the newest one seems like a clear winner.

@edsko
Copy link
Contributor

edsko commented Jan 17, 2017

Moving to http-client-0.5 for the -http-client bindings seems perfectly fine to me, so let's get rid of the CPP and just change the bounds?

@edsko
Copy link
Contributor

edsko commented Jan 17, 2017

I'm okay with leaving the bounds too by the way, actually. That package is meant to show how http-client can be used together with hackage-security in case, say, Stackage wanted to use it. But I don't know if it's actually being used. With the CPP it would demonstrate I suppose how either version works, and LTS 7 at least still uses http-client 0.4. @dcoutts , opinions?

@hvr
Copy link
Member

hvr commented Jan 18, 2017

My opinion here is mostly to encourage reducing CPP usage (and tbh I'm quite surprised Stackage LTS is still holding back http-client-0.5.* adoption which was released back on 05-07-2016 and thereby causing a couple of knock on problems in the process...) :-)

@phadej
Copy link
Contributor Author

phadej commented Jan 18, 2017

LTS-7 series is released on 2016-09-14. To Stackage's defense, they didn't tried to bump too much until then, but cut GHC-8.0.1 based series from what they got. Right after that, the blocking packages were removed, so e.g. http-client-0.5 is in nightly-2016-09-15

This diff is an interesting datapoint: https://www.stackage.org/diff/nightly-2016-09-14/nightly-2016-09-15 (there was packages blocking on aeson-1, http-client-0.5 and haskell-src-exts-0.18)


But, remove CPP or leave it?

@edsko
Copy link
Contributor

edsko commented Jan 19, 2017

If stack decides to support hackage-security, would they want backwards compatibility with http-client-0.4? If the answer is no, then let's get rid of the CPP. If the answer is yes, then let's keep it.

@phadej
Copy link
Contributor Author

phadej commented Jan 19, 2017

newest release of stack depends on http-client >= 0.5. But to clarify, the http-client dependency is in unreleased example code hackage-security-http-client, not hackage-security.

I'll amend this PR shortly

@phadej
Copy link
Contributor Author

phadej commented Jan 20, 2017

FYI

Since the hackage-security-http-client package isn't yet on Hackage, I inlined that module. Also, manually specified indices will be able to opt in or out of Hackage Security for downloads, so compatibility with existing non-HS indices will be retained.

commercialhaskell/stack#2940

@phadej
Copy link
Contributor Author

phadej commented Jan 20, 2017

Please merge, or say I can remove CPP in favour of http-client >= 0.5.

So small change "stuck in committee"

@edsko
Copy link
Contributor

edsko commented Jan 20, 2017

The only purpose of hackage-security-http-client from the very beginning was to make it easier for stack to use hackage-security. If backwards compatibility doesn't matter to stack, let's get rid of the CPP.

Incidentally, overlap with #180?

@phadej
Copy link
Contributor Author

phadej commented Jan 20, 2017

I'll integrate @snoyberg's patch into this, the makeHttpLib.

@phadej
Copy link
Contributor Author

phadej commented Jan 20, 2017

Integrated #180 changes.

@phadej
Copy link
Contributor Author

phadej commented Jan 20, 2017

http-client-0.5. has bytestring >= 0.10, so --constraint="bytestring installed", doesn't work for GHC-7.4, as it bundles bytestring-0.9.2.1 https://ghc.haskell.org/trac/ghc/wiki/Commentary/Libraries/VersionHistory

So it's either "keep CI simple" or "support http-client-0.4". Ping @edsko.

@edsko
Copy link
Contributor

edsko commented Jan 21, 2017

The bytestring installed constraint as well as support for older ghc is important for the main hackage-security library, but neither the contraint nor support for older ghc is important for the -http-client library.

@phadej phadej force-pushed the bounds branch 7 times, most recently from 57bfd64 to b6ebace Compare January 22, 2017 12:21
@phadej
Copy link
Contributor Author

phadej commented Jan 22, 2017

Finally green. I miss shelly or something like that.

@phadej phadej force-pushed the bounds branch 2 times, most recently from 42a9203 to d7775ec Compare January 27, 2017 11:16
@phadej
Copy link
Contributor Author

phadej commented Jan 27, 2017

ping @edsko

Copy link
Member

@hvr hvr left a comment

Choose a reason for hiding this comment

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

please drop the ghc-7.4.2 exception

@@ -35,6 +35,7 @@ before_install:
- HC=${CC}
- unset CC
- PATH=/opt/ghc/bin:/opt/ghc-ppa-tools/bin:$PATH
- if [ $GHCVER = "7.4.2" ]; then mv cabal.ghc-7.4.2.project cabal.project; fi
Copy link
Member

Choose a reason for hiding this comment

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

This most likely isn't needed and we should avoid it. While Stack may have problems coping with that configuration I honestly doubt that Cabal has any issues with this. Have you run into an actual problem? Also it'd defeat the purpose of CI if we exclude configurations even though they're declared to be compatible via build-depends.

@phadej phadej force-pushed the bounds branch 8 times, most recently from fafb3b6 to 525f340 Compare January 27, 2017 17:06
- constrained hackage-security
- everything together
@phadej
Copy link
Contributor Author

phadej commented Jan 27, 2017

Green again.

Copy link
Member

@hvr hvr left a comment

Choose a reason for hiding this comment

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

I was planning to do exactly what you did (i.e. additionally testing smaller subsets of the install-plan) ... now I don't have to anymore :-) 👍

# build all packages and run testsuite

# first build just hackage-security with installed constraints, with and without tests.
# silly yaml, seeing : colon
Copy link
Member

Choose a reason for hiding this comment

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

yeah... just one of many YAML silliness ;-)

@hvr
Copy link
Member

hvr commented Jan 28, 2017

I'll try to code-review to make sure that the relaxation to the new major versions of optparse-applicative and directory are sound; I think I can safely assume that http-client-0.5 compatibility is sound since it was a patch contributed by http-client's author

@hvr hvr merged commit 42dc8f3 into haskell:master Jan 28, 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.

5 participants