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

Don't declare _GNU_SOURCE on Windows #552

Merged
merged 1 commit into from
May 1, 2023
Merged

Don't declare _GNU_SOURCE on Windows #552

merged 1 commit into from
May 1, 2023

Conversation

RyanGlScott
Copy link
Member

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.

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 kazu-yamamoto self-requested a review May 1, 2023 07:24
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.

Excellent!

@kazu-yamamoto kazu-yamamoto merged commit e52accb into haskell:master May 1, 2023
@kazu-yamamoto
Copy link
Collaborator

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

@RyanGlScott
Copy link
Member Author

Thanks!

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

@kazu-yamamoto
Copy link
Collaborator

Done.

@sol
Copy link
Member

sol commented May 2, 2023

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 sol/hpack#548 (comment))

@andreasabel
Copy link
Member

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
Copy link
Member

sol commented May 2, 2023

@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
Copy link
Member

@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
Copy link
Member Author

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

@sol
Copy link
Member

sol commented May 2, 2023

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
Copy link
Member

sol commented May 2, 2023

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Member

sol commented May 8, 2023

@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
Copy link
Collaborator

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

@kazu-yamamoto
Copy link
Collaborator

@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
Copy link
Member

@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
Copy link
Collaborator

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

@andreasabel
Copy link
Member

@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
Copy link
Collaborator

I need to increment x-revision, sigh.

@sol
Copy link
Member

sol commented May 12, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

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

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.

Don't declare _GNU_SOURCE on Windows
4 participants