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

cli: Fix compilation without LANG/LC_ALL set #849

Merged
merged 1 commit into from Aug 24, 2018

Conversation

Projects
None yet
2 participants
@zarybnicky

zarybnicky commented Jul 28, 2018

Embedded files are now assumed to be UTF-8 independently of the system locale. This also replaces the file-embed dependency with Hledger.Cli.TH, where embedStringFile has been changed to assume UTF-8 (utf8_bom).

Compilation without a locale (e.g. in nix-shell --pure or docker run -it fedora:28 bash as @talex5 mentioned) now works without a problem.

I've also changed the nix: sections of stack.yamls to use pure nix shells, and added the required dependencies of happy/alex - stack build --nix works for all build configs. (I'm on NixOS, so I can't actually develop without --nix, and compiling in a pure shell is also a nice bonus.)

Fixes #420, #813. This is also a better solution to the closed #669.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Aug 2, 2018

Owner

@zarybnicky, thanks for making the build more robust. Could you perhaps:

  • submit embedStringFileUtf8 upstream - it seems like a useful feature.
  • include the notes above in the commit description(s)
  • separate the nix/stack updates into their own commit (or, mention why they belong with this one)
Owner

simonmichael commented Aug 2, 2018

@zarybnicky, thanks for making the build more robust. Could you perhaps:

  • submit embedStringFileUtf8 upstream - it seems like a useful feature.
  • include the notes above in the commit description(s)
  • separate the nix/stack updates into their own commit (or, mention why they belong with this one)
@zarybnicky

This comment has been minimized.

Show comment
Hide comment
@zarybnicky

zarybnicky Aug 8, 2018

I've opened an upstream issue, snoyberg/file-embed#27. I've also added to the commit description, and removed the Nix-related changes (I might open another pull request later, it's nothing urgent, just a nice-to-have).

zarybnicky commented Aug 8, 2018

I've opened an upstream issue, snoyberg/file-embed#27. I've also added to the commit description, and removed the Nix-related changes (I might open another pull request later, it's nothing urgent, just a nice-to-have).

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Aug 16, 2018

Owner

Thanks @zarybnicky. I guess we should do a shorter workaround with less code duplication following the suggestions on that issue.

Owner

simonmichael commented Aug 16, 2018

Thanks @zarybnicky. I guess we should do a shorter workaround with less code duplication following the suggestions on that issue.

@zarybnicky zarybnicky closed this Aug 24, 2018

@zarybnicky

This comment has been minimized.

Show comment
Hide comment
@zarybnicky

zarybnicky Aug 24, 2018

Oops, didn't mean to click that.

zarybnicky commented Aug 24, 2018

Oops, didn't mean to click that.

@zarybnicky zarybnicky reopened this Aug 24, 2018

@zarybnicky

This comment has been minimized.

Show comment
Hide comment
@zarybnicky

zarybnicky Aug 24, 2018

Alright, third time's the charm. This version just replaces embedStringFile with embedFile, and uses Data.ByteString functions to print out the docs.

zarybnicky commented Aug 24, 2018

Alright, third time's the charm. This version just replaces embedStringFile with embedFile, and uses Data.ByteString functions to print out the docs.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Aug 24, 2018

Owner

Great, I guess this fixes this and this and is ready for merge ?

Owner

simonmichael commented Aug 24, 2018

Great, I guess this fixes this and this and is ready for merge ?

@zarybnicky

This comment has been minimized.

Show comment
Hide comment
@zarybnicky

zarybnicky Aug 24, 2018

Yup, I think so.

zarybnicky commented Aug 24, 2018

Yup, I think so.

@simonmichael simonmichael merged commit 1190e2a into simonmichael:master Aug 24, 2018

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Aug 24, 2018

Owner

Thanks!

Owner

simonmichael commented Aug 24, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment