Skip to content

Add LC_CTYPE to environment variables in macOS#9227

Merged
balloob merged 3 commits into
home-assistant:devfrom
morberg:patch-1
Sep 14, 2017
Merged

Add LC_CTYPE to environment variables in macOS#9227
balloob merged 3 commits into
home-assistant:devfrom
morberg:patch-1

Conversation

@morberg
Copy link
Copy Markdown
Contributor

@morberg morberg commented Aug 30, 2017

Some componentes, e.g. tradfri, will not work properly unless LANG is an UTF-8 environment. Not sure which locale to choose, went with en_US.UTF-8. Perhaps C.UTF-8 would be a better choice, but I couldn't get that to work properly (only worked from the command line, not as a launch agent).

Edit: changed to set LC_CTYPE to UTF-8, see discussion below for details.

Description:

Related issue: fixes #9008 for macOS.

Checklist:

Couldn't find anything applicable in the checklist.

Some componentes, e.g. tradfri, will not work properly unless LANG is an UTF-8 environment.
@homeassistant
Copy link
Copy Markdown
Contributor

Hi @morberg,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@dale3h
Copy link
Copy Markdown
Member

dale3h commented Aug 31, 2017

Does this PR only apply to macOS?

@morberg
Copy link
Copy Markdown
Contributor Author

morberg commented Aug 31, 2017

Yes, the launchd.plist file is only used on macOS. Linux and Windows uses other methods for auto start. I've updated the PR title and the original comment to state it only fixes #9008 for macOS.

@morberg morberg changed the title Add LANG to environment variables Add LANG to environment variables in macOS Aug 31, 2017
@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Aug 31, 2017

hmm. @balloob ? Is that a good idea to set it fix to en_US?

@pvizeli pvizeli requested a review from balloob August 31, 2017 14:33
@dale3h
Copy link
Copy Markdown
Member

dale3h commented Aug 31, 2017

@pvizeli I do not think that it is a good idea to hardcode to en_US, but instead we should trap the error inside of the component and provide a better warning of what is going on.

@morberg
Copy link
Copy Markdown
Contributor Author

morberg commented Aug 31, 2017

For reference @matemaciek had UTF-8 problems in docker and proposed a fix in python, but later settled on setting locale in the docker script.

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Aug 31, 2017

We set C.UTF-8 inside docker to fix problem with python. That is a general python problem

@morberg
Copy link
Copy Markdown
Contributor Author

morberg commented Sep 2, 2017

macOS doesn't seem to have a C.UTF-8 locale according to locale -a which is probably why I couldn't get that to work.

If LANG is set to an existing encoding, that value is inherited by all LC_ environment variables. If the encoding does not exist, the LC_ variables will remain unchanged. If I understand the environment variables correctly it is LC_CTYPE that needs to be in a UTF-8 locale for python to work properly.

Through trial, googling, and error I could get Home Assistant (and tradfri) to work properly by setting LC_CTYPE to UTF-8 (not C.UTF-8) in the .plist file instead of setting LANG to en_US.UTF-8. Setting LANG to UTF-8 does not work; LC_CTYPE will remain unchanged. locale -a doesn't list UTF-8 as a locale, which makes me a bit sceptical, but it works - at least on my system.

Would it be a better solution to set LC_CTYPE to UTF-8 in the .plist file?

@balloob
Copy link
Copy Markdown
Member

balloob commented Sep 5, 2017

Let's start with setting LC_CTYPE to UTF-8 in the plist file. I want to point as many things as possible at UTF-8 as possible and shy away from language specific things like en_US.

@morberg morberg changed the title Add LANG to environment variables in macOS Add LC_CTYPE to environment variables in macOS Sep 6, 2017
@morberg
Copy link
Copy Markdown
Contributor Author

morberg commented Sep 6, 2017

Sure thing. Added a new commit, let me know if you need anything else.

@balloob balloob merged commit b21bfe5 into home-assistant:dev Sep 14, 2017
@balloob balloob mentioned this pull request Sep 22, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Mar 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error while setting up platform tradfri

5 participants