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

Adjusting the locale environment before the daemons are started #347

Merged
merged 1 commit into from Jan 9, 2018

Conversation

Projects
None yet
4 participants
@matsduf
Contributor

matsduf commented Jan 5, 2018

The language setting via JSON RPC does not work if LANGUAGE is set to something, at least in some OSs. With this fix, to be safe some environment variables are unset, and LC_CTYPE is set to the default locale. With this setting it should work, and it will not be in conflict with anything.

The PR is a work-around for the issue described in issue #350, but this should NOT be considered to be a solution.

@matsduf matsduf requested review from mattias-p, sandoche2k and mtoma Jan 5, 2018

@mtoma

Can I have an example of this behaviour?: "The language setting via JSON RPC does not work if LANGUAGE is set to something, at least in some OSs."

@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Jan 5, 2018

Contributor

I followed the instructions for installing under Ubuntu 16.04. When trying directly to the Backend I hade no language support. In the start file I then added "unset LANGUAGE" and restarted Backend (both RPC and Test Agent). Now I hade support for sv and fr (not da due to another bug). I removed "unset LANGUAGE" and restarted and lost support.

The added lines solves the issue and will not create any problem.

We have the same problem in CLI. When LANGUAGE is set to something, then you cannot select language with --locale LOCALE.

Contributor

matsduf commented Jan 5, 2018

I followed the instructions for installing under Ubuntu 16.04. When trying directly to the Backend I hade no language support. In the start file I then added "unset LANGUAGE" and restarted Backend (both RPC and Test Agent). Now I hade support for sv and fr (not da due to another bug). I removed "unset LANGUAGE" and restarted and lost support.

The added lines solves the issue and will not create any problem.

We have the same problem in CLI. When LANGUAGE is set to something, then you cannot select language with --locale LOCALE.

@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Jan 5, 2018

Contributor

@mtoma, are you against the update?

Contributor

matsduf commented Jan 5, 2018

@mtoma, are you against the update?

unset LC_MESSAGES
unset LC_ALL
export LC_CTYPE="en_US.UTF-8"

This comment has been minimized.

@mattias-p

mattias-p Jan 5, 2018

Contributor

All these shouldn't be done in a wrapper script but inside the daemon itself.

Is there a reason why you set LC_CTYPE to en_US.UTF-8?

@mattias-p

mattias-p Jan 5, 2018

Contributor

All these shouldn't be done in a wrapper script but inside the daemon itself.

Is there a reason why you set LC_CTYPE to en_US.UTF-8?

This comment has been minimized.

@matsduf

matsduf Jan 5, 2018

Contributor

I agree with you that it should be done in the JSON RPC daemon itself, but that is too large change to do now. This is a work-around that makes it work.

en_US.UTF-8 is the default locale selected by Backend unless some other language is selected. I have a previous experience that you get problem with Backend unless it starts in a UTF-8 environment.

@matsduf

matsduf Jan 5, 2018

Contributor

I agree with you that it should be done in the JSON RPC daemon itself, but that is too large change to do now. This is a work-around that makes it work.

en_US.UTF-8 is the default locale selected by Backend unless some other language is selected. I have a previous experience that you get problem with Backend unless it starts in a UTF-8 environment.

This comment has been minimized.

@matsduf

matsduf Jan 5, 2018

Contributor

@mattias-p, what changes do you request?

@matsduf

matsduf Jan 5, 2018

Contributor

@mattias-p, what changes do you request?

This comment has been minimized.

@mattias-p

mattias-p Jan 5, 2018

Contributor

In that case I request that comments be added about the fact that it's a workaround, and previous problems with non-UTF-8 environments.

@mattias-p

mattias-p Jan 5, 2018

Contributor

In that case I request that comments be added about the fact that it's a workaround, and previous problems with non-UTF-8 environments.

@mtoma

This comment has been minimized.

Show comment
Hide comment
@mtoma

mtoma Jan 5, 2018

Contributor

I'm not against but all this LOCALE related stuff is quite complex and in the past a few digit separation issues were fixed in the backend because of Perl JSON module bugs. So just trying to understand a little more what this problem is about.

Contributor

mtoma commented Jan 5, 2018

I'm not against but all this LOCALE related stuff is quite complex and in the past a few digit separation issues were fixed in the backend because of Perl JSON module bugs. So just trying to understand a little more what this problem is about.

@mattias-p

This comment has been minimized.

Show comment
Hide comment
@mattias-p

mattias-p Jan 5, 2018

Contributor

Maybe the problem should be documented as a proper bug report in an issue?

Contributor

mattias-p commented Jan 5, 2018

Maybe the problem should be documented as a proper bug report in an issue?

@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Jan 5, 2018

Contributor

@mtoma and @mattias-p! I have created an issue #350 describing the issue. Is it OK to merge this PR?

Contributor

matsduf commented Jan 5, 2018

@mtoma and @mattias-p! I have created an issue #350 describing the issue. Is it OK to merge this PR?

@mattias-p

This comment has been minimized.

Show comment
Hide comment
@mattias-p

mattias-p Jan 9, 2018

Contributor

Accepting this as it solves the problem for at least some contexts. Follow-up issue #353 was created to replace this change with a solution that works for all supported contexts.

Contributor

mattias-p commented Jan 9, 2018

Accepting this as it solves the problem for at least some contexts. Follow-up issue #353 was created to replace this change with a solution that works for all supported contexts.

@matsduf matsduf merged commit 9408a8f into zonemaster:develop Jan 9, 2018

1 check passed

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

@matsduf matsduf deleted the matsduf:start-script-locale-fix branch Jan 9, 2018

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