Merge lp://qastaging/~xnox/apparmor/py3 into lp://qastaging/apparmor/2.12

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.

Looks good to me, but I can't commit to the parent branch, so waiting on someone from AppArmor Devs to pick it up and run with it.

> - print open(i).read()
> + sys.stdout.write(open(i).read()+"\n")

This will leak fds, which python wonderfully loudly complains about in Python 3. It's also a good opportunity to replace any pairs of open() and close() with a with statement. If an open() and close() isn't wrapped in a with or try/finally, I would argue that is a bug.

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

Pushed revision 2054:
* closes file descriptors
* uses except OSError as ex
* doesn't do version checks
* more stray print statements converted to sys.stdXXX.write() syntax

Thanks for your work on this, it looks great! :)