GNOME Bugzilla – Bug 779332
Rewrite mkenums in Python
Last modified: 2017-08-01 09:39:26 UTC
Created attachment 346859 [details] Python version of glib-mkenums All other dev tools are either in Python or C. The attached file converts mkenums from Perl to Python to ease maintenance and cut down on dependencies. This is a direct translation and not very pythonic. This has been done to minimize behavioral changes to the Perl version. It passes the glib test suite but requires some tweaks (e.g. the version number and python executable need to be set with the configure script).
thanks for doing this work! Getting rid of the perl dependency will be nice indeed. > It passes the glib test suite Thats nice, but I think the more relevant test will be: does this make jhbuild or continuous fall over with mkenums-related failured or not ?
Well there is only one way to find out... (sadly I have not used either so am probably the wrong person to test it) I forgot to mention that this is tested only on Python 3 but it should work on both if you add from __future__ import print_function. The Python 3 version might raise some Unicode errors on files with 8 bit characters, though.
Some unit tests for this would be useful, and would give a lot more confidence in the refactoring.
Created attachment 348430 [details] [review] Changes needed on top of attachment 346859 [details] I had to make the following changes to the python port to make it generate gioenumtypes.[ch] correctly on Windows. I haven't tried it on Linux yet; will do a full run on GStreamer and jhbuild too if possible.
Brief update: I did some more testing: --output is currently broken as well (writing to a file needs a bytes object, not a string object). Will post a patch to fix it once I figure out the best way to do that.
Noticed a regression: gio/gioenums.h /** * GTestDBusFlags: * @G_TEST_DBUS_NONE: No flags. * * Flags to define future #GTestDBus behaviour. * * Since: 2.34 */ typedef enum /*< flags >*/ { G_TEST_DBUS_NONE = 0 } GTestDBusFlags; results in: GType g_test_dbus_flags_get_type (void) { static volatile gsize g_define_type_id__volatile = 0; if (g_once_init_enter (&g_define_type_id__volatile)) { static const GEnumValue values[] = { { G_TEST_DBUS_NONE, "G_TEST_DBUS_NONE", "none" }, { 0, NULL, NULL } }; GType g_define_type_id = g_enum_register_static (g_intern_static_string ("GTestDBusFlags"), values); g_once_init_leave (&g_define_type_id__volatile, g_define_type_id); } return g_define_type_id__volatile; } i.e. it didn't pick up on the /*< flags >*/ tag and made it an enum instead.
Created attachment 351778 [details] [review] glib-mkenums: fix parsing of /*< flags >*/ annotation Fixes get_type function generation for a bunch of Flags enums which were registered as enum types before, which broke some unit tests. Problem is that the flags annotation has no value, so options.get('flags') would always return None even if it was present.
More fixes (on the wip/meson branch which contains the glib-mkenums python port): commit 2aa486b5c932a84691ca099d4c4afab7cd1e2b71 Author: Matej Knopp <matej.knopp@gmail.com> Date: Sun May 28 22:29:20 2017 +0200 glib-mkenums: add back missing --fprod option ... in glib-mkenums python port. https://bugzilla.gnome.org/show_bug.cgi?id=779332 https://bugzilla.gnome.org/show_bug.cgi?id=783198 commit 1eaf57bb3b3f36f6912427f58a3e566fd95722ce Author: Matej Knopp <matej.knopp@gmail.com> Date: Sun May 28 22:29:20 2017 +0200 glib-mkenums: Fix parsing of multiline comments ... in glib-mkenums python port. https://bugzilla.gnome.org/show_bug.cgi?id=779332 https://bugzilla.gnome.org/show_bug.cgi?id=783198
This currently fails to parse SoupHttpVersion from soup-message.h. Probably because of the nick annotations.
> This currently fails to parse SoupHttpVersion from soup-message.h. > Probably because of the nick annotations. Should be fixed by: commit c14880e13ec146dd691ca109f54e69aead9525c3 Author: Tim-Philipp Müller <tim@centricular.com> Date: Mon Jun 26 23:50:01 2017 +0100 glib-mkenums: pick up /*< nick=xyz >*/ annotation again ... in glib-mkenums python port. Was parsed correctly but then skipped due to inverted condition. https://bugzilla.gnome.org/show_bug.cgi?id=779332
The glib-mkenums rewrite landed in master, but it's *very* much broken. The port does not support Python 2.x, which is still going to be used by systems running GLib without Python 3.x. There are multiple build failures on Continuous that can be directly traced to the new glib-mkenums: * mutter: Traceback (most recent call last):
+ Trace 237653
process_file(fname)
enum_prefix = re.sub(r'(.*)([^_])\$', r'\1\2_')
Traceback (most recent call last):
Makefile:2429: recipe for target 'stamp-enum-types' failed Full build log: http://build.gnome.org/continuous/buildmaster/builds/2017/07/14/62/build/log-mutter.txt * gst-plugins-bad: ../../../gst-libs/gst/mpegts/gstmpegts-enumtypes.h:4:0: error: unterminated #ifndef #ifndef __GST_MPEGTS_ENUM_TYPES_H__\n#define __GST_MPEGTS_ENUM_TYPES_H__\n\n#include <glib-object.h>\n\nG_BEGIN_DECLS\n ^ gstmpegts-enumtypes.c:508:7: error: stray ‘\’ in program { 0, NULL, NULL }\n };\n GType g_define_type_id = g_enum_register_static ("GstMpegtsContentNibbleHi", values);\n g_once_init_leave (&g_define_type_id__volatile, g_define_type_id);\n }\n return g_define_type_id__volatile;\n}\n ^ gstmpegts-enumtypes.c:508:7: error: stray ‘\’ in program ... Full build log: http://build.gnome.org/continuous/buildmaster/builds/2017/07/14/64/build/log-gst-plugins-bad.txt
Created attachment 355671 [details] [review] glib-mkenums: unescape \n etc. in command line arguments Fixes generation of GStreamer enumtype files with autotools build.
Comment on attachment 355671 [details] [review] glib-mkenums: unescape \n etc. in command line arguments Isn't replace_specials() supposed to do this?
Attachment 355671 [details] pushed as 8451f0b - glib-mkenums: unescape \n etc. in command line arguments
There are multiple issues with this. First, apparently we now have to do `env PYTHON=python3 ./configure ...` or `--with-python=python3` to build glib. This is broken, they're not interchangable. Second, this regressed the build of ostree. I suspect a lot of people have cargo culted glib-mkenums rules like I did. <buildgnomeorg> continuous:build[20170717.25]: FAILED: (4m:34.3s): built: mesa NetworkManager failed: ostree http://build.gnome.org/continuous/buildmaster/builds/2017/07/17/25/build/ * mclasen (mclasen@108.114.201.143) has joined <ebassi> walters: Could you have a look at ostree? <ebassi> I have no idea why it's failing <walters> hum <ebassi> It's really glnx <walters> we're missing GNU_SOURCE <walters> which should have come from AC_USE_SYSTEM_EXTENSIONS <walters> weird <ebassi> Ah, it's failing in jhbuild as well :-) <walters> did autoconf get updated? <ebassi> Ah, I know what it is <ebassi> Missing include in src/libostree/ostree-enumtypes.c <ebassi> Likely fallout from the new glib-mkenums <buildgnomeorg> continuous:build[20170717.26]: FAILED: (13.5s): failed: ostree http://build.gnome.org/continuous/buildmaster/builds/2017/07/17/26/build/ <ebassi> Sorry to bother you, walters <walters> oh, yeah <walters> no worries! i should have remembered this, it bit me before too <walters> 099576ee4a8f5f23e17def823c44c5de31b14d5d <walters> https://github.com/ostreedev/ostree/commit/099576ee4a8f5f23e17def823c44c5de31b14d5d <ebassi> Right, the fix is to move the --fhead before the --template <ebassi> Sadly, glib-mkenums is full of weird behaviours, and zero tests <ebassi> Somebody uses --fhead --template, others --template --fhead, and nobody knows why stuff happens <walters> yeah i probably cargo culted this from somewhere else <ebassi> Another option is to add the #include into the template itself <walters> glib master doesn't build for me in a f26 container at all: https://paste.fedoraproject.org/paste/SK~EEVCfEnO7HFV3ZZBfdw <walters> this seems like a revert-it-and-try-again-later situation? <ebassi> Yeah, it needs python3 <ebassi> Sadly, I cannot revert it because the new glib-mkenums has been interspersed with the meson branch <ebassi> Because we like making things harder for ourselves <ebassi> I could replace glib-mkenums with the Perl version for a while <walters> you're saying I need to build glib with PYTHON=python3 ./configure now? <ebassi> Or ./configure --with-python=python3 <ebassi> At least until somebody figures out how to fix that single line :-) <walters> python2 and python3 aren't interchangable <ebassi> I know <ebassi> It should be fixable by dropping the line iteration and using a `while True: file.readline()` <ebassi> That would work on both py2 and py3 <ebassi> I think that's basically the only py3-is-not-py2 issue
From aa377212f2c2dabcb2ae16a4e485b540245be2e6 Mon Sep 17 00:00:00 2001 From: Colin Walters <walters@verbum.org> Date: Mon, 17 Jul 2017 10:56:45 -0400 Subject: [PATCH] glib-mkenums: Adapt to work with Python 2 as well Python 2 doesn't permit interleaving iterators and `.readline()`. --- gobject/glib-mkenums.in | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/gobject/glib-mkenums.in b/gobject/glib-mkenums.in index ca2baa9f9..ec1a61333 100755 --- a/gobject/glib-mkenums.in +++ b/gobject/glib-mkenums.in @@ -1,4 +1,5 @@ #!/usr/bin/env @PYTHON@ +# -*- mode: python -*- # If the code below looks horrible and unpythonic, do not panic. # @@ -115,10 +116,15 @@ def parse_entries(file, file_name): global entries, enumindex, enumname, seenbitshift, flags looking_for_name = False - for line in file: + while True: + line = file.readline() + if line == b'': + break # read lines until we have no open comments while re.search(r'/\*([^*]|\*(?!/))*$', line): - line += file.readline() + nextl = file.readline() + if nextl != b'': + line += nextl # strip comments w/o options line = re.sub(r'''/\*(?!<) @@ -396,10 +402,15 @@ def process_file(curfilename): print_warning('No file "{}" found.'.format(curfilename)) return - for line in curfile: + while True: + line = curfile.readline() + if line == b'': + break # read lines until we have no open comments while re.search(r'/\*([^*]|\*(?!/))*$', line): - line += curfile.readline() + nextl = curfile.readline() + if nextl != b'': + line += nextl # strip comments w/o options line = re.sub(r'''/\*(?!<) -- 2.13.2
So on this PR: https://github.com/ostreedev/ostree/pull/1007 Have any other projects been changed for this already? I'm worried about a glib update breaking the build of a lot of projects.
(In reply to Colin Walters from comment #15) > There are multiple issues with this. Indeed, though I think I managed to catch them all. > First, apparently we now have to do `env PYTHON=python3 ./configure ...` or > `--with-python=python3` to build glib. This is broken, they're not > interchangable. They can be, if you pick a subset. The overall goal is, of course, to just move everything to Python 3 by default. > Second, this regressed the build of ostree. I suspect a lot of people have > cargo culted glib-mkenums rules like I did. Indeed, but fewer than I expected. A lot of people either use templates or use the command line arguments, luckily, and do not mix the two. (In reply to Colin Walters from comment #17) > So on this PR: https://github.com/ostreedev/ostree/pull/1007 > > Have any other projects been changed for this already? I'm worried about a > glib update breaking the build of a lot of projects. I haven't changed other projects; the only other project that mixed both command line arguments and templates is libqmi, which did the exact opposite of what you've done: glib-mkenums \ --fhead "some include" \ --template enum.c.in \ --ftail "some other stuff" Given that there is no guarantee about *any* of this, I decided to pick one: - fhead args are prepended to the file-head stanza in the template - ftail args are appended to the file-tail stanza in the template This has been changed in glib-mkenums to allow libqmi to build, considering that it has the same template file shared among sub-libraries, with different --fhead/--ftail arguments to control header guards. Again: there's no guarantee that any of this would work with glib-mkenums. The only reason it worked up until now, and didn't break already, is that very few people have ever touched that pile of Perl in a decade.
It feels somewhat aggressive to me for glib2 to hard require python3 just for this. That said I'm not opposed. Kind of depends on the tool maintainers? I guess my vote here is to at least try to support both 2+3, and only if it becomes painful drop 2?
As far as the command line compatibility; I only glanced at this and it terrifies me =) The hoops you've already jumped through are impressive. I don't have a good sense of how many projects this affects. Maybe someone with access to a git checkout of all of gnome could do a quick search?
How to force glib-mkenums to generate enum not flags? /*< flags=0 >*/ This used to work... glib-mkenums from git master now generates *.enums.xml file with <flags id="">...</flags>, but stable version generates <enum id="">...</enum>.
mate-panel fails for this reason during fedora mass rebuild. https://koji.fedoraproject.org/koji/taskinfo?taskID=20797337 Making all in data make[2]: Entering directory '/builddir/build/BUILD/mate-panel-1.19.2/data' glib-mkenums --comments '<!-- @comment@ -->' --fhead "<schemalist>" --vhead " <@type@ id='org.mate.panel.@EnumName@'>" --vprod " <value nick='@valuenick@' value='@valuenum@'/>" --vtail " </@type@>" --ftail "</schemalist>" ../mate-panel/panel-enums-gsettings.h > org.mate.panel.enums.xml.tmp && mv org.mate.panel.enums.xml.tmp org.mate.panel.enums.xml glib-compile-schemas --strict --dry-run --schema-file=org.mate.panel.enums.xml --schema-file=org.mate.panel.gschema.xml && mkdir -p . && touch org.mate.panel.gschema.valid glib-compile-schemas --strict --dry-run --schema-file=org.mate.panel.enums.xml --schema-file=org.mate.panel.object.gschema.xml && mkdir -p . && touch org.mate.panel.object.gschema.valid glib-compile-schemas --strict --dry-run --schema-file=org.mate.panel.enums.xml --schema-file=org.mate.panel.toplevel.gschema.xml && mkdir -p . && touch org.mate.panel.toplevel.gschema.valid glib-compile-schemas --strict --dry-run --schema-file=org.mate.panel.enums.xml --schema-file=org.mate.panel.menubar.gschema.xml && mkdir -p . && touch org.mate.panel.menubar.gschema.valid org.mate.panel.toplevel.gschema.xml:24:1 Error on line 24 char 1: <enum id='org.mate.panel.PanelOrientation'> not (yet) defined.. --strict was specified; exiting. make[2]: *** [Makefile:637: org.mate.panel.toplevel.gschema.valid] Error 1 make[2]: *** Waiting for unfinished jobs.... make[2]: Leaving directory '/builddir/build/BUILD/mate-panel-1.19.2/data' make[1]: *** [Makefile:533: all-recursive] Error 1 make[1]: Leaving directory '/builddir/build/BUILD/mate-panel-1.19.2' make: *** [Makefile:465: all] Error 2 What can i do to workaround this issue, please?
Created attachment 356680 [details] [review] glib-mkenums: fix parsing of flags annotation Commit 1672678bc48c1c060d1ee6bb3df124b3e4f9ca33 should be reverted! Flags annotation can be used in two ways: 1) /*< flags >*/ or /*< flags=1 >*/ 2) /*< flags=0 >*/
Review of attachment 356680 [details] [review]: This patch does not apply.
Of course commit 1672678b should not be reverted, otherwise the enumerations that it fixed would be broken again. Supporting `flags=0|1` should be an additional commit on top of the existing code. Personally, I think that syntax is immensely broken, and completely undocumented.
(In reply to Emmanuele Bassi (:ebassi) from comment #25) > Of course commit 1672678b should not be reverted, otherwise the enumerations > that it fixed would be broken again. No. It will not be broken with my patch. if 'flags' in options and flags is None: flags = 1 This part takes care of it, no? > Supporting `flags=0|1` should be an additional commit on top of the existing > code. Would not it make it more complicated? > Personally, I think that syntax is immensely broken, and completely > undocumented. Maybe, but that is not reason to break existing code, right?
(In reply to Alberts Muktupāvels from comment #26) > (In reply to Emmanuele Bassi (:ebassi) from comment #25) > > Of course commit 1672678b should not be reverted, otherwise the enumerations > > that it fixed would be broken again. > > No. It will not be broken with my patch. > > if 'flags' in options and flags is None: > flags = 1 > > This part takes care of it, no? Except that your patch does not apply to Git master. > > Supporting `flags=0|1` should be an additional commit on top of the existing > > code. > > Would not it make it more complicated? Not really. > > Personally, I think that syntax is immensely broken, and completely > > undocumented. > > Maybe, but that is not reason to break existing code, right? Well, you're using undocumented behaviour, and you're the only person in the overall GNOME modulesets doing that, which is also why nobody noticed when porting the code.
Created attachment 356701 [details] [review] glib-mkenums: fix parsing of flags annotation
Pushed attachment 356701 [details] [review] to master, along with the revert. Thanks!