[Freeipa-devel] [PATCH] 0748 Handle encoding for ipautil.run

[Petr Viktorin]

On 12/14/2015 10:29 AM, Jan Cholasta wrote:
> On 9.12.2015 12:04, Petr Viktorin wrote:
>> On 12/04/2015 12:49 PM, Jan Cholasta wrote:
>>> On 4.12.2015 12:43, Petr Viktorin wrote:
>>>> On 12/04/2015 12:15 PM, Jan Cholasta wrote:
>>>>> On 4.12.2015 10:53, Petr Viktorin wrote:
>>>>>> On 12/04/2015 08:51 AM, Jan Cholasta wrote:
>>>>>>> On 3.12.2015 15:42, Petr Viktorin wrote:
>>>>>>>> On 12/03/2015 10:55 AM, Jan Cholasta wrote:
>>>> [...]
>>>>>>>>> If we do encode/decode in run(), I think there should be a way to
>>>>>>>>> override the default encoding. My idea was to either accept/return
>>>>>>>>> only
>>>>>>>>> bytes and let the caller handle encoding themselves, or to handle
>>>>>>>>> encoding in run(), but be able to override the defaults.
>>>>>>>>>
>>>>>>>>> Given we handle encoding in run(), I would imagine it like this:
>>>>>>>>>
>>>>>>>>>         def run(args, stdin=None, raiseonerr=True, nolog=(),
>>>>>>>>> env=None,
>>>>>>>>>                 capture_stdout=False, skip_output=False, cwd=None,
>>>>>>>>>                 runas=None, timeout=None, suplementary_groups=[],
>>>>>>>>>                 capture_stderr=False, encode_stdout=False,
>>>>>>>>>                 encode_stderr=False, encoding=None):
>>>>>>>>>
>>>>>>>>> i.e. nothing is captured by default, when stdout or stderr are
>>>>>>>>> captured
>>>>>>>>> they are returned as bytes by default, when stdout or stderr are
>>>>>>>>> returned as text they are decoded using the locale encoding by
>>>>>>>>> default,
>>>>>>>>> or the encoding can be explicitly specified.
>>>>>>>>>
>>>>>>>>> Do you think this is feasible?
>>>>>>>>
>>>>>>>> Feasible, yes.
>>>>>>>> In the majority of cases where the output is needed, we want text
>>>>>>>> encoded with locale.getpreferredencoding(), so I'd rather make
>>>>>>>> that the
>>>>>>>> default rather than bytes.
>>>>>>>>
>>>>>>>> Or we could make "encode_stdout" imply "capture_stdout", so you
>>>>>>>> wouldn't
>>>>>>>> have to always specify both. (It would be better named
>>>>>>>> "encoded_stdout"
>>>>>>>> in that case.)
>>>>>>>
>>>>>>> I think it should be OK to decode by default. (IMO it would be
>>>>>>> best to
>>>>>>> reverse the logic and name it "raw_stdout", with False default.
>>>>>>> Do we
>>>>>>> need "raw_stderr" at all? I don't think we do.)
>>>>>>
>>>>>> Actually, now that I think about it: if the result was a namedtuple
>>>>>> subclass, we can always set extra raw_stdout/raw_stderr attributes on
>>>>>> it. (Unless skip_output=True, of course.)
>>>>>>
>>>>>>       result = run(['generate_binary_data'])
>>>>>>       use(result.raw_stdout)
>>>>>>
>>>>>>       # and for backcompat,
>>>>>>       rc, _out, _err = result
>>>>>
>>>>> Good idea.
>>>>>
>>>>> Perhaps we should use the same names as CalledProcessError for the
>>>>> attributes, i.e. "returncode", "output", and "error_output",
>>>>> "raw_output", "raw_error_output" for the new attributes?
>>>
>>> (Actually, since "returncode" is not "return_code", it should probably
>>> be "returncode", "output", "erroroutput", "raw_output",
>>> "raw_erroroutput".)
>>>
>>>>
>>>> OK. It's also good to use different names than Popen's stdout/stderr,
>>>> which are file-like objects rather than strings. But then,
>>>> capture_stdout/capture_stderr should be
>>>> capture_output/capture_error_output.
>>
>> Here is the new patch.
>> I also added output_log and error_log to the result to abstract the
>> following common pattern:
>>
>>      result = run([...])
>>      if result.returncode != 0:
>>          stderr = result.error_output
>>          if six.PY3:
>>              error = stderr.encode(locale.getpreferredencoding(),
>> 'replace')
>>          self.log.critical('...: %s', stderr)
> 
> 1) certutil -L -r outputs raw DER cert, so this will explode in Python 3
> when pem == False:
> 
>          else:
>              args.append('-r')
>          try:
> -            cert, err, returncode = self.run_certutil(args)
> +            result = self.run_certutil(args, capture_output=True)
>          except ipautil.CalledProcessError:
>              raise RuntimeError("Failed to get %s" % nickname)
> -        return cert
> +        return result.output
> 
>      def has_nickname(self, nickname):
>          try:
> 
> 
> 2) There are some PEP8 transgressions:
> 
> $ git show -U0 | pep8 --diff
> ./ipaplatform/base/services.py:297:22: E127 continuation line
> over-indented for visual indent
> ./ipapython/ipautil.py:279:1: E302 expected 2 blank lines, found 1
> ./ipapython/ipautil.py:280:40: E128 continuation line under-indented for
> visual indent
> ./ipapython/ipautil.py:283:1: E302 expected 2 blank lines, found 1
> ./ipapython/ipautil.py:330:80: E501 line too long (80 > 79 characters)
> ./ipapython/ipautil.py:437:43: E127 continuation line over-indented for
> visual indent
> ./ipapython/ipautil.py:442:43: E127 continuation line over-indented for
> visual indent
> ./ipapython/kernel_keyring.py:57:11: E225 missing whitespace around
> operator
> ./ipaserver/install/cainstance.py:626:80: E501 line too long (80 > 79
> characters)
> ./ipaserver/install/ipa_backup.py:498:17: E128 continuation line
> under-indented for visual indent
> ./ipaserver/install/ipa_backup.py:521:21: E122 continuation line missing
> indentation or outdented
> ./ipaserver/install/ipa_backup.py:530:17: E122 continuation line missing
> indentation or outdented
> ./ipaserver/install/ipa_backup.py:605:52: E225 missing whitespace around
> operator
> ./ipaserver/install/ipa_backup.py:606:17: E122 continuation line missing
> indentation or outdented
> ./ipaserver/install/krbinstance.py:303:80: E501 line too long (84 > 79
> characters)
> ./ipatests/test_ipapython/test_keyring.py:97:33: E222 multiple spaces
> after operator
> 
> 
> 3) The patch needs a rebase.
> 
> 
> I took the liberty of fixing all of the above, see the attached patch.
> 
> ACK from me. If you are OK with the changes, I will push the patch.

I am, ACK. Thank you!


-- 
Petr Viktorin