Re: [Freeipa-devel] [PATCHES] More Python 3 porting
On 10/07/2015 10:27 AM, Jan Cholasta wrote: > On 6.10.2015 12:04, Petr Viktorin wrote: >> On 10/05/2015 07:56 AM, Jan Cholasta wrote: >>> On 2.10.2015 13:09, Petr Viktorin wrote: On 10/01/2015 03:15 PM, Jan Cholasta wrote: > Hi, > > On 1.10.2015 13:01, Martin Basti wrote: >> >> >> On 09/30/2015 10:25 AM, Petr Viktorin wrote: >>> On 09/23/2015 04:46 PM, Petr Viktorin wrote: On 09/22/2015 02:59 PM, David Kupka wrote: > On 18/09/15 17:00, Petr Viktorin wrote: >> Hello, >> Here are more patches that bring IPA closer to Python 3 >> compatibility. [...] >>> >> LGTM >> >> I ran xmlrpc tests, DNSSEC ci tests, backup and restore CI test and >> everything works > > Patches 713-719: ACK > > > Patch 720: > > You missed: > > ipa-client/ipa-install/ipa-client-install:32:from ConfigParser > import RawConfigParser Thanks, fixed. > Patches 721-722: ACK > > > Patch 723: > > Why the "NoneType = type(None)" in parameters.py? It is used only at: > > ipalib/parameters.py:388:type = NoneType # Ouch, this wont be > very > useful in the real world! I believe this is less confusing than `type = type(None)`, but I can change that if needed. >>> >>> I don't care which one is used TBH, just that it is done consistently >>> accross the whole patch, and this seemed like the simpler thing to do. >> >> OK, changed. >> > Patch 724: > > The SSHPublicKey class was written with the assumption that "str" > means > binary data, so unless I'm missing something, you only need to replace > "str" with "bytes". It specifically did take non-binary data as str: -if isinstance(key, str) and key[:3] != '\0\0\0': -key = key.decode(encoding) >>> >>> I don't follow, this is quite obviously binary data. It reads: "If key >>> is binary and does not start with 3 null bytes, decode it to text using >>> the specified encoding." >> >> Right, it's text (non-binary) data encoded in str (bytes), so it needs >> to be encoded. >> I've removed this for Python 3, where text data shouldn't be in bytes. Since this means the '\0\0\0' check is skipped in __init__ under Python 3, I've added it also to _parse_raw. >>> >>> When the SSH integration feature was first introduced, SSH public keys >>> were stored in the raw binary form in LDAP, i.e. not text data. We still >>> need to support that, so support for binary data and the 3 null check >>> must remain in SSHPublicKey. >> >> Changed, updated patches attached. > > Thanks, ACK. > > I took the liberty of amending patch 718 to silence this pylint false > positive I was getting on F22: > > ipalib/plugins/otptoken.py:496: [E1101(no-member), > HTTPSHandler.https_open] Instance of 'HTTPSHandler' has no 'do_open' > member) Thanks! > Pushed to master: f82d3da1e8e5dc1d0716201af5abb724a8e78fde > > BTW, in patch 724, binascii.Error is handled in addition to TypeError > with base64.b64decode(). There are multiple places where > base64.b64decode() is used in IPA where only TypeError is handled. Are > you planning on fixing this as well? I'll go through them to make sure we're handling them correctly. -- Petr Viktorin -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] More Python 3 porting
On 6.10.2015 12:04, Petr Viktorin wrote: On 10/05/2015 07:56 AM, Jan Cholasta wrote: On 2.10.2015 13:09, Petr Viktorin wrote: On 10/01/2015 03:15 PM, Jan Cholasta wrote: Hi, On 1.10.2015 13:01, Martin Basti wrote: On 09/30/2015 10:25 AM, Petr Viktorin wrote: On 09/23/2015 04:46 PM, Petr Viktorin wrote: On 09/22/2015 02:59 PM, David Kupka wrote: On 18/09/15 17:00, Petr Viktorin wrote: Hello, Here are more patches that bring IPA closer to Python 3 compatibility. [...] LGTM I ran xmlrpc tests, DNSSEC ci tests, backup and restore CI test and everything works Patches 713-719: ACK Patch 720: You missed: ipa-client/ipa-install/ipa-client-install:32:from ConfigParser import RawConfigParser Thanks, fixed. Patches 721-722: ACK Patch 723: Why the "NoneType = type(None)" in parameters.py? It is used only at: ipalib/parameters.py:388:type = NoneType # Ouch, this wont be very useful in the real world! I believe this is less confusing than `type = type(None)`, but I can change that if needed. I don't care which one is used TBH, just that it is done consistently accross the whole patch, and this seemed like the simpler thing to do. OK, changed. Patch 724: The SSHPublicKey class was written with the assumption that "str" means binary data, so unless I'm missing something, you only need to replace "str" with "bytes". It specifically did take non-binary data as str: -if isinstance(key, str) and key[:3] != '\0\0\0': -key = key.decode(encoding) I don't follow, this is quite obviously binary data. It reads: "If key is binary and does not start with 3 null bytes, decode it to text using the specified encoding." Right, it's text (non-binary) data encoded in str (bytes), so it needs to be encoded. I've removed this for Python 3, where text data shouldn't be in bytes. Since this means the '\0\0\0' check is skipped in __init__ under Python 3, I've added it also to _parse_raw. When the SSH integration feature was first introduced, SSH public keys were stored in the raw binary form in LDAP, i.e. not text data. We still need to support that, so support for binary data and the 3 null check must remain in SSHPublicKey. Changed, updated patches attached. Thanks, ACK. I took the liberty of amending patch 718 to silence this pylint false positive I was getting on F22: ipalib/plugins/otptoken.py:496: [E1101(no-member), HTTPSHandler.https_open] Instance of 'HTTPSHandler' has no 'do_open' member) Pushed to master: f82d3da1e8e5dc1d0716201af5abb724a8e78fde BTW, in patch 724, binascii.Error is handled in addition to TypeError with base64.b64decode(). There are multiple places where base64.b64decode() is used in IPA where only TypeError is handled. Are you planning on fixing this as well? Honza -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] More Python 3 porting
On 10/05/2015 07:56 AM, Jan Cholasta wrote: > On 2.10.2015 13:09, Petr Viktorin wrote: >> On 10/01/2015 03:15 PM, Jan Cholasta wrote: >>> Hi, >>> >>> On 1.10.2015 13:01, Martin Basti wrote: On 09/30/2015 10:25 AM, Petr Viktorin wrote: > On 09/23/2015 04:46 PM, Petr Viktorin wrote: >> On 09/22/2015 02:59 PM, David Kupka wrote: >>> On 18/09/15 17:00, Petr Viktorin wrote: Hello, Here are more patches that bring IPA closer to Python 3 compatibility. >> [...] > LGTM I ran xmlrpc tests, DNSSEC ci tests, backup and restore CI test and everything works >>> >>> Patches 713-719: ACK >>> >>> >>> Patch 720: >>> >>> You missed: >>> >>> ipa-client/ipa-install/ipa-client-install:32:from ConfigParser >>> import RawConfigParser >> >> >> Thanks, fixed. >> >>> Patches 721-722: ACK >>> >>> >>> Patch 723: >>> >>> Why the "NoneType = type(None)" in parameters.py? It is used only at: >>> >>> ipalib/parameters.py:388:type = NoneType # Ouch, this wont be very >>> useful in the real world! >> >> I believe this is less confusing than `type = type(None)`, but I can >> change that if needed. > > I don't care which one is used TBH, just that it is done consistently > accross the whole patch, and this seemed like the simpler thing to do. OK, changed. >>> Patch 724: >>> >>> The SSHPublicKey class was written with the assumption that "str" means >>> binary data, so unless I'm missing something, you only need to replace >>> "str" with "bytes". >> >> It specifically did take non-binary data as str: >> >> -if isinstance(key, str) and key[:3] != '\0\0\0': >> -key = key.decode(encoding) > > I don't follow, this is quite obviously binary data. It reads: "If key > is binary and does not start with 3 null bytes, decode it to text using > the specified encoding." Sorry, I meant binary data. >> I've removed this for Python 3, where text data shouldn't be in bytes. >> >> Since this means the '\0\0\0' check is skipped in __init__ under Python >> 3, I've added it also to _parse_raw. > > When the SSH integration feature was first introduced, SSH public keys > were stored in the raw binary form in LDAP, i.e. not text data. We still > need to support that, so support for binary data and the 3 null check > must remain in SSHPublicKey. > >> >> It's not necessary to dispatch to "_parse_raw" or "_parse_base64 or >> _parse_openssh" based on type, but I believe this makes the control flow >> clearer to follow. >> >>> Patch 725: ACK >> >> > > -- Petr Viktorin -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] More Python 3 porting
Please ignore that mail, I sent an unfinished draft by mistake. On 10/06/2015 12:02 PM, Petr Viktorin wrote: [...] > > OK, changed. > > Patch 724: The SSHPublicKey class was written with the assumption that "str" means binary data, so unless I'm missing something, you only need to replace "str" with "bytes". >>> >>> It specifically did take non-binary data as str: >>> >>> -if isinstance(key, str) and key[:3] != '\0\0\0': >>> -key = key.decode(encoding) >> >> I don't follow, this is quite obviously binary data. It reads: "If key >> is binary and does not start with 3 null bytes, decode it to text using >> the specified encoding." > > Sorry, I meant binary data. > -- Petr Viktorin -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] More Python 3 porting
On 2.10.2015 13:09, Petr Viktorin wrote: On 10/01/2015 03:15 PM, Jan Cholasta wrote: Hi, On 1.10.2015 13:01, Martin Basti wrote: On 09/30/2015 10:25 AM, Petr Viktorin wrote: On 09/23/2015 04:46 PM, Petr Viktorin wrote: On 09/22/2015 02:59 PM, David Kupka wrote: On 18/09/15 17:00, Petr Viktorin wrote: Hello, Here are more patches that bring IPA closer to Python 3 compatibility. [...] LGTM I ran xmlrpc tests, DNSSEC ci tests, backup and restore CI test and everything works Patches 713-719: ACK Patch 720: You missed: ipa-client/ipa-install/ipa-client-install:32:from ConfigParser import RawConfigParser Thanks, fixed. Patches 721-722: ACK Patch 723: Why the "NoneType = type(None)" in parameters.py? It is used only at: ipalib/parameters.py:388:type = NoneType # Ouch, this wont be very useful in the real world! I believe this is less confusing than `type = type(None)`, but I can change that if needed. I don't care which one is used TBH, just that it is done consistently accross the whole patch, and this seemed like the simpler thing to do. Patch 724: The SSHPublicKey class was written with the assumption that "str" means binary data, so unless I'm missing something, you only need to replace "str" with "bytes". It specifically did take non-binary data as str: -if isinstance(key, str) and key[:3] != '\0\0\0': -key = key.decode(encoding) I don't follow, this is quite obviously binary data. It reads: "If key is binary and does not start with 3 null bytes, decode it to text using the specified encoding." I've removed this for Python 3, where text data shouldn't be in bytes. Since this means the '\0\0\0' check is skipped in __init__ under Python 3, I've added it also to _parse_raw. When the SSH integration feature was first introduced, SSH public keys were stored in the raw binary form in LDAP, i.e. not text data. We still need to support that, so support for binary data and the 3 null check must remain in SSHPublicKey. It's not necessary to dispatch to "_parse_raw" or "_parse_base64 or _parse_openssh" based on type, but I believe this makes the control flow clearer to follow. Patch 725: ACK -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] More Python 3 porting
On 09/30/2015 10:25 AM, Petr Viktorin wrote: On 09/23/2015 04:46 PM, Petr Viktorin wrote: On 09/22/2015 02:59 PM, David Kupka wrote: On 18/09/15 17:00, Petr Viktorin wrote: Hello, Here are more patches that bring IPA closer to Python 3 compatibility. Hi Petr, thanks for another batch of Python 3 compatibility patches. Unfortunately I hit a lot of pylint errors. Some of them are false positives for sure. Could you please look at them, mark the false positive with "pylint: disable=E" directive and fix the rest? http://fpaste.org/270090/92665414/ Thanks. I'm actually having some trouble running pylint on an f23 machine; have you seen this error before? $ ./make-lint Traceback (most recent call last): File "./make-lint", line 280, in sys.exit(main()) File "./make-lint", line 251, in main linter.check(files) File "/usr/lib/python2.7/site-packages/pylint/lint.py", line 747, in check self._do_check(files_or_modules) File "/usr/lib/python2.7/site-packages/pylint/lint.py", line 869, in _do_check self.check_astroid_module(ast_node, walker, rawcheckers, tokencheckers) File "/usr/lib/python2.7/site-packages/pylint/lint.py", line 924, in check_astroid_module tokens = utils.tokenize_module(ast_node) File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 137, in tokenize_module with module.stream() as stream: AttributeError: 'Module' object has no attribute 'stream' Anyway, I've ran pylint on f21. Updated patches attached. ping, could someone take a look at the patches? LGTM I ran xmlrpc tests, DNSSEC ci tests, backup and restore CI test and everything works -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] More Python 3 porting
On 09/23/2015 04:46 PM, Petr Viktorin wrote: > On 09/22/2015 02:59 PM, David Kupka wrote: >> On 18/09/15 17:00, Petr Viktorin wrote: >>> Hello, >>> Here are more patches that bring IPA closer to Python 3 compatibility. >>> >>> >>> >>> >> >> Hi Petr, >> thanks for another batch of Python 3 compatibility patches. >> Unfortunately I hit a lot of pylint errors. Some of them are false >> positives for sure. Could you please look at them, mark the false >> positive with "pylint: disable=E" directive and fix the rest? >> >> http://fpaste.org/270090/92665414/ >> > > Thanks. > I'm actually having some trouble running pylint on an f23 machine; have > you seen this error before? > > $ ./make-lint > Traceback (most recent call last): > File "./make-lint", line 280, in > sys.exit(main()) > File "./make-lint", line 251, in main > linter.check(files) > File "/usr/lib/python2.7/site-packages/pylint/lint.py", line 747, in check > self._do_check(files_or_modules) > File "/usr/lib/python2.7/site-packages/pylint/lint.py", line 869, in > _do_check > self.check_astroid_module(ast_node, walker, rawcheckers, tokencheckers) > File "/usr/lib/python2.7/site-packages/pylint/lint.py", line 924, in > check_astroid_module > tokens = utils.tokenize_module(ast_node) > File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 137, in > tokenize_module > with module.stream() as stream: > AttributeError: 'Module' object has no attribute 'stream' > > > Anyway, I've ran pylint on f21. Updated patches attached. ping, could someone take a look at the patches? -- Petr Viktorin -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] More Python 3 porting
On 18/09/15 17:00, Petr Viktorin wrote: Hello, Here are more patches that bring IPA closer to Python 3 compatibility. Hi Petr, thanks for another batch of Python 3 compatibility patches. Unfortunately I hit a lot of pylint errors. Some of them are false positives for sure. Could you please look at them, mark the false positive with "pylint: disable=E" directive and fix the rest? http://fpaste.org/270090/92665414/ And one nitpick, I believe that the plus signs are not needed. -self.arabic_hello_utf8 = '\xd9\x85\xd9\x83\xd9\x8a\xd9\x84' + \ - '\xd8\xb9\x20\xd9\x85\xd8\xa7\xd9' + \ - '\x84\xd9\x91\xd8\xb3\xd9\x84\xd8\xa7' +self.arabic_hello_utf8 = (b'\xd9\x85\xd9\x83\xd9\x8a\xd9\x84' + + b'\xd8\xb9\x20\xd9\x85\xd8\xa7\xd9' + + b'\x84\xd9\x91\xd8\xb3\xd9\x84\xd8\xa7') -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
