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

Fix support for non-ASCII module names #4939

Merged
merged 1 commit into from Jul 7, 2019
Merged

Conversation

qrilka
Copy link
Contributor

@qrilka qrilka commented Jul 6, 2019

Note: Documentation fixes for https://docs.haskellstack.org/en/stable/ should target the "stable" branch, not master.

Please include the following checklist in your PR:

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

Tested manually and added an integration test.
Fixes #4938

Another case when importing Data.ByteString.Char8 is an error...

@qrilka qrilka requested a review from snoyberg July 6, 2019 23:18
@snoyberg snoyberg merged commit bdbc92d into stable Jul 7, 2019
@snoyberg snoyberg deleted the non-ascii-module-name branch July 7, 2019 06:33
@borsboom
Copy link
Contributor

The new integration test seems to be failing on macOS on CI (see https://dev.azure.com/commercialhaskell/stack/_build/results?buildId=1945)

However, I just tried locally on my MacBook Pro and it seems to pass here, so not sure what's going on.

@neongreen
Copy link
Collaborator

It might depend on the file system. https://apple.stackexchange.com/questions/83935/unicode-normalization-for-filenames-and-copied-text-from-pdfs

Täst in the source code uses a precomposed ä, but Git on macOS might be creating a filename with a two-symbol ä (there's a setting in Git that controls that), or this might happen at the filesystem level (HFS+ and APFS have different rules).

@neongreen
Copy link
Collaborator

An easy way to find out if that is the case would be to try with ß instead.

borsboom added a commit that referenced this pull request Jul 11, 2019
This fixes an integration test failure (see
#4939 (comment))
@borsboom
Copy link
Contributor

I did a trial of upgrading CI to Mojave/10.14 (it was on High Sierra/10.13), and that may have fixed it. Doing the full test now with Mojave. Aside from that, indications so far are that this is not triggered in Stack itself but more likely in GHC's package handling (or maybe Cabal).

@borsboom
Copy link
Contributor

borsboom commented Jul 11, 2019

#4953, which upgrades CI to Mojave, looks to fix this on Azure (see https://dev.azure.com/commercialhaskell/stack/_build/results?buildId=1955).

@snoyberg did discover that the .cabal file generated by hpack is different when the test fails. On Azure it wrote Ta\204\136st while on @snoyberg's machine it wrote T\195\164st, so it's possible the problem is introduced by hpack reading the modules in a different way than the other tools and getting tripped up by macOS's unicode normalization.

@bjornbm
Copy link

bjornbm commented Jul 14, 2019

I didn't quite catch if the change you made fixes the problem on both HFS+ and APFS… it sounded like it is likely still broken on HFS+?? (I assume the High Sierra builds were on HFS+ and Mojave on APFS.)

FWIW elsewhere I have used unicode-transforms to normalize filenames in macOS to be able to work with them in a sane fashion (e.g., length filename returning 4 instead of 5 for something like "Täst"). The gist of it being something along the lines of:

import Data.Text.Normalize (normalize, NormalizationMode (NFC))

filenameToText :: FilePath -> Text
filenameToText = normalize NFC . pack

(There is a normalize that operates on bytestrings instead of Text as well.)

If hpack needs fixing I guess it would be somewhere around https://github.com/sol/hpack/blob/master/src/Hpack/Util.hs#L89.

@borsboom
Copy link
Contributor

I think this needs some more investigation. I actually thought my system was still on HFS+, but turns out upgrading to Mojave also changed my filesystem to APFS, so probably the same is true on Azure CI. So what you say about it still being broken with HFS+ is very plausible.

The original bug that was fixed was a problem on all operating systems and all filesystems, whereas what we're talking about is a different bug that only affects some macOS systems. It's probably better to open a separate issue.

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.

None yet

5 participants