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

WINIO: Add support for WINIO to Cabal. #6848

Closed
wants to merge 1 commit into from

Conversation

Mistuke
Copy link
Collaborator

@Mistuke Mistuke commented May 25, 2020


This makes openNewBinaryFile use openBinaryTempFileWithDefaultPermissions on Windows when the new I/O manager is used. It's my understanding that the only difference between the two is the file mode? 666 vs 600. However base has supprted openBinaryTempFileWithDefaultPermissions for a while now with 666 as permissions.

Without this any calls to openNewBinaryFile in GHC 8.12 and the new I/O manager enabled will cause a panic.

Related to #6847

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog (add file to changelog.d directory).
  • The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

@phadej
Copy link
Collaborator

phadej commented Jun 10, 2020

I'm not sure what happened with Quick jobs.

I noticed that "Add Cabal, GHC... to GitHub iActions mages* issues was resolved, so it might or might not related.

@@ -36,10 +40,17 @@ import qualified System.Posix
-- TODO: This file should probably be removed.

-- This is a copy/paste of the openBinaryTempFile definition, but
-- if uses 666 rather than 600 for the permissions. The base library
-- needs to be changed to make this better.
-- if uses 666 rather than 600 for the permissions. Newer versions
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/if/it/ ?

@phadej
Copy link
Collaborator

phadej commented Jun 11, 2020

The Quick jobs failed, (#6890 will fix them soon).

@phadej
Copy link
Collaborator

phadej commented Jun 11, 2020

@Mistuke Could you fix a typo and rebase on top of master. Then we can merge this, I think.

@Mistuke Mistuke force-pushed the wip/new-io-manager-support branch from f996a72 to 6ec7bdb Compare June 12, 2020 16:53
@Mistuke
Copy link
Collaborator Author

Mistuke commented Jun 14, 2020

@phadej not sure what that CI failure is.. seems to have failed checkout?

@phadej
Copy link
Collaborator

phadej commented Jun 16, 2020

CI was green in #6908 (with the same commit).

I merged it. If there is still something please open a followup!

@phadej phadej closed this Jun 16, 2020
@phadej phadej added this to the 3.4.0.0-rc1 milestone Jul 10, 2020
@phadej phadej mentioned this pull request Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants