Merge ~sylvain-pineau/checkbox-ng:no_psutil_xenial into checkbox-ng:master

[Sylvain Pineau]

Close your eyes and click approve...

[Sylvain Pineau (sylvain-pineau)]

tested on a remote xenial system with the following launcher:

[launcher]
app_id = com.canonical.certification:checkbox-test
launcher_version = 1
stock_reports = text, submission_files

[test plan]
unit = com.canonical.certification::graphics-integrated-gpu-cert-manual
forced = yes

[daemon]
normal_user = u

[Maciej Kisielewski (kissiel)]

One change I would like to see is making the distinction on "startup" and selecting the appropriate function for future use, like so:

# global scope

try:
    import psutil
    if psutil.__version__ < (...):
        throw Something()
    prep_display = _prep_disp_using_psutil
except ( ImportError, Someting):
    prep_display = _prep_disp_using_proc

This way we don't go deep into psutil use exception as a code flow.

[Sylvain Pineau (sylvain-pineau)]

setting the display must be done when starting/resuming the session not when the service starts (i.e at import time)

[Taihsiang Ho (tai271828)]

The merge request is good to me, though I love Maciej's idea as well. Thanks for the quick fix.

review: Approve

[Maciej Kisielewski (kissiel)]

Right. But I would still prefer for this not to be run every time the DISPLAY value is needed.
Also, I think _prepare_display_without_psutil can be made non-member (doesn't use self)

[Sylvain Pineau (sylvain-pineau)]

checking psutil.__version__ would require a version comparison import such as distutils since the req is 4.0.0. I don't think it worths the effort and catching the AttributeError is probably a good compromise. Keep in mind that this DISPLAY madness is just a one-time operation (session start or session resume).

Regarding member vs non-member, let's say that I prefer to hide it from globals...

[Maciej Kisielewski (kissiel)]

Mmm, indeed. It's called once per stack bring-up, great!

I disagree on the placement of both DISPLAY-getting functions, but not to a point where I would block this from landing.

+1

review: Approve

[Devices Certification Bot (ce-certification-qa)]

The merge was fine but running tests failed.

[xenial] [13:29:39] starting container
Device project added to xenial-testing
[bionic] [13:29:40] starting container
Device project added to bionic-testing
[xenial] [13:29:47] provisioning container
[bionic] [13:29:53] provisioning container
[bionic] [13:31:12] Unable to provision requirements in container!
[bionic] output: https://paste.ubuntu.com/p/dmCDq64Rnq/
[bionic] [13:31:15] Fixing file permissions in source directory
[bionic] Destroying failed container to reclaim resources
[xenial] [13:31:47] Unable to provision requirements in container!
[xenial] output: https://paste.ubuntu.com/p/QVPX8VvHxB/
[xenial] [13:31:49] Fixing file permissions in source directory
[xenial] Destroying failed container to reclaim resources

review: Needs Fixing

[Devices Certification Bot (ce-certification-qa)]

The merge was fine but running tests failed.

[xenial] [13:42:04] starting container
Device project added to xenial-testing
[bionic] [13:42:06] starting container
Device project added to bionic-testing
[xenial] [13:42:13] provisioning container
[bionic] [13:42:18] provisioning container
[bionic] [13:43:38] Unable to provision requirements in container!
[bionic] output: https://paste.ubuntu.com/p/JyJgBDSNmM/
[bionic] [13:43:41] Fixing file permissions in source directory
[bionic] Destroying failed container to reclaim resources
[xenial] [13:45:53] Unable to provision requirements in container!
[xenial] output: https://paste.ubuntu.com/p/8fvRfJT2Q7/
[xenial] [13:45:56] Fixing file permissions in source directory
[xenial] Destroying failed container to reclaim resources

review: Needs Fixing

diff --git a/plainbox/impl/session/remote_assistant.py b/plainbox/impl/session/remote_assistant.py
index 326f81f..907bce2 100644
--- a/plainbox/impl/session/remote_assistant.py
+++ b/plainbox/impl/session/remote_assistant.py
@@ -25,7 +25,7 @@ import time
 import sys
 from collections import namedtuple
 from threading import Thread, Lock
-from subprocess import DEVNULL, CalledProcessError, check_call
+from subprocess import DEVNULL, CalledProcessError, check_call, check_output
 
 from plainbox.impl.execution import UnifiedRunner
 from plainbox.impl.session.assistant import SessionAssistant
@@ -186,6 +186,16 @@ class RemoteSessionAssistant():
         self._last_response = response
         self._state = Running
 
+    def _prepare_display_without_psutil(self):
+        try:
+            value = check_output(
+                'strings /proc/*/environ 2>/dev/null | '
+                'grep -m 1 -oP "(?<=DISPLAY=).*"',
+                shell=True, universal_newlines=True).rstrip()
+            return {'DISPLAY': value}
+        except CalledProcessError:
+            return None
+
     def prepare_extra_env(self):
         # If possible also set the DISPLAY env var
         # i.e when a user desktop session is running
@@ -195,6 +205,9 @@ class RemoteSessionAssistant():
                 p_user = psutil.Process(p).username()
             except psutil.AccessDenied:
                 continue
+            except AttributeError:
+                # psutil < 4.0.0 doesn't provide Process.environ()
+                return self._prepare_display_without_psutil()
             if ("DISPLAY" in p_environ and p_user != 'gdm'):  # gdm uses :1024
                 return {'DISPLAY': p_environ['DISPLAY']}
                 break