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

Handle non-ASCII filenames correctly on Windows #91

Merged
merged 5 commits into from Aug 31, 2016

Conversation

Projects
None yet
2 participants
@Blaisorblade
Copy link
Contributor

commented Aug 14, 2016

Not yet finished, but I've confirmed this code fixes the bug on Windows. EDIT: at least with GHC 7.10.3. I'll add a test and rebase.

Blaisorblade added some commits Aug 14, 2016

Open files through GHC intl-safe API (#90)
Fix #90. Idea: reuse low-level API from GHC internals, since it has
already tested low-level code to deal with non-ASCII filenames on
different platforms.
Drop redundant fclose_helper call
A null file pointer doesn't need closing; `fclose_helper` will indeed do
nothing on a null pointer.
Add testcase
I've ensured both parts of the test fail without the changes.

To ensure the whole test fails without the fixes, accented characters
must be in the path argument, not just in the current working directory.

@Blaisorblade Blaisorblade force-pushed the Blaisorblade:90-safe-intl-filename branch from 29825c4 to 5c2b6b2 Aug 14, 2016

@Blaisorblade

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2016

This PR is now ready for review, though on Windows I only tested on GHC 7.10.3 (with LTS-3 with relaxed version checks and LTS-6).

-- indicated via both 'rawOpenFlags' and 'openMode'.
openFile :: FilePath -> CInt -> String -> IO File
openFile file rawOpenFlags openMode = do
fd <- liftIO $ Posix.withFilePath file $ \file' ->

This comment has been minimized.

Copy link
@snoyberg

snoyberg Aug 25, 2016

Owner

I'm surprised that such a large change is needed. Is it insufficient to simply replace withCString with Posix.withFilePath and leave the rest of the code the same as it was before?

This comment has been minimized.

Copy link
@Blaisorblade

Blaisorblade Aug 29, 2016

Author Contributor

IIRC from two weeks ago, the first problem is that fopen doesn't do the right thing on Windows, because it takes normal characters instead of wide ones. This might be wrong, but I remember solving this by hand looked non trivial.
Instead, this code simply reuses the existing tested solution in GHC's libraries, which happens to provide a variant of open instead of fopen. To wit, as soon as this worked on Mac, it worked on Windows.
I look into it again if you want for more accurate details, but not tonight.
Also, sorry for the delay in replying, forgot y this email.

@snoyberg

This comment has been minimized.

Copy link
Owner

commented Aug 31, 2016

Sorry for the delay here, vacation definitely intervened.

I'm going to start by setting up appveyor so we can get some Windows testing going on this.

@snoyberg

This comment has been minimized.

Copy link
Owner

commented Aug 31, 2016

OK, I can confirm that due to wide characters the simple approach I mentioned does not work.

@snoyberg snoyberg merged commit 5c2b6b2 into snoyberg:master Aug 31, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@snoyberg

This comment has been minimized.

Copy link
Owner

commented Aug 31, 2016

I'll release to Hackage once the AppVeyor and Travis builds succeed.

@Blaisorblade Blaisorblade deleted the Blaisorblade:90-safe-intl-filename branch Sep 5, 2016

@Blaisorblade

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

Thanks for the merge... and apologies for kicking this hornet's nest (#92, #94, #95...).

@snoyberg

This comment has been minimized.

Copy link
Owner

commented Sep 6, 2016

I'm honestly blown away that you seem to have been the first person to
trigger this bug in Hackage, cabal-install, and Stack. Well done :)

On Mon, Sep 5, 2016 at 10:46 PM Paolo G. Giarrusso notifications@github.com
wrote:

Thanks for the merge... and apologies for kicking this hornet's nest (#92
#92, #94
#94, #95
#95...).


You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub
#91 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AADBBxHHX3KkzJrpYEwyU6XjTW4PcaOAks5qnHGFgaJpZM4Jj9Yg
.

snoyberg added a commit to commercialhaskell/stack that referenced this pull request Nov 20, 2016

Drop Data.Yaml.Extra #2491
This has been fixed upstream, see:
snoyberg/yaml#91

snoyberg added a commit to commercialhaskell/stack that referenced this pull request Nov 20, 2016

Drop Data.Yaml.Extra #2491
This has been fixed upstream, see:
snoyberg/yaml#91
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.