GNOME Bugzilla – Bug 732328
[patch] add python3 support
Last modified: 2015-06-11 00:30:16 UTC
Created attachment 279375 [details] [review] Make glade Python 3 compatible Hi, I've put together a simple patch to make glade Python 3 compatible (while maintaining compatibility with Python 2.6 and 2.7). The patch doesn't actually make glade build with Python 3 by default, it just makes it possible. I'd really appreciate if you could test it and possibly merge, if you don't find any issues. Thanks!
Just found out that this should block bug 684666...
I'm porting my project to python3 and we use several custom widgets. I really need glade to support python3.
Review of attachment 279375 [details] [review]: Hello Bohuslav, thanks for the patch! I did not had time to test it, but I did a quick review and have a few question that would help me merge this faster ::: m4/python.m4 @@ +90,3 @@ fi AC_SUBST(PYTHON_LIBS) AC_SUBST(PYTHON_LIB_LOC) Ok I assume you got this m4 from python3 sources right? ::: plugins/python/glade-python.c @@ +28,3 @@ +#if PY_MAJOR_VERSION >= 3 +#define IS_PY3K Why do we need IS_PY3K macro? @@ +31,3 @@ +#define PY_CHARTYPE wchar_t +#else +// include bytesobject.h to map PyBytes_* to PyString_* We prefer C style comments ;) @@ +35,3 @@ +#define PY_CHARTYPE char +#endif + Would not be better to conditionally define wchar_t ? @@ +46,3 @@ Py_InitializeEx (0); + PySys_SetArgv (1, (PY_CHARTYPE **) &argv); And simply use wchar_t here? @@ +108,3 @@ "(use PYTHONPATH env variable to specify non default paths)\n%s", PYGOBJECT_REQUIRED_MAJOR, PYGOBJECT_REQUIRED_MINOR, + PYGOBJECT_REQUIRED_MICRO, PyBytes_AsString (pvalue)); OK, quick question, whats the deal with PyString_AsString() in python 3? was it removed or deprecated?
(In reply to comment #3) > Review of attachment 279375 [details] [review]: > > Hello Bohuslav, thanks for the patch! > I did not had time to test it, > but I did a quick review and have a few question that would help me merge this > faster > > ::: m4/python.m4 > @@ +90,3 @@ > fi > AC_SUBST(PYTHON_LIBS) > AC_SUBST(PYTHON_LIB_LOC) > > Ok I assume you got this m4 from python3 sources right? I'm not sure I understand the question. If you're asking whether I got the m4 patch somewhere else, than no, I implemented it myself. Is that what you wanted to know? > ::: plugins/python/glade-python.c > @@ +28,3 @@ > > +#if PY_MAJOR_VERSION >= 3 > +#define IS_PY3K > > Why do we need IS_PY3K macro? Heh, it's true that it's actually not needed... I just put it in all libraries that I patch, since in almost all cases, it's also used throughout the source code to conditionalize string/unicode usage etc. But since it's not needed in glade, I can remove it from the patch. > @@ +31,3 @@ > +#define PY_CHARTYPE wchar_t > +#else > +// include bytesobject.h to map PyBytes_* to PyString_* > > We prefer C style comments ;) Ok, will fix :) > @@ +35,3 @@ > +#define PY_CHARTYPE char > +#endif > + > > Would not be better to conditionally define wchar_t ? Yes, that would be possible. What I wanted to achieve by this was explicitly showing to anyone reading the code that this is a macro that may hold different values. To me, wchar_t reads as "this is always some kind of wide character", but this is not the case if compiled with Python 2. So this is the reason why I used the macro, but I can use just wchar_t if you wish. > @@ +46,3 @@ > Py_InitializeEx (0); > > + PySys_SetArgv (1, (PY_CHARTYPE **) &argv); > > And simply use wchar_t here? > > @@ +108,3 @@ > "(use PYTHONPATH env variable to specify non default > paths)\n%s", > PYGOBJECT_REQUIRED_MAJOR, PYGOBJECT_REQUIRED_MINOR, > + PYGOBJECT_REQUIRED_MICRO, PyBytes_AsString (pvalue)); > > OK, quick question, whats the deal with PyString_AsString() in python 3? > was it removed or deprecated? PyString_AsString has been completely removed. In Python 3, there is only PyBytes and PyUnicode. PyBytes is, in Python 3, analogue to PyString. Some more info about this can be found at [1]. I'll attach a new patch with removed IS_PY3K macro and changed comment style in a moment. Thanks for the review! [1] https://docs.python.org/3/howto/cporting.html#str-unicode-unification
Created attachment 287497 [details] [review] Version 2 of glade Python 3 compat patch Second version of the patch with fixed comment style and removed IS_PY3K macro.
Review of attachment 287497 [details] [review]: Yeah I asked because I prefer not to review m4 macros :) So ideally I would like to keep using the macros from https://git.gnome.org/browse/pygobject/tree/m4/python.m4 Could you check if the macros from that file works? ::: plugins/python/glade-python.c @@ +45,3 @@ Py_InitializeEx (0); + PySys_SetArgv (1, (PY_CHARTYPE **) &argv); ok so argv here comes from glib so it means its UTF8 PySys_SetArgv in python 3 wants wchar_t so we can not simply cast it, right? so to convert it properly I think we should do something like this wchar_t *argv = PyUnicode_AsWideCharString (PyUnicode_FromString (g_get_prgname ()), NULL) plus the coresponding frees :) @@ +88,3 @@ const gchar *module_path; + Py_SetProgramName ((PY_CHARTYPE *) PACKAGE_NAME); Same here, btw I am not even sure if we need to call this at all :) @@ +107,3 @@ "(use PYTHONPATH env variable to specify non default paths)\n%s", PYGOBJECT_REQUIRED_MAJOR, PYGOBJECT_REQUIRED_MINOR, + PYGOBJECT_REQUIRED_MICRO, PyBytes_AsString (pvalue)); It would be nice here if we could use PyUnicode_AsString() instead Anyways it depends on what PyErr_Fetch() actually returns.
(In reply to comment #6) > Review of attachment 287497 [details] [review]: > > Yeah I asked because I prefer not to review m4 macros :) > > So ideally I would like to keep using the macros from > https://git.gnome.org/browse/pygobject/tree/m4/python.m4 > > Could you check if the macros from that file works? I think they wouldn't work. To comment on the specific points that I changed and their counterparts in the pygobject macros: - PYTHON_INCLUDES definition from pygobject should work fine. - PYTHON_LIBS from pygobject won't work, I think, since 1) On 64bit systems it should use /usr/lib64 (my version does that) 2) With python3, the libpython so file has "abi flags" [1], for example on Fedora it's libpython3.4m.so, e.g. using -lpython{PYTHON_VERSION} is not enough (it doesn't contain the "m" abi flag). My version does this. Alternatively, `${PYTHON}-config --libs` can be used, I think. - PYTHON_LIB_LOC should also point to /usr/lib64 on 64bit systems; my version does that > ::: plugins/python/glade-python.c > @@ +45,3 @@ > Py_InitializeEx (0); > > + PySys_SetArgv (1, (PY_CHARTYPE **) &argv); > > ok so argv here comes from glib so it means its UTF8 > > PySys_SetArgv in python 3 wants wchar_t so we can not simply cast it, right? > > so to convert it properly I think we should do something like this > wchar_t *argv = PyUnicode_AsWideCharString (PyUnicode_FromString (g_get_prgname > ()), NULL) > > plus the coresponding frees :) You're absolutely right, my mistake. > @@ +88,3 @@ > const gchar *module_path; > > + Py_SetProgramName ((PY_CHARTYPE *) PACKAGE_NAME); > > Same here, btw I am not even sure if we need to call this at all :) I don't think it's necessary, but it's probably a good thing to do. > @@ +107,3 @@ > "(use PYTHONPATH env variable to specify non default > paths)\n%s", > PYGOBJECT_REQUIRED_MAJOR, PYGOBJECT_REQUIRED_MINOR, > + PYGOBJECT_REQUIRED_MICRO, PyBytes_AsString (pvalue)); > > It would be nice here if we could use PyUnicode_AsString() instead > Anyways it depends on what PyErr_Fetch() actually returns. Hmm, actually I think it can return an arbitrary Python object, which makes me think that even the old code is potentially wrong. IMO the best approach would be to first use PyObject_Bytes to get Bytes representation and then get the char * from the Bytes representation. Does that sound good? Thanks [1] http://legacy.python.org/dev/peps/pep-3149/
Unfortunately, I currently have no time to work on this patch, but I'd like to encourage anyone who wants to see this implemented to take my patch and fix it according to the above notes.
Created attachment 295434 [details] [review] Version 3 of glade Python 3 compat patch Ok, so I managed to scrape some time and fix the issues from previous version of the patch, I'm attaching a new version. Hopefully everything works, I only did some simple smoketests.
Ping, is there any update about the review of patch please?
Thanks for the ping - Juan and I have been terribly busy, but it looks like the latest version has the adjustments Juan was asking for. Juan, can you just mark the patch status to commit-now or smth if the patch is OK ?
Review of attachment 295434 [details] [review]: Hi, sorry for the late response! I made some comments, if you do not have time to make a new patch, I can apply yours and do the modifications leter ::: plugins/python/glade-python.c @@ +121,1 @@ There is no need to call char_to_wchar() here since PACKAGE_NAME is a macro with a literal I think using a macro like this should be enough #define WCHAR_REAL(str) L##str #define WCHAR(str) WCHAR_REAL(str) WCHAR(PACKAGE_NAME) -> WCHAR_REAL("glade") -> L"glade" @@ +149,3 @@ "(use PYTHONPATH env variable to specify non default paths)\n%s", PYGOBJECT_REQUIRED_MAJOR, PYGOBJECT_REQUIRED_MINOR, + PYGOBJECT_REQUIRED_MICRO, pvalue_char); we should probably free pvalue_char and unref pvalue while we are at it :)
Your comments are right. Please feel free to apply my patch and do the modifications later. Since I'm not a glade developer, I'd have to read the source code again to understand that I'm doing the right things (since I've already forgotten everything from my past work on this...) - so I'm guessing this will take you much less time than it'd take me. Thanks!
Hi, whats the status of this bugzilla?
Review of attachment 295434 [details] [review]: Fixed in master. I commited this patch, did some cleanup and fixed a related issue. Now you should be able to have catalogs and modules written in python in the paths defined in the preferences dialog. I only tested it on Debian so let me know how it goes with other systems. cheers