Code review comment for lp:~xnox/apparmor/py3

On Jun 11, 2012, at 05:04 PM, Dmitrijs Ledkovs wrote:

>Dmitrijs Ledkovs has proposed merging lp:~dmitrij.ledkov/apparmor/py3 into lp:apparmor.
>
>Requested reviews:
> Canonical Foundations Team (canonical-foundations)
> AppArmor Developers (apparmor-dev)
>
>For more details, see:
>https://code.launchpad.net/~dmitrij.ledkov/apparmor/py3/+merge/109682
>
>Initial port to python3 for utilities.
>
>Python2 compatibility is not broken, all code should be 'bilingual'.
>
>Adds helpers in the Make.rules to detect python2 and/or python3.
>
>Runs test and check targets with both pythons, if available.
>
>Python3 is entirely optional with these changes, but the test/check targets will fail if python3 incompatible code is introduced and python3 is available during build. If you do not want the build to fail export PYTHON_VERSIONS=python2 before running check/test targets.
>
>Currently the install defaults to python2, with fallback to python3
>
>This does not have anything to do with swig python binding.

=== modified file 'utils/apparmor/easyprof.py'
--- utils/apparmor/easyprof.py 2012-05-08 05:37:48 +0000
+++ utils/apparmor/easyprof.py 2012-06-11 17:03:23 +0000
> @@ -70,7 +70,8 @@
> try:
> sp = subprocess.Popen(command, stdout=subprocess.PIPE,
> stderr=subprocess.STDOUT)
> - except OSError, ex:
> + except OSError:
> + ex = sys.exc_info()[1]
> return [127, str(ex)]

I think in general, a better way of doing this would be to change the except
line to

    except OSError as ex:

> @@ -181,7 +183,8 @@
> fn = policy
> else:
> f, fn = tempfile.mkstemp(prefix='aa-easyprof')
> - os.write(f, policy)
> + policy_bytes = bytes(policy, 'utf-8') if sys.version > '3' else policy
> + os.write(f, policy_bytes)

I generally don't like version checks unless there's no other way. In this
case, you could do something like this instead:

    if not isinstance(policy, bytes):
        policy = policy.encode('utf-8')

=== modified file 'utils/test/test-aa-easyprof.py'
--- utils/test/test-aa-easyprof.py 2012-05-09 18:05:07 +0000
+++ utils/test/test-aa-easyprof.py 2012-06-11 17:03:23 +0000
> @@ -101,6 +101,7 @@
> def tearDown(self):
> '''Teardown for tests'''
> if os.path.exists(self.tmpdir):
> + sys.stdout.write("%s\n" % self.tmpdir)
> recursive_rm(self.tmpdir)
>
> #

Why did you decide against using print(). I'm sure there's a good reason, but
it might be useful in at least some of these cases to explain in comments why
sys.stdout.write() is being used instead of print(). Otherwise, when the next
person comes along they probably won't understand why this idiom was chosen.