[Merge] lp:~tdaitx/ubuntu/focal/apport/lp-1851806-fix into lp:~ubuntu-core-dev/ubuntu/focal/apport/ubuntu

[Brian Murray]

Review: Needs Fixing

I've a couple of comments in line. Could you please also explain some more about the argument passing change ("data/apport: fix number of arguments passed through socks into a container.")? Thanks for working on this!

Diff comments:

> 
> === modified file 'apport/ui.py'
> --- apport/ui.py	2019-12-03 03:18:16 +0000
> +++ apport/ui.py	2020-01-06 14:09:26 +0000
> @@ -243,8 +243,11 @@
>              logind_session = None
>          else:
>              reports = apport.fileutils.get_new_reports()
> -            proc_pid_fd = os.open('/proc/%s' % os.getpid(), os.O_RDONLY | os.O_PATH | os.O_DIRECTORY)
> -            logind_session = apport.Report.get_logind_session(proc_pid_fd)
> +            if _python2:

_python2 is set in apport/report.py but not in apport/ui.py afaict. Later on you use "if PY3:" and I think the above should be changed to also use the PY3 variable.

> +                logind_session = apport.Report.get_logind_session(os.getpid())
> +            else:
> +                proc_pid_fd = os.open('/proc/%s' % os.getpid(), os.O_RDONLY | os.O_PATH | os.O_DIRECTORY)
> +                logind_session = apport.Report.get_logind_session(proc_pid_fd=proc_pid_fd)
>  
>          for f in reports:
>              if not self.load_report(f):
> 
> === modified file 'test/test_report.py'
> --- test/test_report.py	2019-12-12 23:13:17 +0000
> +++ test/test_report.py	2020-01-06 14:09:26 +0000
> @@ -2275,9 +2275,28 @@
>          self.assertEqual(expected, pr.crash_signature())
>  
>      def test_get_logind_session(self):
> +        ret = apport.Report.get_logind_session(os.getpid())
> +        if ret is None:
> +            # ensure that we don't run under logind, and thus the None is
> +            # justified
> +            with open('/proc/self/cgroup') as f:
> +                contents = f.read()
> +            sys.stdout.write('[not running under logind] ')
> +            sys.stdout.flush()
> +            self.assertNotIn('name=systemd:/user', contents)
> +            return
> +
> +        (session, timestamp) = ret
> +        self.assertNotEqual(session, '')
> +        # session start must be >= 2014-01-01 and "now"
> +        self.assertLess(timestamp, time.time())

This says "assertLess" but the comment lead me to believe that session start must be >= "now". Am I misunderstanding or could the comment be improved?

> +        self.assertGreater(timestamp,
> +                           time.mktime(time.strptime('2014-01-01', '%Y-%m-%d')))
> +
> +    def test_get_logind_session_fd(self):
>          proc_pid_fd = os.open('/proc/%s' % os.getpid(), os.O_RDONLY | os.O_PATH | os.O_DIRECTORY)
>          self.addCleanup(os.close, proc_pid_fd)
> -        ret = apport.Report.get_logind_session(proc_pid_fd)
> +        ret = apport.Report.get_logind_session(proc_pid_fd=proc_pid_fd)
>          if ret is None:
>              # ensure that we don't run under logind, and thus the None is
>              # justified


-- 
https://code.launchpad.net/~tdaitx/ubuntu/focal/apport/lp-1851806-fix/+merge/377182
Your team Ubuntu Core Development Team is subscribed to branch lp:~ubuntu-core-dev/ubuntu/focal/apport/ubuntu.