haskell / network

Low-level networking interface
http://hackage.haskell.org/package/network
Other
325 stars 187 forks source link

Don't declare `_GNU_SOURCE` on Windows #552

Closed RyanGlScott closed 1 year ago

RyanGlScott commented 1 year ago

Doing so will cause network to have an undeclared dependency against the mingwex library on Windows, which can cause issues with GHC's runtime linker. (See https://gitlab.haskell.org/ghc/ghc/-/issues/23309 for the full story.) Thankfully, there is no particular need to define _GNU_SOURCE on Windows in the first place, so I have guarded its definition with CPP.

Fixes #551.

kazu-yamamoto commented 1 year ago

Thanks as always. If you wish, I will release a new version.

RyanGlScott commented 1 year ago

Thanks!

I would definitely appreciate a new Hackage release, as this will fix a pretty serious Windows-only issue.

kazu-yamamoto commented 1 year ago

Done.

sol commented 1 year ago

Do we also want Hackage revisions for old versions of network (< 3.1.2.9)? Something like:

if impl(ghc >= 9.4.5) && os(windows)
  buildable: false

(see https://github.com/sol/hpack/pull/548#issuecomment-1530830218)

andreasabel commented 1 year ago

Do we also want Hackage revisions for old versions of network (< 3.1.2.9)? Something like:

if impl(ghc >= 9.4.5) && os(windows)
  buildable: false

@sol : Unfortunately, AFAIK, you cannot add a conditional in a revision, Hackage will refuse your revision attempt. There also seems no precise way to use the base proxy for GHC, because both 9.4.4 and 9.4.5 ship with the same base version 4.17.1.0.

However, could we nevertheless constrain any network < 3.1.2.9 with base < 4.17, so that with any GHC >= 9.4 one has to use network >= 3.1.2.9? @RyanGlScott @kazu-yamamoto

sol commented 1 year ago

@andreasabel would doing that (constraining the base version indiscriminately) potentially break existing freeze files? Or does cabal ignore revisions when there is a freeze file (as it probably should)?

andreasabel commented 1 year ago

@andreasabel would doing that (constraining the base version indiscriminately) potentially break existing freeze files? Or does cabal ignore revisions when there is a freeze file (as it probably should)?

Sorry, I don't know, I do not use cabal freeze (as it does not support multi-GHC settings).

RyanGlScott commented 1 year ago

Freeze files pin the time of Hackage index state when they were created (example), so subsequent revisions will not affect the freeze file.

sol commented 1 year ago

Freeze files pin the time of Hackage index state when they were created

That's great!

Given this, I think indiscriminately adding base < 4.17, while still not great, is acceptable. It's also, as far as I understand, our only real option to address this problem at the root.

sol commented 1 year ago

However, could we nevertheless constrain any network < 3.1.2.9 with base < 4.17, so that with any GHC >= 9.4 one has to use network >= 3.1.2.9?

So yes, all for it 👍

andreasabel commented 1 year ago

However, could we nevertheless constrain any network < 3.1.2.9 with base < 4.17, so that with any GHC >= 9.4 one has to use network >= 3.1.2.9?

So yes, all for it 👍

@kazu-yamamoto Do you wish to do this?

kazu-yamamoto commented 1 year ago

Sorry for the delay. I wan on vacation. I need to admit that I cannot follow this conversation completely. If I should modify network.cabal, please send a PR. If I should modify the metadata on Hackage, please tell me instructions.

andreasabel commented 1 year ago

The suggestion is to add an upper bound for all older network packages on hackage.

constrain any network < 3.1.2.9 with base < 4.17, so that with any GHC >= 9.4 one has to use network >= 3.1.2.9?

This could be done with the hackage-cli tool.

  1. Do you think this is a good idea?
  2. Would you make these revisions? (Otherwise, if you authorize me, I can also do it.)
sol commented 1 year ago

@kazu-yamamoto the alternative would be that every user of network would need to add something along the lines of https://github.com/mpilgrem/hpack/blob/2187fa8f822f4fd485df47511548df9d1a341e2c/package.yaml#L38-L41

(that's at least how I understand the situation)

kazu-yamamoto commented 1 year ago

I agree. Let me remind how to use hackage-ci. This would take time a little.

kazu-yamamoto commented 1 year ago

@andreasabel @sol Would you give a look at https://github.com/kazu-yamamoto/network-cabal? If you like this, I will do hackage-cli push-cabal.

andreasabel commented 1 year ago

@andreasabel @sol Would you give a look at kazu-yamamoto/network-cabal? If you like this, I will do hackage-cli push-cabal.

I gave it a brief look, but I cannot really check this efficiently since it does not display the diff...

If you do hackage-cli push-cabal *.cabal without --publish, it gives you the diff for each file without committing the revision, so you can give it a brief sanity check. (And then repeat with --publish.)

kazu-yamamoto commented 1 year ago

@andreasabel Each commit shows you the diffs. Isn't it good enough?

andreasabel commented 1 year ago

@andreasabel Each commit shows you the diffs. Isn't it good enough?

Apologies, indeed. I looked through the commits now, and they all set the < 4.17 bound correctly, AFAICS. (The commit messages sometimes say < 5, which confused me a bit in the beginning, but that does not matter.)

So, LGTM!

kazu-yamamoto commented 1 year ago

I need to increment x-revision, sigh.

sol commented 1 year ago

Kazu, thanks for taking care of this 🙏😊

I need to increment x-revision, sigh.

I think I never had to do this explicitly. Shouldn't the tooling already take care of it somehow?

kazu-yamamoto commented 1 year ago

OK. I found --incr-rev. I removed all unchanged cabal files to avoid errors. And I think I have done it. Please check the hackage.

kazu-yamamoto commented 1 year ago

For record, I wrote README.md: https://github.com/kazu-yamamoto/network-cabal