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

On 11/06/12 21:34, Barry Warsaw wrote:
> 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:
>

Ok, this does look better. This way was not documented on the Python/3
wiki page, nor in the porting to python3 book.

Noted.

>> @@ -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')
>

Makes sense. Cjwatson was arguing for version checks, as they are easy
to grep for when eventually going python3 only.

> === 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.
>

You should ask this question to apparmor's upstream. As far as I can see
they target python2.5 and up.

--
Regards,
Dmitrijs.