Hey all, for past few days, I've been working on python3-compat patch for sssd. The patch [1] is attached to the issue that requests this feature [2]. Some comments: - The patch tries to maintain backward compatibility for Python 2.6 and 2.7 - I admit I only tested 2.7 so far, but it seemed to work ok. - I didn't yet solve the build part - for now, I just handedit configure.ac to set "PYTHON=python3" and src/external/python.m4 to set "AC_PATH_PROG(PYTHON, python3)" and compile with that. - I managed to run Python tests in src/tests except python-test.py itself (it says "OSError: [Errno 5] Could not initialize connection to the confdb" and I didn't yet investigate why that happens). - To run tests, one must modify the hashbangs to /usr/bin/python3 (I now see that I left one of these in the actual patch, so please disregard that).
I'd like to ask you, sssd devels, to give me some comments on the patch (or ask questions) and if you have some time to spare, doing more extensive testing would also be very welcome.
On Tue, Jul 08, 2014 at 10:38:00AM -0400, Bohuslav Kabrda wrote:
Hey all, for past few days, I've been working on python3-compat patch for sssd. The patch [1] is attached to the issue that requests this feature [2].
Awesome, thank you very much for the patch!
Some comments:
- The patch tries to maintain backward compatibility for Python 2.6 and 2.7 - I admit I only tested 2.7 so far, but it seemed to work ok.
The oldest OS release we support is RHEL-6. I haven't checked what Ubuntu uses, but I would expect at least 2.7
- I didn't yet solve the build part - for now, I just handedit
configure.ac to set "PYTHON=python3" and src/external/python.m4 to set "AC_PATH_PROG(PYTHON, python3)" and compile with that.
This is fine for testing, but do you have some experience from other projects on the best approach? I think we can help with coding up the build system part, I'm just not sure what the right approach is..
- I managed to run Python tests in src/tests except python-test.py itself (it says "OSError: [Errno 5] Could not initialize connection to the confdb" and I didn't yet investigate why that happens).
Yeah, don't worry about that one, it requires root currently because the files are owned by root. I wonder if we should remove that test and the python sysdb API altogether, I don't think anyone ever uses it.
- To run tests, one must modify the hashbangs to /usr/bin/python3 (I now see that I left one of these in the actual patch, so please disregard that).
Maybe I miss something here, but if the changes are backwards compatible, why does it matter to use python3 explicitly?
I'd like to ask you, sssd devels, to give me some comments on the patch (or ask questions) and if you have some time to spare, doing more extensive testing would also be very welcome.
-- Regards, Bohuslav "Slavek" Kabrda.
[1] https://fedorahosted.org/sssd/attachment/ticket/2017/sssd-python3-compat.pat... [2] https://fedorahosted.org/sssd/ticket/2017 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
----- Original Message -----
On Tue, Jul 08, 2014 at 10:38:00AM -0400, Bohuslav Kabrda wrote:
Some comments:
- The patch tries to maintain backward compatibility for Python 2.6 and 2.7
- I admit I only tested 2.7 so far, but it seemed to work ok.
The oldest OS release we support is RHEL-6. I haven't checked what Ubuntu uses, but I would expect at least 2.7
Yeah, RHEL 6 has 2.6, so we should be fine there. I'll do the testing as soon as I find some time for it :)
- I didn't yet solve the build part - for now, I just handedit
configure.ac to set "PYTHON=python3" and src/external/python.m4 to set "AC_PATH_PROG(PYTHON, python3)" and compile with that.
This is fine for testing, but do you have some experience from other projects on the best approach? I think we can help with coding up the build system part, I'm just not sure what the right approach is..
So, there are basically two different approaches to this and I don't think any of them is "the best". I do admit I prefer the second one though: 1) configure can be modified to accept an argument for --with-python, e.g. user would be able to use "--with-python=python2" or "--with-python=python3"; you have to do two builds if you want to build for two Python releases 2) configure can be modified to accept a "--with-python3" option in addition to "--with-python" (libreport is doing something that is closer to 2), although they strictly require both Python major versions and don't build if one is not present [3].
- I managed to run Python tests in src/tests except python-test.py itself
(it says "OSError: [Errno 5] Could not initialize connection to the confdb" and I didn't yet investigate why that happens).
Yeah, don't worry about that one, it requires root currently because the files are owned by root. I wonder if we should remove that test and the python sysdb API altogether, I don't think anyone ever uses it.
Ok, thanks for the info.
- To run tests, one must modify the hashbangs to /usr/bin/python3 (I now
see that I left one of these in the actual patch, so please disregard that).
Maybe I miss something here, but if the changes are backwards compatible, why does it matter to use python3 explicitly?
So, the way I think this should be done is that you run tests for all versions for Python version(s) that you're currently building for. The tests have explicit hashbang that refers to /usr/bin/python2, so you have to modify that when running tests for Python 3. The way I think this should be done systematically is, that you should invoke tests using the Python interpreter executable found by configure - for example, invoke "/usr/bin/python2 src/test/foo-test.py" rather than just "foo-test.py".
I'd like to ask you, sssd devels, to give me some comments on the patch (or ask questions) and if you have some time to spare, doing more extensive testing would also be very welcome.
-- Regards, Bohuslav "Slavek" Kabrda.
[1] https://fedorahosted.org/sssd/attachment/ticket/2017/sssd-python3-compat.pat... [2] https://fedorahosted.org/sssd/ticket/2017
[3] https://github.com/abrt/libreport/commit/6ee995deaf7195c024d77a9ba44509b2507...
Thanks for the feedback, Slavek
On Wed, Jul 09, 2014 at 08:43:27AM -0400, Bohuslav Kabrda wrote:
----- Original Message -----
On Tue, Jul 08, 2014 at 10:38:00AM -0400, Bohuslav Kabrda wrote:
Some comments:
- The patch tries to maintain backward compatibility for Python 2.6 and 2.7
- I admit I only tested 2.7 so far, but it seemed to work ok.
The oldest OS release we support is RHEL-6. I haven't checked what Ubuntu uses, but I would expect at least 2.7
Yeah, RHEL 6 has 2.6, so we should be fine there. I'll do the testing as soon as I find some time for it :)
- I didn't yet solve the build part - for now, I just handedit
configure.ac to set "PYTHON=python3" and src/external/python.m4 to set "AC_PATH_PROG(PYTHON, python3)" and compile with that.
This is fine for testing, but do you have some experience from other projects on the best approach? I think we can help with coding up the build system part, I'm just not sure what the right approach is..
So, there are basically two different approaches to this and I don't think any of them is "the best". I do admit I prefer the second one though:
- configure can be modified to accept an argument for --with-python, e.g. user would be able to use "--with-python=python2" or "--with-python=python3"; you have to do two builds if you want to build for two Python releases
- configure can be modified to accept a "--with-python3" option in addition to "--with-python"
One more question -- why do you see as preferable to select the python version we are building for? With your patch, the code would run equally well with /usr/bin/python pointing to python2 and python3, right? So can we just use whatever /usr/bin/python points to and be happy? Why should the maintainer select one (or even two) python releases?
(libreport is doing something that is closer to 2), although they strictly require both Python major versions and don't build if one is not present [3].
I don't think we should require both python versions.
- I managed to run Python tests in src/tests except python-test.py itself
(it says "OSError: [Errno 5] Could not initialize connection to the confdb" and I didn't yet investigate why that happens).
Yeah, don't worry about that one, it requires root currently because the files are owned by root. I wonder if we should remove that test and the python sysdb API altogether, I don't think anyone ever uses it.
Ok, thanks for the info.
- To run tests, one must modify the hashbangs to /usr/bin/python3 (I now
see that I left one of these in the actual patch, so please disregard that).
Maybe I miss something here, but if the changes are backwards compatible, why does it matter to use python3 explicitly?
So, the way I think this should be done is that you run tests for all versions for Python version(s) that you're currently building for. The tests have explicit hashbang that refers to /usr/bin/python2, so you have to modify that when running tests for Python 3. The way I think this should be done systematically is, that you should invoke tests using the Python interpreter executable found by configure - for example, invoke "/usr/bin/python2 src/test/foo-test.py" rather than just "foo-test.py".
I'd like to ask you, sssd devels, to give me some comments on the patch (or ask questions) and if you have some time to spare, doing more extensive testing would also be very welcome.
-- Regards, Bohuslav "Slavek" Kabrda.
[1] https://fedorahosted.org/sssd/attachment/ticket/2017/sssd-python3-compat.pat... [2] https://fedorahosted.org/sssd/ticket/2017
[3] https://github.com/abrt/libreport/commit/6ee995deaf7195c024d77a9ba44509b2507...
Thanks for the feedback, Slavek
----- Original Message -----
On Wed, Jul 09, 2014 at 08:43:27AM -0400, Bohuslav Kabrda wrote:
So, there are basically two different approaches to this and I don't think any of them is "the best". I do admit I prefer the second one though:
- configure can be modified to accept an argument for --with-python, e.g.
user would be able to use "--with-python=python2" or "--with-python=python3"; you have to do two builds if you want to build for two Python releases 2) configure can be modified to accept a "--with-python3" option in addition to "--with-python"
One more question -- why do you see as preferable to select the python version we are building for? With your patch, the code would run equally well with /usr/bin/python pointing to python2 and python3, right? So can we just use whatever /usr/bin/python points to and be happy? Why should the maintainer select one (or even two) python releases?
Because some libraries/applications depending on sssd might not be ported to whichever Python sssd is compiled with. E.g. if you compiled just with python3, people wouldn't be able to import sssd from python2 and vice versa. All major distributions I know build parallel Python 2 and Python 3 stacks because of it and they try to have as much built for both Python versions as possible. Imagine you would stop building python2 bindings while half of your fedora dependencies isn't ported to Python 2 :) To sum it up, you should be able to choose what Python you're building with, IMO. Also, because of Python upstream recommendation, /usr/bin/python will probably keep pointing to python2, maybe even forever. So it will come down to each Linux distro own policy to say which Python is the "default one" and which is the "optional one".
(libreport is doing something that is closer to 2), although they strictly require both Python major versions and don't build if one is not present [3].
I don't think we should require both python versions.
I tend to agree, I just wanted to show you all options that I know of.
Thanks a lot, Slavek
On Fri, Jul 18, 2014 at 10:22:07AM -0400, Bohuslav Kabrda wrote:
----- Original Message -----
On Wed, Jul 09, 2014 at 08:43:27AM -0400, Bohuslav Kabrda wrote:
So, there are basically two different approaches to this and I don't think any of them is "the best". I do admit I prefer the second one though:
- configure can be modified to accept an argument for --with-python, e.g.
user would be able to use "--with-python=python2" or "--with-python=python3"; you have to do two builds if you want to build for two Python releases 2) configure can be modified to accept a "--with-python3" option in addition to "--with-python"
One more question -- why do you see as preferable to select the python version we are building for? With your patch, the code would run equally well with /usr/bin/python pointing to python2 and python3, right? So can we just use whatever /usr/bin/python points to and be happy? Why should the maintainer select one (or even two) python releases?
Because some libraries/applications depending on sssd might not be ported to whichever Python sssd is compiled with. E.g. if you compiled just with python3, people wouldn't be able to import sssd from python2 and vice versa. All major distributions I know build parallel Python 2 and Python 3 stacks because of it and they try to have as much built for both Python versions as possible. Imagine you would stop building python2 bindings while half of your fedora dependencies isn't ported to Python 2 :)
Ah, that's a good point, thanks.
In that case, I prefer option 2) above as well and also the suggestion to test with both selected python versions makes perfect sense now.
Thanks for the explanation! Now, to proceed with the patch you suggested, would you like us to code up the build changes?
To sum it up, you should be able to choose what Python you're building with, IMO. Also, because of Python upstream recommendation, /usr/bin/python will probably keep pointing to python2, maybe even forever. So it will come down to each Linux distro own policy to say which Python is the "default one" and which is the "optional one".
Right, but on some distributions (ArchLinux comes to mind), /usr/bin/python points to python3 already.
(libreport is doing something that is closer to 2), although they strictly require both Python major versions and don't build if one is not present [3].
I don't think we should require both python versions.
I tend to agree, I just wanted to show you all options that I know of.
Thanks a lot, Slavek
----- Original Message -----
On Fri, Jul 18, 2014 at 10:22:07AM -0400, Bohuslav Kabrda wrote:
----- Original Message -----
One more question -- why do you see as preferable to select the python version we are building for? With your patch, the code would run equally well with /usr/bin/python pointing to python2 and python3, right? So can we just use whatever /usr/bin/python points to and be happy? Why should the maintainer select one (or even two) python releases?
Because some libraries/applications depending on sssd might not be ported to whichever Python sssd is compiled with. E.g. if you compiled just with python3, people wouldn't be able to import sssd from python2 and vice versa. All major distributions I know build parallel Python 2 and Python 3 stacks because of it and they try to have as much built for both Python versions as possible. Imagine you would stop building python2 bindings while half of your fedora dependencies isn't ported to Python 2 :)
Ah, that's a good point, thanks.
In that case, I prefer option 2) above as well and also the suggestion to test with both selected python versions makes perfect sense now.
Thanks for the explanation! Now, to proceed with the patch you suggested, would you like us to code up the build changes?
I'd highly appreciate if you could do the build changes yourself, I probably won't be able to get to any coding next week or two and I think it's better to get this merged and tested sooner than later.
To sum it up, you should be able to choose what Python you're building with, IMO. Also, because of Python upstream recommendation, /usr/bin/python will probably keep pointing to python2, maybe even forever. So it will come down to each Linux distro own policy to say which Python is the "default one" and which is the "optional one".
Right, but on some distributions (ArchLinux comes to mind), /usr/bin/python points to python3 already.
Yes, and I think it's Arch only. And they actually were the reason why Python upstream created this recommendation for other distros - Fedora, for one, is going to adhere to it, so we'll keep /usr/bin/python pointing to python2 even if we'll pronounce python3 "the default" Python interpreter.
Regards, Slavek
On Fri, Jul 18, 2014 at 10:34:00AM -0400, Bohuslav Kabrda wrote:
----- Original Message -----
On Fri, Jul 18, 2014 at 10:22:07AM -0400, Bohuslav Kabrda wrote:
----- Original Message -----
One more question -- why do you see as preferable to select the python version we are building for? With your patch, the code would run equally well with /usr/bin/python pointing to python2 and python3, right? So can we just use whatever /usr/bin/python points to and be happy? Why should the maintainer select one (or even two) python releases?
Because some libraries/applications depending on sssd might not be ported to whichever Python sssd is compiled with. E.g. if you compiled just with python3, people wouldn't be able to import sssd from python2 and vice versa. All major distributions I know build parallel Python 2 and Python 3 stacks because of it and they try to have as much built for both Python versions as possible. Imagine you would stop building python2 bindings while half of your fedora dependencies isn't ported to Python 2 :)
Ah, that's a good point, thanks.
In that case, I prefer option 2) above as well and also the suggestion to test with both selected python versions makes perfect sense now.
Thanks for the explanation! Now, to proceed with the patch you suggested, would you like us to code up the build changes?
I'd highly appreciate if you could do the build changes yourself, I probably won't be able to get to any coding next week or two and I think it's better to get this merged and tested sooner than later.
OK: https://fedorahosted.org/sssd/ticket/2386
Out of curiosity, is there some Fedora Feature or a bug that tracks converting packages to Python 3? Do we need to hit some particular deadline?
To sum it up, you should be able to choose what Python you're building with, IMO. Also, because of Python upstream recommendation, /usr/bin/python will probably keep pointing to python2, maybe even forever. So it will come down to each Linux distro own policy to say which Python is the "default one" and which is the "optional one".
Right, but on some distributions (ArchLinux comes to mind), /usr/bin/python points to python3 already.
Yes, and I think it's Arch only. And they actually were the reason why Python upstream created this recommendation for other distros - Fedora, for one, is going to adhere to it, so we'll keep /usr/bin/python pointing to python2 even if we'll pronounce python3 "the default" Python interpreter.
Thanks for the clarification.
----- Original Message -----
On Fri, Jul 18, 2014 at 10:34:00AM -0400, Bohuslav Kabrda wrote:
I'd highly appreciate if you could do the build changes yourself, I probably won't be able to get to any coding next week or two and I think it's better to get this merged and tested sooner than later.
OK: https://fedorahosted.org/sssd/ticket/2386
Out of curiosity, is there some Fedora Feature or a bug that tracks converting packages to Python 3? Do we need to hit some particular deadline?
There is no hard deadline yet, doing this ASAP would however be appreciated, since there are other libraries depending on sssd that can't start porting without having sssd built with python3. We try to keep the progress tracked at [1], but it's not complete and up-to-date. We're working on a simple tool that would automate the checking for us, but it's not ready yet.
Right, but on some distributions (ArchLinux comes to mind), /usr/bin/python points to python3 already.
Yes, and I think it's Arch only. And they actually were the reason why Python upstream created this recommendation for other distros - Fedora, for one, is going to adhere to it, so we'll keep /usr/bin/python pointing to python2 even if we'll pronounce python3 "the default" Python interpreter.
Thanks for the clarification.
YW! Slavek
On Fri, Jul 18, 2014 at 11:28:21AM -0400, Bohuslav Kabrda wrote:
----- Original Message -----
On Fri, Jul 18, 2014 at 10:34:00AM -0400, Bohuslav Kabrda wrote:
I'd highly appreciate if you could do the build changes yourself, I probably won't be able to get to any coding next week or two and I think it's better to get this merged and tested sooner than later.
OK: https://fedorahosted.org/sssd/ticket/2386
Out of curiosity, is there some Fedora Feature or a bug that tracks converting packages to Python 3? Do we need to hit some particular deadline?
There is no hard deadline yet, doing this ASAP would however be appreciated, since there are other libraries depending on sssd that can't start porting without having sssd built with python3. We try to keep the progress tracked at [1], but it's not complete and up-to-date. We're working on a simple tool that would automate the checking for us, but it's not ready yet.
Right, but on some distributions (ArchLinux comes to mind), /usr/bin/python points to python3 already.
Yes, and I think it's Arch only. And they actually were the reason why Python upstream created this recommendation for other distros - Fedora, for one, is going to adhere to it, so we'll keep /usr/bin/python pointing to python2 even if we'll pronounce python3 "the default" Python interpreter.
Thanks for the clarification.
YW! Slavek
After Slavek's nag ping on IRC I rebased the patch, fixed some minor issues and made sure the patch passed make check on all platforms: http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/515/
I think we should merge it as-is when we split master and sssd-1-12.
On Fri, Dec 12, 2014 at 04:25:43PM +0100, Jakub Hrozek wrote:
After Slavek's nag ping on IRC I rebased the patch, fixed some minor issues and made sure the patch passed make check on all platforms: http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/515/
I think we should merge it as-is when we split master and sssd-1-12.
Formal ACK from me.
I'm just waiting for the CI run to finish before pushing..
On Fri, Jan 09, 2015 at 03:51:38PM +0100, Jakub Hrozek wrote:
On Fri, Dec 12, 2014 at 04:25:43PM +0100, Jakub Hrozek wrote:
After Slavek's nag ping on IRC I rebased the patch, fixed some minor issues and made sure the patch passed make check on all platforms: http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/515/
I think we should merge it as-is when we split master and sssd-1-12.
Formal ACK from me.
I'm just waiting for the CI run to finish before pushing..
CI: http://sssd-ci.duckdns.org/logs/commit/02/9a446fd2a6c4a1dd6bd040ca5dca82d469...
master: 341a00311680a440d7f979f06c34c70d86c9367a
Thank you very much for the patch.
sssd-devel@lists.fedorahosted.org