Hi, is there any plan/estimate of making libuser compatible with Python 3? I'm currently trying to figure out what it'd take to move Fedora to Python 3 and since libuser-python is one of Anaconda's deps, it'd need to be converted as one of the first. Thanks.
Hello, "no, currently there isn't a plan". This wasn't just a simple question, though, was it?
I don't know about anything that would make a conversion impossible, if it helps.
No, it wasn't just a simple question. So theoretically, I may be able to find some time to provide a patch. What are the criteria for accepting? (oldest Python version that you still want to support?) Thanks.
Honestly I should probably be doing the port, if only to learn the Python 3 API details. Still, a patch would definitely be appreciated.
Criteria: 0. Passes the test suite 1. Works 2. Roughly similar coding style to the existing one.
I'd like the code to build and work on unmodified F19 if possible, dropping support for older releases is not a problem.
Regardless of who would do the port, is the goal to have a package that provides both Python 2 and Python 3 bindings (=> some changes to the build process), or do you want Python 3 only?
The important thing for me is whether libuser will work with Python 3. Python 2 is not really that important (for me). On one side, backwards compatibility is good (and your users may ask for it), on the other hand supporting only one Python major release might save you some headaches.
Just FYI: I started working on this, so please don't duplicate the effort.
First rough attempt to make libuser python3 compatible libuser-python3-port.patch
Hi, so I just attached first version of patch that makes libuser compile with Python 3 and passes some hand tests. Some notes: - This patch will make libuser compile with Python 2.7 and >=3.3 (it should in theory also compile with 2.6, but I didn't try that). - I replaced all return values of type PyInt with PyLong (PyLong works both on Python 2 and 3, PyInt has been removed on Python 3) - this means that on Python 2 these will now return "long" values, too. I'm not sure whether that's acceptable for you - if not, I can easily "ifdef" these to preserve the original behaviour. - I haven't tried to solve the building problem for now, there are few hacks that I used to build and test libuser with Python 3: - In generated configure, there is a loop near comment "# Find any Python interpreter" - in that loop, I put "python3" on the first place. - On Fedora, PYTHON_CPPFLAGS need to be modified, since the include dir ends with "m", e.g. "-I/usr/include/python$(PYTHON_VERSION)m" needs to be used. It'd probably be best to get it from sysconfig.get_config_var('INCLUDEPY') dynamically. - After build, libusermodule.so needs to be renamed to libuser.so - the reason for this is that Python 2 looks for both when you use "import libuser", while Python 3 will only try to look for libuser.so. - I replaced most of the tp_getattr functions with tp_getattro that are preferred in Python 3. - I also converted some test scripts from the python/ directory, but they seem very old and use stuff that's not in libuser anymore. I didn't yet start converting tests from the tests/ directory.
As for "doing the build right" part, libreport is doing that [1] - I currently don't know about better way then they used.
Hope this helps, please feel free to contact me with any questions/issues about my patch. Thanks!
[1] https://github.com/abrt/libreport/commit/6ee995deaf7195c024d77a9ba44509b2507a6768#diff-67e997bcfdac55191033d57a16d1408a
Improved python3 patch for libuser libuser-python3-port-v2.patch
So I just attached an improved version of the patch. Things that are different from the previous version:
It'd be great if you could tell me your thoughts on the patch. Thanks!
Replying to [comment:6 bkabrda]:
In generated configure, there is a loop near comment "# Find any Python interpreter" - in that loop, I put "python3" on the first place. Just running (./configure PYTHON=/usr/bin/python3) might be good enough. After build, libusermodule.so needs to be renamed to libuser.so - the reason for this is that Python 2 looks for both when you use "import libuser", while Python 3 will only try to look for libuser.so. If so, we can just rename the file; the module name is an ABI, the file name isn’t. I also converted some test scripts from the python/ directory, but they seem very old and use stuff that's not in libuser anymore. Yeah, they are not hooked into anything and were last updated in 2002; so just dropping them might be easiest.
In generated configure, there is a loop near comment "# Find any Python interpreter" - in that loop, I put "python3" on the first place. Just running (./configure PYTHON=/usr/bin/python3) might be good enough.
After build, libusermodule.so needs to be renamed to libuser.so - the reason for this is that Python 2 looks for both when you use "import libuser", while Python 3 will only try to look for libuser.so. If so, we can just rename the file; the module name is an ABI, the file name isn’t.
I also converted some test scripts from the python/ directory, but they seem very old and use stuff that's not in libuser anymore. Yeah, they are not hooked into anything and were last updated in 2002; so just dropping them might be easiest.
Replying to [comment:7 bkabrda]:
I fixed the second hack for building by introducing PYINCLUDEDIR (extracted from Python in configure.ac and used in Makefile.am); the other two hacks for building are still needed. Minor comment: the executable can be referred to as $PYTHON, and Makefile.am should be able to use the $(...) syntax (which is somewhat better because it allows run-time overriding the configure-detected value). It'd be great if you could tell me your thoughts on the patch. Thanks! Without checking it line by line it generally looks very good, when the most severe comment I have is a plea to keep the current indentation style :)
It'd be great if you could tell me your thoughts on the patch. Thanks! Without checking it line by line it generally looks very good, when the most severe comment I have is a plea to keep the current indentation style :)
Other things that caught my eye are purely stylistical and I can clean them up if necessary: * SET_ME_PROMPT makes the code IMHO more difficult to read rather than easier. * the INITERROR macro could also probably be avoided, have an un-macroized "goto error" case and then have an #if within the function the same way "return module;" is currently #if-ed.
More generally on the build system, it seems it would after all be easiest to only support one version of Python at a time (i.e. $PYTHON), and then have the spec file build the code twice. Otherwise we would end up with having to build the module, ''and then run tests'', twice from the same Makefiles, while probably still allowing for only having one Python version available, and that would just be too ugly.
Replying to [comment:8 mitr]:
Replying to [comment:6 bkabrda]: In generated configure, there is a loop near comment "# Find any Python interpreter" - in that loop, I put "python3" on the first place. Just running (./configure PYTHON=/usr/bin/python3) might be good enough.
Ah, true :)
Yes, that would be preferrable. Python 2.6 and 2.7 will be able to import even the renamed one, so I think we could actually make it the default behaviour regardless of Python version, but I'll need to recheck this.
Should I do it or would you like this to be handled in separate patch?
Replying to [comment:7 bkabrda]: I fixed the second hack for building by introducing PYINCLUDEDIR (extracted from Python in configure.ac and used in Makefile.am); the other two hacks for building are still needed. Minor comment: the executable can be referred to as $PYTHON, and Makefile.am should be able to use the $(...) syntax (which is somewhat better because it allows run-time overriding the configure-detected value).
I fixed the second hack for building by introducing PYINCLUDEDIR (extracted from Python in configure.ac and used in Makefile.am); the other two hacks for building are still needed. Minor comment: the executable can be referred to as $PYTHON, and Makefile.am should be able to use the $(...) syntax (which is somewhat better because it allows run-time overriding the configure-detected value).
Ok, I'll fix this one.
Ah, sorry about that, I'll fix that as well.
Other things that caught my eye are purely stylistical and I can clean them up if necessary: * SET_ME_PROMPT makes the code IMHO more difficult to read rather than easier.
Hmm, it makes it more readable for me, but you're the upstream, so no problem with changing that.
the INITERROR macro could also probably be avoided, have an un-macroized "goto error" case and then have an #if within the function the same way "return module;" is currently #if-ed.
True, I can fix that.
Replying to [comment:9 mitr]:
Yeah, so that means that ./configure PYTHON=/usr/bin/whatever is the way to go, do I understand it right?
Just BTW I'll probably not be able to work on the patch during next week, but I'll try to get to it ASAP after that. Thanks a lot for the review!
Replying to [comment:10 bkabrda]:
Replying to [comment:8 mitr]: Replying to [comment:6 bkabrda]: I also converted some test scripts from the python/ directory, but they seem very old and use stuff that's not in libuser anymore. Yeah, they are not hooked into anything and were last updated in 2002; so just dropping them might be easiest. Should I do it or would you like this to be handled in separate patch? Doesn’t really matter. Arguably it’s a waste to throw it away after you have already ported it; so whatever you want to do is fine. Replying to [comment:9 mitr]: More generally on the build system, it seems it would after all be easiest to only support one version of Python at a time (i.e. $PYTHON), and then have the spec file build the code twice. Otherwise we would end up with having to build the module, ''and then run tests'', twice from the same Makefiles, while probably still allowing for only having one Python version available, and that would just be too ugly. Yeah, so that means that ./configure PYTHON=/usr/bin/whatever is the way to go, do I understand it right?
Replying to [comment:6 bkabrda]: I also converted some test scripts from the python/ directory, but they seem very old and use stuff that's not in libuser anymore. Yeah, they are not hooked into anything and were last updated in 2002; so just dropping them might be easiest.
Should I do it or would you like this to be handled in separate patch? Doesn’t really matter. Arguably it’s a waste to throw it away after you have already ported it; so whatever you want to do is fine.
I currently think that would be both quite practical and reasonably easy to use, yes. But we could do something else if this turned out to be difficult, it’s not set in stone.
Just BTW I'll probably not be able to work on the patch during next week, but I'll try to get to it ASAP after that. Same here.
Another improvement of libuser-python3 patch libuser-python3-port-v3.patch
So I just added third version of the patch. Changes: - Python bindings sofile now builds as libuser.so instead of libusermodule.so (hope I did this right, I'm not that familiar with autotools). - Fixed indentation style. - $PYTHON is used in configure.ac instead of python${am_cv_python_version}. - $(PYINCLUDEDIR) is used instead of @PYINCLUDEDIR@ in Makefile.am. - Removed SET_ME_PROMPT and INITERROR macros. Hopefully this makes the patch acceptable. Thanks!
libuser-python3-port-v4.patch libuser-python3-port-v4.patch
Updated the patch to apply on top of my development branch (which I forgot to push, so it was obviously up to me to rebase the patch), and to port the test suite to Python 3.
The test suite passes, which means that the patch is Perfect™, thanks!
I still need to go back and make the test suite run against Python 2 from the same sources as well, and read and fully understand the patch line by line.
Thank you very much. Feel free to reach out to me if you need some comments or pointers to documentation, etc.
Any estimate on how long it will take this to be merged, released and built in Fedora? Thanks!
Sorry, I don’t have a good estimate I could give with any kind of certainity. It’s been !#2 on my todo list most of the intervening time, and I keep hoping for one a bit quiet week to get this done, but so far failing.
The patch assumes that the appropriate encoding for C strings is UTF-8, when it in fact should be using the encoding specified by LC_CTYPE. Is there a convenient way to use that, particularly for PyArg_ParseTupleAndKeywords? The default “s” format uses UTF-8; then there is “es”, which requires an encoding name, but there seems to be no standard encoding name for “what LC_CTYPE specifies”.
There is locale.getpreferredencoding(), but calling that every time to parse an argument would be really awkward.
(Will research more tomorrow, just noting my progress in case somebody already knows a solution.)
If you had any time to review: The complete port (based on your patch, but also modernizing getters/setters, methods etc.) is committed upstream.
The RPM package is not yet committed, but a testing build is available at http://koji.fedoraproject.org/koji/taskinfo?taskID=9245830 . The tests are passing, but I will try to get it tested against anaconda before pushing this to rawhide to avoid the worst-case disruption from possible bugs caused by the modernization.
Released in libuser-0.61. Thanks for the patch!
Login to comment on this ticket.