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

Explicitly set locale to C.UTF-8 inside buildAndWatch script #236

Merged
merged 1 commit into from
Jan 21, 2023

Conversation

TikhonJelvis
Copy link
Member

This should fix the problem reported in #29

@Martinsos
Copy link
Contributor

Yup, fixed!

@tomjaguarpaw
Copy link
Collaborator

Do we understand why #30 isn't enough? Do we want to undo #30 at the same time as merging this? I'm a bit uncomfortable setting environment variables all over the place, especially seemingly redundantly.

@TikhonJelvis
Copy link
Member Author

#30 only sets the environment variable during the Nix build and in a Nix shell. That used to be fine since buildAndWatch used to only exist inside the project's Nix shell, but at some point we rewrote the script so that it only runs outside a Nix shell. Instead, the new script uses Nix to build the executable but then runs it in a normal environment which may or may not have the right kind of locale settings.

A better solution would probably be rewriting the Haskell code so that it did not depend on the system's locale settings, but getting that right seems like a bit of a pain—the way locale stuff works in the standard library seems pretty fiddly, and it might even depend on Hakyll internals.

@@ -12,6 +12,7 @@ function buildAndWatchWithNix() {
exit 1
fi

export LC_ALL=C.UTF-8 # fix locale error with Hakyll (see #29)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than exporting this for the entire script we could run the individual commands with this set. For example:
nix-build -A builder && LC_ALL=C.UTF8 ./result/bin/…

that would prevent the change from leaking outside of the command that needs it.

@tomjaguarpaw
Copy link
Collaborator

Personally I'd prefer to understand exactly which part of the pipeline needs the envvar, and set it only there, but approving, since I'm sure you know what you're doing.

Copy link
Collaborator

@rebeccaskinner rebeccaskinner left a comment

Choose a reason for hiding this comment

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

I agree with @tomjaguarpaw that we can probably just merge this for now and maybe revisit later see if we can remove the need for it.

@TikhonJelvis
Copy link
Member Author

Probably best to set the env var for any invocation of ./result/bin/haskell-org-site for consistency. We can always change the approach if it becomes a problem in the future.

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.

4 participants