On (29/01/15 13:09), Petr Viktorin wrote:
>On 01/29/2015 01:05 PM, Bohuslav Kabrda wrote:
>>----- Original Message -----
>>>ehlo,
>>>
>>>some parts of sssd was not properly ported to python3.
>>>I know there were changes related to unicode, string and bytes.
>>>I am not sure whether my patches for read and open are correct,
>>>especially patch "SSSDConfig: os.write".
>>>
>>>SSSDConfig (python-sssdconfig is used by authconfig and ipa-client-install?
>>>So I don't want to break it.
>>>
>>>Please review patches or propose better version.
>>>
>>>LS
>>
>>Not sure why I missed so many things in my original patch (maybe I searched
>> just for *.py, not *.py.in ;)). Anyway:
and IIRC sbus_codegen was not in git when you sent patches.
>>- Patch 1 LGTM, I only don't understand why you need to import
"exceptions"
>>module (and why only in Python 2). Note that Python 2 documentation says
>>that "This module never needs to be imported explicitly". [1]
It might have been used due to older version of python (RHEL5),
which we do not support any more.
I removed it.
>>- Patch 2 LGMT, but I think you could just write:
>>
>>'long': long if sys.version_info[0] == 2 else int
>>
That's nicer.
>>(but that's just nitpicking...)
>>- Patch 3: this depends. Is the object that you're writing binary or text?
>>I think what you want to do is keep "wb" mode and encode output to a
certain
>>encoding (utf8?). So this could be:
>>
>>of = open(outputfile, "wb")
>>output = self.dump(self.opts).encode("utf-8")
>>of.write(output)
Output file should be valid sssd configuration file which if text file.
I used your proposal.
>>The problem is that the "str" type in Python 2 is sort of polymorphic,
in
>>that it can behave both as binary bytestring and as unicode - but .encode()
>>works on it nonetheless. In Python 3 you have either "bytes" (binary
string)
>>or "str" (unicode text string) and you have to choose which one you
want
>>to write ("wb" mode is for "bytes", while "w" is for
"str").
>>If "self.dump(self.opts)" returns "str" in Python 3, then I
suggest you use
>>'.encode("utf-8")' on it and leave the mode "wb".
>>This should keep working in Python 2.
>>- Patch 4 LGTM
>>- Patch 5 LGTM
>>
>>Slavek
>>
>>[1]
https://docs.python.org/2/library/exceptions.html#module-exceptions
>>
>
>Patch 1/5: Note that even under Python 2, `except ValueError, KeyError:`
>would not work (it catches ValueError, and stores the exception instance
>under the name KeyError); please test these changes well.
>
It means that in python2 KeyError was not handled at all.
python2 documentation says:
"If an exception occurs which does not match the exception named in the except
clause, it is passed on to outer try statements; if no handler is found, it is
an unhandled exception and execution stops with a message as shown above."
In my patch, you can see that different exception was thrown (ParsingError) in
handler. I'm not sure hot to test it therefore you are in CC ;-)
Shall I remove KeyError?
>Patch 4/5: you can use io.StringIO in both 2.6+ and 3.x+
>
Yes, but...
Traceback (most recent call last):
File "../src/sbus/sbus_codegen", line 931, in <module>
main()
File "../src/sbus/sbus_codegen", line 919, in main
generate_source(parser.parsed_interfaces, filename, options.include)
File "../src/sbus/sbus_codegen", line 600, in generate_source
out("/* The following definitions are auto-generated from %s */", basename)
File "../src/sbus/sbus_codegen", line 217, in out
sys.stdout.write(str)
TypeError: unicode argument expected, got 'str'
So I'm find with such solution for StringIO. The sbus_codegen is python script
which generate C source file. I do not expect any unicode characters.
But I'm not opposed better version.
Thank you very much for review.
I squashed some small patches.
LS
I realized there were hardcoded python2 on some places. The 3rd patch should
fix it. I used "env pyhton" for internal python scripts and for tests which are
not installed.
I had a discussion with Pavel B. and we agreed
that patch "sbus_codegen: Port to python3" needn't be push to upstream yet.
It would break his other patches. So current version would be used mostly by
downstream.
LS