Fix encoding issues: use with-utf8 library and other tricks
#595
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @f-f ! |
|
@gibranrosa thank you for trying this out! It sounds like we're a little bit closer to a solution. Some questions:
|
|
|
@gibranrosa thank you! Could you post here the output of |
|
spago -V build: |
|
Just to register that I've found a Windows setting to make UTF-8 the default, and thus all works fine, but it is a Beta setting yet, and there is known bugs with this enabled. It's in Control Panel > Region > Administrative Here is the SuperUser answer explaining: |
|
I was narrowing down the error putting logDebugs and apparently the exception occurs at Purs.hs, line 135, in the call to shellStrictWithErr from Turtle library. Below the logs I put and below that the Spago buid log: versionImpl :: Text -> Spago (Either Text Version.SemVer)
versionImpl purs = do
logDebug "passed Purs.compile - Enters versionImpl" -- <--- Terminates here
shellStrictWithErr (purs <> " --version") empty >>= \case
(ExitSuccess, out, _err) -> do
logDebug "passed Purs.compile - versionImpl"
let versionText = headMay $ Text.split (== ' ') out
parsed = versionText >>= (hush . Version.semver)
logDebug "passed Purs.compile - versionImpl - pass versionText"
pure $ case parsed of
Nothing -> Left $ Messages.failedToParseCommandOutput (purs <> " --version") out
Just p -> Right p
(_, _out, _err) -> do
logDebug "passed Purs.compile - versionImpl - Err case"
pure $ Left $ "Failed to run '" <> purs <> " --version'" |
|
Continuing the debugging, I've found setting the encoding to latin1 (win-1252) right before shellStrictWithErr and resetting to UTF8 right after works too: versionImpl :: Text -> Spago (Either Text Version.SemVer)
versionImpl purs = do
logDebug "passed Purs.compile - Enters versionImpl"
liftIO $ GHC.IO.Encoding.setLocaleEncoding GHC.IO.Encoding.latin1
shellStrictWithErr (purs <> " --version") empty >>= \case
(ExitSuccess, out, _err) -> do
liftIO $ GHC.IO.Encoding.setLocaleEncoding GHC.IO.Encoding.utf8
logDebug "passed Purs.compile - versionImpl"
let versionText = headMay $ Text.split (== ' ') out
parsed = versionText >>= (hush . Version.semver)
logDebug "passed Purs.compile - versionImpl - pass versionText"
pure $ case parsed of
Nothing -> Left $ Messages.failedToParseCommandOutput (purs <> " --version") out
Just p -> Right p
(_, _out, _err) -> do
logDebug "passed Purs.compile - versionImpl - Err case"
pure $ Left $ "Failed to run '" <> purs <> " --version'"
|
|
@gibranrosa great debugging! It looks like |
|
@f-f Thanks! Here is the commit in my fork: gibranrosa@b446506 versionImpl purs = do
Turtle.Bytes.shellStrictWithErr (purs <> " --version") empty >>= \case
(ExitSuccess, out, _err) -> do
let versionText = headMay $ Text.split (== ' ') (Text.Encoding.decodeUtf8With lenientDecode out)
parsed = versionText >>= (hush . Version.semver)
pure $ case parsed of
Nothing -> Left $ Messages.failedToParseCommandOutput (purs <> " --version") (Text.Encoding.decodeUtf8With lenientDecode out)
Just p -> Right p
(_, _out, _err) -> do
pure $ Left $ "Failed to run '" <> purs <> " --version'"
|
3 tasks
|
I tried to compile my application kakkun61/elm-license-checker@5517b19, and then I got the following log. My environment is:
PowerShell version details |
|
@kakkun61 Could you try the build with the fix I did with the help from @f-f ? |
It works on my pc with code point 936 (GBK). |
|
Oh, I had done with this #595. I tried https://github.com/gibranrosa/spago/releases/tag/0.14-windows10-fix, and I got no error! |
|
Alright, it looks like we're on a good path! Thanks @gibranrosa for the fix (which I cherry-picked in this branch, but feel free to PR it already to the master branch) and to @kakkun61 and @wangbinyq for testing 🙂 I'm going to try changing the Windows build to run with Japanese encoding, to see if we can spot any tests failing. In the meanwhile you could also try running |
|
I've ran the tests and there is some errors that does not look related to this change, right? |
|
@gibranrosa that's right, it doesn't look like the failures depend on these changes - ..however I wonder why they do happen 🤔 |
f-f
changed the title
with-utf8 librarywith-utf8 library + a few other tricks
f-f
changed the title
with-utf8 library + a few other trickswith-utf8 library and other tricks
f-f
changed the title
with-utf8 library and other trickswith-utf8 library and other tricks
|
I switched the Windows tests to use Japanese encoding, and they all pass now! I'll merge and cut a new release with the fix, thanks everyone for the great help 🙂 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thanks to @jamesdbrock yesterday I discovered the
with-utf8library, which tagline is "Get your IO right on the first try"There's an excellent blogpost about the rationale and the implementation, and it looks like putting the library into use might fix all the encoding problems we're facing here in Spago when dealing with encodings different than UTF8.
@bbarker @kakkun61 @gibranrosa could you try building Spago from this branch to see if this fixes your issues?
Fix #533
Fix #507
Close #576