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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.8.18.2

* Handle non-ASCII filenames correctly on Windows [#91](https://github.com/snoyberg/yaml/pull/91)

## 0.8.18.1

* Improve prettyPrintParseException when context is empty [#89](https://github.com/snoyberg/yaml/pull/89)
Expand Down
38 changes: 26 additions & 12 deletions Text/Libyaml.hs
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,15 @@ module Text.Libyaml

import Prelude hiding (pi)

import Data.Bits ((.|.))
import Foreign.C
import Foreign.Ptr
import Foreign.ForeignPtr
#if MIN_VERSION_base(4,7,0)
import Foreign.ForeignPtr.Unsafe
#endif
import Foreign.Marshal.Alloc
import qualified System.Posix.Internals as Posix

#if !MIN_VERSION_base(4,8,0)
import Control.Applicative
Expand Down Expand Up @@ -147,11 +149,10 @@ foreign import ccall unsafe "yaml_parser_set_input_file"
data FileStruct
type File = Ptr FileStruct

foreign import ccall unsafe "fopen"
c_fopen :: Ptr CChar
-> Ptr CChar
-> IO File

foreign import ccall unsafe "fdopen"
c_fdopen :: CInt
-> Ptr CChar
-> IO File
foreign import ccall unsafe "fclose"
c_fclose :: File
-> IO ()
Expand Down Expand Up @@ -503,6 +504,24 @@ decode bs =
c_yaml_parser_delete ptr
free ptr

-- XXX copied from GHC.IO.FD
std_flags, read_flags, output_flags, write_flags :: CInt
std_flags = Posix.o_NOCTTY
output_flags = std_flags .|. Posix.o_CREAT
read_flags = std_flags .|. Posix.o_RDONLY
write_flags = output_flags .|. Posix.o_WRONLY

-- | Open a C FILE* from a file path, using internal GHC API to work correctly
-- on all platforms, even on non-ASCII filenames. The opening mode must be
-- indicated via both 'rawOpenFlags' and 'openMode'.
openFile :: FilePath -> CInt -> String -> IO File
openFile file rawOpenFlags openMode = do
fd <- liftIO $ Posix.withFilePath file $ \file' ->
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Posix.c_open file' rawOpenFlags 0o666
if fd /= (-1)
then withCString openMode $ \openMode' -> c_fdopen fd openMode'
else return nullPtr

decodeFile :: MonadResource m => FilePath
#if MIN_VERSION_conduit(1, 0, 0)
-> Producer m Event
Expand All @@ -521,12 +540,9 @@ decodeFile file =
free ptr
throwIO $ YamlException "Yaml out of memory"
else do
file' <- liftIO
$ withCString file $ \file' -> withCString "r" $ \r' ->
c_fopen file' r'
file' <- openFile file read_flags "r"
if file' == nullPtr
then do
c_fclose_helper file'
c_yaml_parser_delete ptr
free ptr
throwIO $ YamlException
Expand Down Expand Up @@ -599,9 +615,7 @@ encodeFile filePath =
bracketP getFile c_fclose $ \file -> runEmitter (alloc file) (\u _ -> return u)
where
getFile = do
file <- withCString filePath $
\filePath' -> withCString "w" $
\w' -> c_fopen filePath' w'
file <- openFile filePath write_flags "w"
if file == nullPtr
then throwIO $ YamlException $ "could not open file for write: " ++ filePath
else return file
Expand Down
1 change: 1 addition & 0 deletions examples/Config.hs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import Data.Yaml (FromJSON(..), (.:))
import Text.RawString.QQ
import Data.ByteString (ByteString)
import Control.Applicative
import Prelude -- Ensure Applicative is in scope and we have no warnings, before/after AMP.

configYaml :: ByteString
configYaml = [r|
Expand Down
13 changes: 13 additions & 0 deletions test/Data/YamlSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import Control.Monad
import Control.Exception (try, SomeException)
import Test.Hspec
import Data.Either.Compat
import System.Directory (createDirectory)
import Test.Mockery.Directory

import qualified Data.Yaml as D
Expand Down Expand Up @@ -63,6 +64,7 @@ spec = do
describe "Data.Yaml" $ do
it "encode/decode" caseEncodeDecodeData
it "encode/decode file" caseEncodeDecodeFileData
it "encode/decode files with non-ASCII names" caseEncodeDecodeNonAsciiFileData
it "encode/decode strings" caseEncodeDecodeStrings
it "decode invalid file" caseDecodeInvalid
it "processes datatypes" caseDataTypes
Expand Down Expand Up @@ -317,6 +319,17 @@ caseEncodeDecodeFileData = withFile "" $ \fp -> do
out <- D.decodeFile fp
out @?= Just sample

caseEncodeDecodeNonAsciiFileData :: Assertion
caseEncodeDecodeNonAsciiFileData = do
let mySample = (object ["foo" .= True])
inTempDirectory $ do
createDirectory "accenté"
D.encodeFile "accenté/bar.yaml" mySample
out1 <- D.decodeFile "accenté/bar.yaml"
out1 @?= Just mySample
out2 <- D.decodeFile "test/resources/accenté/foo.yaml"
out2 @?= Just mySample

caseEncodeDecodeStrings :: Assertion
caseEncodeDecodeStrings = do
let out = D.decode $ D.encode sample
Expand Down
1 change: 1 addition & 0 deletions test/resources/accenté/foo.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
foo: true
4 changes: 3 additions & 1 deletion yaml.cabal
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: yaml
version: 0.8.18.1
version: 0.8.18.2
license: BSD3
license-file: LICENSE
author: Michael Snoyman <michael@snoyman.com>, Anton Ageev <antage@gmail.com>,Kirill Simonov
Expand All @@ -20,6 +20,7 @@ extra-source-files: c/helper.h
test/resources/foo.yaml
test/resources/bar.yaml
test/resources/baz.yaml
test/resources/accenté/foo.yaml
test/resources/loop/foo.yaml
test/resources/loop/bar.yaml
README.md
Expand Down Expand Up @@ -130,6 +131,7 @@ test-suite spec
, text
, aeson >= 0.7
, unordered-containers
, directory
, vector
, resourcet
, aeson-qq
Expand Down