Merge ~sylvain-pineau/checkbox-ng:fix-1841874 into checkbox-ng:master

[Sylvain Pineau]

The old runner/ctrl code did prepare a differential env for jobs running as root to only carry on some specific user var bit not all.

The unified runner did not have this distinction. This patch brings back the get_differential_execution_environment function.

[Sylvain Pineau (sylvain-pineau)]

tested with the stress auto test plan with both power30 and reboot30 selected.

[Maciej Kisielewski (kissiel)]

Hmmm. Now I finally understand what this was for :D

Thanks for fixing it!

review: Approve

diff --git a/plainbox/impl/execution.py b/plainbox/impl/execution.py
index f73aec2..410df68 100644
--- a/plainbox/impl/execution.py
+++ b/plainbox/impl/execution.py
@@ -278,12 +278,13 @@ class UnifiedRunner(IJobRunner):
                          {"ID": job.id, "CMD": cmd,
                           "ENV": env})
             if 'preserve-cwd' in job.get_flag_set() or os.getenv("SNAP"):
-                return_code = call(extcmd_popen, cmd, stdin=subprocess.PIPE,
-                        env=env)
+                return_code = call(
+                    extcmd_popen, cmd, stdin=subprocess.PIPE, env=env)
             else:
                 with self.temporary_cwd(job, config) as cwd_dir:
-                    return_code = call(extcmd_popen, cmd,
-                            stdin=subprocess.PIPE, env=env, cwd=cwd_dir)
+                    return_code = call(
+                        extcmd_popen, cmd, stdin=subprocess.PIPE, env=env,
+                        cwd=cwd_dir)
             if 'noreturn' in job.get_flag_set():
                 import signal
                 signal.pause()
@@ -365,10 +366,11 @@ class UnifiedRunner(IJobRunner):
         if leftovers:
             logger.warning(
                 _("Job {0} created leftover filesystem artefacts"
-                " in its working directory").format(job.id))
+                  " in its working directory").format(job.id))
             for item in leftovers:
-                logger.warning(_("Leftover file/directory: %r"),
-                            os.path.relpath(item, cwd_dir))
+                logger.warning(_(
+                    "Leftover file/directory: %r"),
+                    os.path.relpath(item, cwd_dir))
             logger.warning(
                 _("Please store desired files in $PLAINBOX_SESSION_SHARE"
                     "and use regular temporary files for everything else"))
@@ -385,8 +387,8 @@ class UnifiedRunner(IJobRunner):
             in_r, in_w = os.pipe()
             os.write(in_w, self._password_provider() + b'\n')
             cmd = ['sudo', '--prompt', '', '--reset-timestamp', '--stdin',
-                    '--user', 'root', 'kill', '-s', str(signal),
-                    '-{}'.format(self._running_jobs_pid)]
+                   '--user', 'root', 'kill', '-s', str(signal),
+                   '-{}'.format(self._running_jobs_pid)]
             try:
                 subprocess.check_call(cmd, stdin=in_r)
             except subprocess.CalledProcessError:
@@ -418,14 +420,13 @@ class FakeJobRunner(UnifiedRunner):
         builder.return_code = 0
         return builder.get_result()
 
+
 def get_execution_environment(job, config, session_dir, nest_dir):
     """
     Get the environment required to execute the specified job:
 
     :param job:
         job definition with the command and environment definitions
-    :param job_state:
-        The JobState associated to the job to execute.
     :param config:
         A PlainBoxConfig instance which can be used to load missing environment
         definitions that apply to all jobs. It is used to provide values for
@@ -477,6 +478,7 @@ def get_execution_environment(job, config, session_dir, nest_dir):
         [nest_dir] + env.get("PATH", "").split(os.pathsep))
     # Add per-session shared state directory
     env['PLAINBOX_SESSION_SHARE'] = os.path.join(session_dir, "CHECKBOX_DATA")
+
     def set_if_not_none(envvar, source):
         """Update env if the source variable is not None"""
         if source is not None:
@@ -497,6 +499,59 @@ def get_execution_environment(job, config, session_dir, nest_dir):
             env[env_var] = config.environment[env_var]
     return env
 
+
+def get_differential_execution_environment(job, config, session_dir, nest_dir):
+    """
+    Get the environment required to execute the specified job:
+
+    :param job:
+        job definition with the command and environment definitions
+    :param config:
+        A PlainBoxConfig instance which can be used to load missing
+        environment definitions that apply to all jobs. It is used to
+        provide values for missing environment variables that are required
+        by the job (as expressed by the environ key in the job definition
+        file).
+    :param session_dir:
+        Base directory of the session this job will execute in.
+        This directory is used to co-locate some data that is unique to
+        this execution as well as data that is shared by all executions.
+    :param nest_dir:
+        A directory with a nest of symlinks to all executables required to
+        execute the specified job. This is simply passed to
+        :meth:`get_execution_environment()` directly.
+    :returns:
+        Differential environment (see below).
+
+    This implementation computes the desired environment (as it was
+    computed in the base class) and then discards all of the environment
+    variables that are identical in both sets. The exception are variables
+    that are mentioned in
+    :meth:`plainbox.impl.job.JobDefinition.get_environ_settings()` which
+    are always retained.
+    """
+    base_env = os.environ
+    target_env = get_execution_environment(job, config, session_dir, nest_dir)
+    delta_env = {
+        key: value
+        for key, value in target_env.items()
+        if key not in base_env or base_env[key] != value
+        or key in job.get_environ_settings()
+    }
+    if 'reset-locale' in job.get_flag_set():
+        delta_env['LANG'] = 'C.UTF-8'
+        delta_env['LANGUAGE'] = ''
+        delta_env['LC_ALL'] = 'C.UTF-8'
+    # Preserve the copy_vars variables + those prefixed with SNAP on Snappy
+    if (os.getenv("SNAP") or os.getenv("SNAP_APP_PATH")):
+        copy_vars = ['PYTHONHOME', 'PYTHONUSERBASE', 'LD_LIBRARY_PATH',
+                     'GI_TYPELIB_PATH']
+        for key, value in base_env.items():
+            if key in copy_vars or key.startswith('SNAP'):
+                delta_env[key] = value
+    return delta_env
+
+
 def get_execution_command(job, config, session_dir,
                           nest_dir, target_user=None):
     """Generate a command argv to run in the shell."""
@@ -509,9 +564,13 @@ def get_execution_command(job, config, session_dir,
         #   - gets password as the first line of stdin (--stdin)
         #   - change the user to the target user (--user)
         cmd = ['sudo', '--prompt', '', '--reset-timestamp', '--stdin',
-                '--user', target_user]
+               '--user', target_user]
     cmd += ['env']
-    env = get_execution_environment(job, config, session_dir, nest_dir)
+    if target_user:
+        env = get_differential_execution_environment(
+            job, config, session_dir, nest_dir)
+    else:
+        env = get_execution_environment(job, config, session_dir, nest_dir)
     cmd += ["{key}={value}".format(key=key, value=value)
             for key, value in sorted(env.items())]
     cmd += [job.shell, '-c', job.command]