Merge lp:~pieq/checkbox/fix-1484872-env-variables-forced-to-non-international into lp:checkbox

[Pierre Equoy]

[Zygmunt Krynicki (zyga)]

12:11 <@zyga> ePierre: https://code.launchpad.net/~pierre-equoy/checkbox/fix-1484872-env-variables-forced-to-non-international/+merge/269873
12:11 <@zyga> ePierre: can you explain how this works please
12:11 <@zyga> ePierre: deleting an environment variable really really means that that variable is gone in the process we fork off
12:11 < ePierre> zyga, sure
12:11 <@zyga> ePierre: I assume there's something that re-establishes them
12:11 <@zyga> ePierre: but I wonder what that is and so now I'm all ears
12:12 < ePierre> ok so we were checking this with spineau
12:12 < ePierre> zyga, we realized that currently, LANGUAGE is deleted from the env dict that is returned
12:13 < ePierre> zyga, but because of that, when pkexec is called, it is not called with a special value for LANGUAGE
12:13 <@zyga> mmm
12:13 <@zyga> ah, so this is for pkexec
12:13 <@zyga> I think you need to change your patch
12:13 < ePierre> zyga, so pkexec uses the available value in the system env, which is, for a system using Chinese, something like zh_CN:zh
12:13 <@zyga> sudo and pkexec use a "differential" environment
12:13 <@zyga> it's a hack that lets us set the environment in the process that they start
12:14 <@zyga> because they will otherwise change the environment significantly (sudo) or entirely (pkexec)
12:15 <@zyga> ePierre: I think this patch is not right, one sec
12:15 <@zyga> ePierre: can you please go and patch the differential environment
12:16 <@zyga> ePierre: I think it's the more correct approach, the generimc method is correct for non-helper execution controllers
12:16 <@zyga> ePierre: and for both pkexec, sudo and p-t-l you need to do things explicitly (and check all three)
12:16 <@zyga> ePierre: as a test case run a job that runs locale
12:16 <@zyga> as in locale(1)
12:16 <@zyga> and run it with each execution controller
12:17 <@zyga> ePierre: the patch should special case locale variables in differential environment
12:17 <@zyga> ePierre: and make sure to see how each of sudo/pkexec/p-t-l execution controllers work please
12:17 < ePierre> zyga what is p-t-l?
12:18 <@zyga> ePierre: plainbox-trusted-launcher
12:18 < ePierre> ha, the famous trusted launcher, ok
12:18 <@zyga> thanks
12:18 <@zyga> ePierre: ping me if you have any questions, thanks for working on this!
12:18 < ePierre> zyga, ok, thanks

review: Needs Fixing

[Pierre Equoy (pieq)]

<ePierre> zyga,when you say to patch the differential environment, is it something done by plainbox?
<zyga> ePierre: yes
<zyga> ePierre: it's all in the same module (ctrl.yp)
<zyga> ePierre: just read it and see what happens
<ePierre> zyga, ok
<zyga> ePierre: there's a method there with that word in the name
<ePierre> zyga, yes I found it. I will check this
<zyga> ePierre: start with sudo/pkexec
<zyga> ePierre: it's the easiest to run
<zyga> ePierre: p-t-l is a bit hard to actually start to use as it's meant for systemwide installed packages
<ePierre> zyga, regarding my patch, I replaced existing lines that were removing the keys in the env dict
<ePierre> zyga, was this correct?
<ePierre> (I mean to remove keys from the env dict)
<zyga> ePierre: no, because I think a different solution is needed for each controller
<zyga> ePierre: you only got lucky here :)
<zyga> ePierre: but if the environment was different to start with it would not work
<zyga> ePierre: you need to explicitly reset those keys in the differential environment
<ePierre> zyga, but should I do this in all the three controllers? RootViaPkexecExecutionController, RootViaPTL1ExecutionController and RootViaSudoExecutionController ?
<zyga> ePierre: no, you should add it to their common base class
<zyga> ePierre: iterate, it will be clear
<zyga> ePierre: if you add it to all three the code will be the same
<ePierre> CheckBoxDifferentialExecutionController
<zyga> ePierre: and then you can spot the shared base class
<zyga> ePierre: exactly
<ePierre> zyga, and by “explicitly reset those keys”, you mean I should add the special cases (LANGUAGES, LC_*...) here?
<zyga> ePierre: yeah, that code there tries to compute the delta so that it can be explicitly passed to each of the three controllers
<zyga> ePierre: just make sure the delta always, unconditionally has those language keys

Unmerged revisions

3979. By Pierre Equoy on 2015-09-02

Force environment into non-internationalized values

Previously, get_execution_environment() method in ctrl.py would set LANG
variable to 'C.UTF-8' but would not do anything to LANGUAGE or LC_* variables.

We change this to make sure these variables are all set to empty strings in
order to force the system to use default (non-internationalized) values.

This allow us to use English regexp in scripts, even on non-English systems.

=== modified file 'plainbox/plainbox/impl/ctrl.py'
--- plainbox/plainbox/impl/ctrl.py	2015-07-07 08:32:21 +0000
+++ plainbox/plainbox/impl/ctrl.py	2015-09-02 10:07:32 +0000
@@ -734,10 +734,10 @@
             # Use non-internationalized environment
             env['LANG'] = 'C.UTF-8'
             if 'LANGUAGE' in env:
-                del env['LANGUAGE']
+                env['LANGUAGE'] = ''
             for name in list(env.keys()):
                 if name.startswith("LC_"):
-                    del env[name]
+                    env[name] = ''
         else:
             # Set the per-provider gettext domain and locale directory
             if job.provider.gettext_domain is not None:
 
=== modified file 'plainbox/plainbox/impl/test_ctrl.py'
--- plainbox/plainbox/impl/test_ctrl.py	2015-08-31 10:05:19 +0000
+++ plainbox/plainbox/impl/test_ctrl.py	2015-09-02 10:07:32 +0000
@@ -694,6 +694,38 @@
         # Ensure that LANG is reset to C.UTF-8
         self.assertEqual(env['LANG'], 'C.UTF-8')
 
+    @mock.patch.dict('os.environ', clear=True, LANGUAGE='fake_LANGUAGE')
+    def test_get_execution_environment_resets_LANGUAGE(self):
+        # Call the tested method
+        env = self.ctrl.get_execution_environment(
+            self.job, self.job_state, self.config, self.SESSION_DIR,
+            self.NEST_DIR)
+        # Ensure that LANGUAGE exists and is set to ''
+        self.assertEqual(env['LANGUAGE'], '')
+
+    @mock.patch.dict('os.environ', clear=True,
+    LC_IDENTIFICATION='fake_LC_IDENTIFICATION', LC_TIME='fake_LC_TIME',
+    LC_NUMERIC='fake_LC_NUMERIC', LC_PAPER='fake_LC_PAPER',
+    LC_MEASUREMENT='fake_LC_MEASUREMENT', LC_ADDRESS='fake_LC_ADDRESS',
+    LC_MONETARY='fake_LC_MONETARY', LC_NAME='fake_LC_NAME',
+    LC_TELEPHONE='fake_LC_TELEPHONE')
+    def test_get_execution_environment_resets_LC(self):
+        # Call the tested method
+        env = self.ctrl.get_execution_environment(
+            self.job, self.job_state, self.config, self.SESSION_DIR,
+            self.NEST_DIR)
+        # Ensure that different LC_* are set to ''
+        self.assertEqual(env['LC_IDENTIFICATION'], '')
+        self.assertEqual(env['LC_TIME'], '')
+        self.assertEqual(env['LC_NUMERIC'], '')
+        self.assertEqual(env['LC_PAPER'], '')
+        self.assertEqual(env['LC_MEASUREMENT'], '')
+        self.assertEqual(env['LC_ADDRESS'], '')
+        self.assertEqual(env['LC_MONETARY'], '')
+        self.assertEqual(env['LC_NAME'], '')
+        self.assertEqual(env['LC_TELEPHONE'], '')
+
+
     @mock.patch.dict('os.environ', clear=True, LANG='fake_LANG')
     def test_get_execution_environment_preserves_LANG_if_requested(self):
         self.job.get_flag_set.return_value = {'preserve-locale'}