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
----- 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: - 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] - Patch 2 LGMT, but I think you could just write:
'long': long if sys.version_info[0] == 2 else int
(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)
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
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:
- 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]
- Patch 2 LGMT, but I think you could just write:
'long': long if sys.version_info[0] == 2 else int
(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)
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.
Patch 4/5: you can use io.StringIO in both 2.6+ and 3.x+
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
On 01/29/2015 04:39 PM, Lukas Slebodnik wrote:
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.
There are lots of patches on the list that touches sbus_codegen. Please wait with those if it conflicts.
- 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
On (29/01/15 16:39), Lukas Slebodnik wrote:
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
On 02/02/2015 09:08 AM, Lukas Slebodnik wrote: [...]
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?
Yes, remove KeyError. Looking at the history, since 2009 the code worked without catching KeyError (even if that wasn't the intention). I see no point in adding it now.
Otherwise LGTM
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.
I see. People could expect "io" to be the io module, so you could write
if sys.version_info[0] > 2: from io import StringIO else: from StringIO import StringIO ... sys.stdout = buf = StringIO()
... but at this point that's just nitpicking.
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.
On (02/02/15 13:32), Petr Viktorin wrote:
On 02/02/2015 09:08 AM, Lukas Slebodnik wrote: [...]
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?
Yes, remove KeyError. Looking at the history, since 2009 the code worked without catching KeyError (even if that wasn't the intention). I see no point in adding it now.
Removed
Otherwise LGTM
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.
I see. People could expect "io" to be the io module, so you could write
if sys.version_info[0] > 2: from io import StringIO else: from StringIO import StringIO ... sys.stdout = buf = StringIO()
Fixed
... but at this point that's just nitpicking.
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.
Thank you very much for review.
Updated version is attached.
LS
On 02/10/2015 11:36 PM, Lukas Slebodnik wrote:
On (02/02/15 13:32), Petr Viktorin wrote:
On 02/02/2015 09:08 AM, Lukas Slebodnik wrote: [...]
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?
Yes, remove KeyError. Looking at the history, since 2009 the code worked without catching KeyError (even if that wasn't the intention). I see no point in adding it now.
Removed
Otherwise LGTM
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.
I see. People could expect "io" to be the io module, so you could write
if sys.version_info[0] > 2: from io import StringIO else: from StringIO import StringIO ... sys.stdout = buf = StringIO()Fixed
... but at this point that's just nitpicking.
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.
Thank you very much for review.
Updated version is attached.
LGTM
On (12/02/15 12:22), Petr Viktorin wrote:
On 02/10/2015 11:36 PM, Lukas Slebodnik wrote:
On (02/02/15 13:32), Petr Viktorin wrote:
On 02/02/2015 09:08 AM, Lukas Slebodnik wrote: [...]
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?
Yes, remove KeyError. Looking at the history, since 2009 the code worked without catching KeyError (even if that wasn't the intention). I see no point in adding it now.
Removed
Otherwise LGTM
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.
I see. People could expect "io" to be the io module, so you could write
if sys.version_info[0] > 2: from io import StringIO else: from StringIO import StringIO ... sys.stdout = buf = StringIO()
Fixed
... but at this point that's just nitpicking.
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.
Thank you very much for review.
Updated version is attached.
LGTM
Here is a link to ci result http://sssd-ci.duckdns.org/logs/job/7/32/summary.html.
I think we should push these patches also to stable branch sssd-1.12. So package maintainers can decide wheter to build with python2 or python3. The other patches on the list which allow to build bindings for python2 and python3 are quite invasive and changes some behavoiur therefore are not intended to push to stable branch.
These patches depends on Bohuslav's patch "Python3 support in SSSD" 341a00311680a440d7f979f06c34c70d86c9367a "BUILD: Include python-test.py in the tarball" 51d65c4ad15c2cc23f38fa09dd6efeb15e4f3e86
BTW We agreed with Pavel (pbrezina) to do not push patch sbus_codegen patch to master because he did some changes in that file. So I will prepare next version afther his patches will be pushed.
LS
On Fri, Feb 13, 2015 at 02:24:12PM +0100, Lukas Slebodnik wrote:
On (12/02/15 12:22), Petr Viktorin wrote:
On 02/10/2015 11:36 PM, Lukas Slebodnik wrote:
On (02/02/15 13:32), Petr Viktorin wrote:
On 02/02/2015 09:08 AM, Lukas Slebodnik wrote: [...]
>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?
Yes, remove KeyError. Looking at the history, since 2009 the code worked without catching KeyError (even if that wasn't the intention). I see no point in adding it now.
Removed
Otherwise LGTM
>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.
I see. People could expect "io" to be the io module, so you could write
if sys.version_info[0] > 2: from io import StringIO else: from StringIO import StringIO ... sys.stdout = buf = StringIO()
Fixed
... but at this point that's just nitpicking.
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.
Thank you very much for review.
Updated version is attached.
LGTM
Here is a link to ci result http://sssd-ci.duckdns.org/logs/job/7/32/summary.html.
I think we should push these patches also to stable branch sssd-1.12. So package maintainers can decide wheter to build with python2 or python3. The other patches on the list which allow to build bindings for python2 and python3 are quite invasive and changes some behavoiur therefore are not intended to push to stable branch.
These patches depends on Bohuslav's patch "Python3 support in SSSD" 341a00311680a440d7f979f06c34c70d86c9367a "BUILD: Include python-test.py in the tarball" 51d65c4ad15c2cc23f38fa09dd6efeb15e4f3e86
BTW We agreed with Pavel (pbrezina) to do not push patch sbus_codegen patch to master because he did some changes in that file. So I will prepare next version afther his patches will be pushed.
LS
Pushed all except codegen to master: * e8058322725ba050014777ee2484f7e833ab1e3a * a71004c112cd5d61d3a9e37a4cfc5760dc9a1cec * 1ac368d0962ef8cc83dcd642c7fec8b3cba5b6fe and all to sssd-1-12: * 9687d7db79e15846de385537a99525d11cae6a15 (Slavek's patch) * 42563a20baf2f334c01a8f821c5c2d98c208fc84 (tarball patch) * d36ff71364db4abc08053d36d392aa602fc5860a * af8e9134176b4517ecfdd2e10e6204fd3f0ad765 * d0a95d87f41721bb57149471897cc920a8730c20 * 46020d2dbb0f092fca7ec2a27e8c822543ab50fd (codegen)
On (13/02/15 18:43), Jakub Hrozek wrote:
On Fri, Feb 13, 2015 at 02:24:12PM +0100, Lukas Slebodnik wrote:
On (12/02/15 12:22), Petr Viktorin wrote:
On 02/10/2015 11:36 PM, Lukas Slebodnik wrote:
On (02/02/15 13:32), Petr Viktorin wrote:
On 02/02/2015 09:08 AM, Lukas Slebodnik wrote: [...]
>>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?
Yes, remove KeyError. Looking at the history, since 2009 the code worked without catching KeyError (even if that wasn't the intention). I see no point in adding it now.
Removed
Otherwise LGTM
> >>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.
I see. People could expect "io" to be the io module, so you could write
if sys.version_info[0] > 2: from io import StringIO else: from StringIO import StringIO ... sys.stdout = buf = StringIO()
Fixed
... but at this point that's just nitpicking.
> >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.
Thank you very much for review.
Updated version is attached.
LGTM
Here is a link to ci result http://sssd-ci.duckdns.org/logs/job/7/32/summary.html.
I think we should push these patches also to stable branch sssd-1.12. So package maintainers can decide wheter to build with python2 or python3. The other patches on the list which allow to build bindings for python2 and python3 are quite invasive and changes some behavoiur therefore are not intended to push to stable branch.
These patches depends on Bohuslav's patch "Python3 support in SSSD" 341a00311680a440d7f979f06c34c70d86c9367a "BUILD: Include python-test.py in the tarball" 51d65c4ad15c2cc23f38fa09dd6efeb15e4f3e86
BTW We agreed with Pavel (pbrezina) to do not push patch sbus_codegen patch to master because he did some changes in that file. So I will prepare next version afther his patches will be pushed.
LS
Pushed all except codegen to master:
- e8058322725ba050014777ee2484f7e833ab1e3a
- a71004c112cd5d61d3a9e37a4cfc5760dc9a1cec
- 1ac368d0962ef8cc83dcd642c7fec8b3cba5b6fe
and all to sssd-1-12:
- 9687d7db79e15846de385537a99525d11cae6a15 (Slavek's patch)
- 42563a20baf2f334c01a8f821c5c2d98c208fc84 (tarball patch)
- d36ff71364db4abc08053d36d392aa602fc5860a
- af8e9134176b4517ecfdd2e10e6204fd3f0ad765
- d0a95d87f41721bb57149471897cc920a8730c20
- 46020d2dbb0f092fca7ec2a27e8c822543ab50fd (codegen)
Thank you
LS
On (13/02/15 18:43), Jakub Hrozek wrote:
On Fri, Feb 13, 2015 at 02:24:12PM +0100, Lukas Slebodnik wrote:
On (12/02/15 12:22), Petr Viktorin wrote:
On 02/10/2015 11:36 PM, Lukas Slebodnik wrote:
On (02/02/15 13:32), Petr Viktorin wrote:
On 02/02/2015 09:08 AM, Lukas Slebodnik wrote: [...]
>>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?
Yes, remove KeyError. Looking at the history, since 2009 the code worked without catching KeyError (even if that wasn't the intention). I see no point in adding it now.
Removed
Otherwise LGTM
> >>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.
I see. People could expect "io" to be the io module, so you could write
if sys.version_info[0] > 2: from io import StringIO else: from StringIO import StringIO ... sys.stdout = buf = StringIO()
Fixed
... but at this point that's just nitpicking.
> >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.
Thank you very much for review.
Updated version is attached.
LGTM
Here is a link to ci result http://sssd-ci.duckdns.org/logs/job/7/32/summary.html.
I think we should push these patches also to stable branch sssd-1.12. So package maintainers can decide wheter to build with python2 or python3. The other patches on the list which allow to build bindings for python2 and python3 are quite invasive and changes some behavoiur therefore are not intended to push to stable branch.
These patches depends on Bohuslav's patch "Python3 support in SSSD" 341a00311680a440d7f979f06c34c70d86c9367a "BUILD: Include python-test.py in the tarball" 51d65c4ad15c2cc23f38fa09dd6efeb15e4f3e86
BTW We agreed with Pavel (pbrezina) to do not push patch sbus_codegen patch to master because he did some changes in that file. So I will prepare next version afther his patches will be pushed.
LS
Pushed all except codegen to master:
- e8058322725ba050014777ee2484f7e833ab1e3a
- a71004c112cd5d61d3a9e37a4cfc5760dc9a1cec
- 1ac368d0962ef8cc83dcd642c7fec8b3cba5b6fe
Attached is a rebased sbus_codegen patch on top of current master. There isn't significant change.
LS
On Thu, Mar 12, 2015 at 12:45:55PM +0100, Lukas Slebodnik wrote:
On (13/02/15 18:43), Jakub Hrozek wrote:
On Fri, Feb 13, 2015 at 02:24:12PM +0100, Lukas Slebodnik wrote:
On (12/02/15 12:22), Petr Viktorin wrote:
On 02/10/2015 11:36 PM, Lukas Slebodnik wrote:
On (02/02/15 13:32), Petr Viktorin wrote:
On 02/02/2015 09:08 AM, Lukas Slebodnik wrote: [...] >>>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?
Yes, remove KeyError. Looking at the history, since 2009 the code worked without catching KeyError (even if that wasn't the intention). I see no point in adding it now.
Removed
Otherwise LGTM
>> >>>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.
I see. People could expect "io" to be the io module, so you could write
if sys.version_info[0] > 2: from io import StringIO else: from StringIO import StringIO ... sys.stdout = buf = StringIO()
Fixed
... but at this point that's just nitpicking.
>> >>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.
Thank you very much for review.
Updated version is attached.
LGTM
Here is a link to ci result http://sssd-ci.duckdns.org/logs/job/7/32/summary.html.
I think we should push these patches also to stable branch sssd-1.12. So package maintainers can decide wheter to build with python2 or python3. The other patches on the list which allow to build bindings for python2 and python3 are quite invasive and changes some behavoiur therefore are not intended to push to stable branch.
These patches depends on Bohuslav's patch "Python3 support in SSSD" 341a00311680a440d7f979f06c34c70d86c9367a "BUILD: Include python-test.py in the tarball" 51d65c4ad15c2cc23f38fa09dd6efeb15e4f3e86
BTW We agreed with Pavel (pbrezina) to do not push patch sbus_codegen patch to master because he did some changes in that file. So I will prepare next version afther his patches will be pushed.
LS
Pushed all except codegen to master:
- e8058322725ba050014777ee2484f7e833ab1e3a
- a71004c112cd5d61d3a9e37a4cfc5760dc9a1cec
- 1ac368d0962ef8cc83dcd642c7fec8b3cba5b6fe
Attached is a rebased sbus_codegen patch on top of current master. There isn't significant change.
LS
Sorry, forgot to send push-mail: * e239b5bedd877e6c5002b22ea11926a12b4c781c
sssd-devel@lists.fedorahosted.org
-
Bohuslav Kabrda -
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Petr Viktorin