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

Fix sendAll on Windows #72

Conversation

Projects
None yet
2 participants
@adinapoli-iohk
Copy link
Contributor

commented Aug 30, 2017

This PR fixes some lookup problems Windows users were facing when
using the library. The culprit (as confirmed by some tests) was in
the implementation of sendAll, that on Windows was defined as part of
the code of this library, but this is not necessary anymore unless an
old version of GHC (< 7) is being used.

For modern environments, Network.Socket.ByteString.sendAll is all we
need.

adinapoli-iohk added some commits Aug 30, 2017

Fix sendAll on Windows.
This commit fixes some lookup problems Windows users were facing when
using the library. The culprit (as confirmed by some tests) was in
the implementation of `sendAll`, that on Windows was defined as part of
the code of this library, but this is not necessary anymore unless an
old version of GHC (< 7) is being used.

For modern environments, Network.Socket.ByteString.sendAll is all we
need.
@adinapoli-iohk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2017

@kazu-yamamoto

This comment has been minimized.

Copy link
Owner

commented Aug 31, 2017

The dns library supports only three major versions.
Do you have any strong reason to support GHC 6.x?

@adinapoli-iohk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

@kazu-yamamoto hey!

Sorry, I think the mistake was mine for naming the CPP constant poorly: when I saw the expression:

#if __GLASGOW_HASKELL__ < 709

I must have thought we were implying some oldish version of GHC like 6.x, thus my ill-named GHC6 constant. What about I rename that simply GHC709?

Do you have any strong reason to support GHC 6.x?

I do not! In reality I was just taken in by the conditional build-depends in the cabal manifest:

  if impl(ghc >= 7)
    Build-Depends:      base >= 4 && < 5
                      , attoparsec
                      , binary
                      , bytestring
                      , conduit >= 1.1
                      , conduit-extra >= 1.1
                      , containers
                      , iproute >= 1.3.2
                      , mtl
                      , network >= 2.3
                      , random
                      , resourcet
                      , safe == 0.3.*

As far as I'm concerned, I'm perfectly happy to drop those and simply remove the custom sendAll once for all 😉

@adinapoli-iohk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2017

@kazu-yamamoto To keep this "hot": Which are the changes you would like to see applied to this PR to make it mergeable into master? Thank you! 😉

@kazu-yamamoto

This comment has been minimized.

Copy link
Owner

commented Sep 2, 2017

What about I rename that simply GHC709?

Good.

Would you clean up this PR, rebase onto master and do push -f?
I would like to merge it.

P.S. I setup AppVoyer which provides GHC 7.10.3.
Do you know how to use multiple GHC versions on AppVoyer?

@kazu-yamamoto

This comment has been minimized.

Copy link
Owner

commented Sep 2, 2017

Wow. Your patch fixes the test failure on AppVoyer. Great.
So, I will treat this patch by myself.

@adinapoli-iohk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2017

Wow. Your patch fixes the test failure on AppVoyer. Great.

@kazu-yamamoto Aye, and we are successfully using it on production for DNS lookups on Windows 😉

Do you know how to use multiple GHC versions on AppVoyer?

I don't know if stack is your cup of tea, but using it on Appveyor for multiple versions of GHC is quite easy! For example: https://github.com/lpeterse/haskell-socket/blob/master/appveyor.yml

So, I will treat this patch by myself.

Great, ok! I won't touch this patch any further unless told otherwise, please shout if I need to do anything else. 😛

@kazu-yamamoto

This comment has been minimized.

Copy link
Owner

commented Sep 4, 2017

Merged. And multiple GHCs are supported on AppVeyor.
Thank you for your contribution!

@adinapoli-iohk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2017

Thank you very much, @kazu-yamamoto !

Can I be cheeky and ask for a new Hackage release some time this week? 🙏 😁

@kazu-yamamoto

This comment has been minimized.

Copy link
Owner

commented Sep 4, 2017

The next release of master is a major. So, we should be carefully.

But if you want to merge your fix into to the stable-2.0 branch, I can release a 2.0.13 very quickly.
What do you think?

@adinapoli-iohk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2017

But if you want to merge your fix into to the stable-2.0 branch, I can release a 2.0.13 very quickly. What do you think?

Hey! Just to be sure to be on the same page, can I summarise what you are saying? You are telling me that releasing the current master is a major release, so it shouldn't be rushed, but that we can still release 2.0.13 relative quickly if we drop the default DNS thing and we include only:

  • The RetryLimitExceeded patch;
  • The sendAll fix patch;

Am I right?

@kazu-yamamoto

This comment has been minimized.

Copy link
Owner

commented Sep 4, 2017

Yes!

@adinapoli-iohk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2017

Works for me!

@kazu-yamamoto kazu-yamamoto referenced this pull request Sep 5, 2017

Closed

Version 2.0.13 #76

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.