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

Allow GHC bindists to set locale #1249

Merged
merged 1 commit into from
Feb 21, 2020
Merged

Allow GHC bindists to set locale #1249

merged 1 commit into from
Feb 21, 2020

Conversation

joneshf
Copy link
Contributor

@joneshf joneshf commented Feb 20, 2020

In #1248, it was discovered that the locale for GHC bindists was not able to be overridden. It would default to C.UTF-8. If a system didn't have the C.UTF-8 locale, it could lead to (unclear) encoding failures later on. In particular, haddock might fail in an unclear way:

haddock: internal error: /tmp/tmputn68mya/doc/html/path-io/haddock-response300-1.txt: hGetContents: invalid argument (invalid byte sequence)

This PR allows setting the locale for GHC bindists and also writes up a little bit in the troubleshooting section. I wasn't 100% sure how to make this happen, but this seems to allow my minimal reproduction to build. Lemme know if anything should change, or if this is not the right approach.

I couldn't find any tests to extend for the changes I made here. Should a new test be created, or is this not an area to test?

Some systems might not have the default locale: `C.UTF-8`. When this
locale is not found, some tooling can fail when manipulating files. In
particular, `haddock` can fail when reading files that might contain
UTF-8 encoded values:

```
haddock: internal error: /tmp/tmputn68mya/doc/html/path-io/haddock-response300-1.txt: hGetContents: invalid argument (invalid byte sequence)
```

We have support for setting the locale with nixpkgs, but there was no
support for setting the locale with GHC bindists. We allow overriding
the default locale when using GHC bindists in case a system doesn't have
the `C.UTF-8` locale.

We also write up a little blurb about troubleshooting, in case anyone
else runs into a similar issue.
Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

Thank you, that looks great! In particular thanks for the troubleshooting section!

I couldn't find any tests to extend for the changes I made here. Should a new test be created, or is this not an area to test?

This is a bit tricky to test, I think. Just setting locale = en_US.UTF-8 does not test this extensively, as the build would still pass in our test environment if we accidentally ignored that setting. Maybe @bazel_skylib//lib:unittest.bzl's action testing framework could be used to test that LANG=en_US.UTF-8 is really set for the build actions. Though I don't know how much effort that would be to implement.

@aherrmann aherrmann added the merge-queue merge on green CI label Feb 21, 2020
@mergify mergify bot merged commit 88fb4bf into tweag:master Feb 21, 2020
@mergify mergify bot removed the merge-queue merge on green CI label Feb 21, 2020
@joneshf
Copy link
Contributor Author

joneshf commented Feb 21, 2020

Gotcha! Thanks for the quick review!

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.

2 participants