[apparmor] [Merge] lp:~dmitrij.ledkov/apparmor/py3 into lp:apparmor

[Dmitrijs Ledkovs]

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.



https://code.launchpad.net/~dmitrij.ledkov/apparmor/py3/+merge/109682
Your team AppArmor Developers is requested to review the proposed merge of lp:~dmitrij.ledkov/apparmor/py3 into lp:apparmor.