GNOME Bugzilla – Bug 784528
Rewrite glib-genmarshal in Python
Last modified: 2017-07-11 10:47:01 UTC
The genmarshal utility is written C, and makes it annoying when it comes to cross-compilation. Since we're porting glib-mkenums is also being ported to Python, let's get glib-genmarshal ported as well.
Created attachment 354897 [details] [review] Rewrite glib-genmarshal in Python We're in the process or rewriting other tools in Python to reduce the number of dependencies of GLib. Additionally, making glib-genmarshal a Python script reduces the complexity when cross-compiling, as we don't need a native build to generate the marshallers.
The script behaves the same, except for the handling of using stdin as the input; Python's argparse uses '-' as the synonym for standard input. I've also taken the chance of adding a couple features: - verbose/quiet output (with colours) - allow switching to `#pragma once` in place of header guards - allow injecting an header into the generated body
Created attachment 354899 [details] [review] Rewrite glib-genmarshal in Python We're in the process or rewriting other tools in Python to reduce the number of dependencies of GLib. Additionally, making glib-genmarshal a Python script reduces the complexity when cross-compiling, as we don't need a native build to generate the marshallers.
Created attachment 354900 [details] [review] Rewrite glib-genmarshal in Python We're in the process or rewriting other tools in Python to reduce the number of dependencies of GLib. Additionally, making glib-genmarshal a Python script reduces the complexity when cross-compiling, as we don't need a native build to generate the marshallers.
Created attachment 354901 [details] [review] Rewrite glib-genmarshal in Python We're in the process or rewriting other tools in Python to reduce the number of dependencies of GLib. Additionally, making glib-genmarshal a Python script reduces the complexity when cross-compiling, as we don't need a native build to generate the marshallers.
Created attachment 354902 [details] [review] Rewrite glib-genmarshal in Python We're in the process or rewriting other tools in Python to reduce the number of dependencies of GLib. Additionally, making glib-genmarshal a Python script reduces the complexity when cross-compiling, as we don't need a native build to generate the marshallers.
(In reply to Emmanuele Bassi (:ebassi) from comment #2) > I've also taken the chance of adding a couple features: > > - verbose/quiet output (with colours) I’m sold.
Review of attachment 354902 [details] [review]: docs/reference/gobject/glib-genmarshal.xml needs updating to list the `-` = read from stdin thing, if nothing else. ::: gobject/glib-genmarshal.in @@ +76,3 @@ +STD_PREFIX = 'g_cclosure_marshal' + +# Keep it in sync with gmarshal.list Hmm, looks like we might be able to drop gmarshal.list with the combination of this patch and bug #722047. @@ +121,3 @@ + sys.stderr.write(': ') + sys.stderr.write(msg) + sys.stderr.write('\n') Nitpick: This code might be a bit clearer if it built up the string first (format()) then did a single write() call. @@ +126,3 @@ + '''Print an error, and terminate''' + print_color(msg, color=Color.RED, prefix='ERROR') + sys.exit(1) Do we want to provide different exit statuses for different errors? The C version used statuses 0, 1, 2 and EXIT_FAILURE. @@ +175,3 @@ + pass + +IN_ARGS = { Nitpick: Some documentation for what IN_ARGS means and what its fields are would be useful. @@ +183,3 @@ + 'signal': 'BOOLEAN', + 'ctype': 'gboolean', + 'getter': 'g_marshal_value_peek_boolean' Nitpick: Would be more friendly to have a trailing comma on all of these entries, to reduce future diff noise. @@ +304,3 @@ +} + +OUT_ARGS = { Nitpick: Similarly, some documentation for OUT_ARGS would also be nice. @@ +308,3 @@ + 'signal': 'VOID', + 'ctype': 'void', + 'setter': None Nitpick: Add trailing commas. @@ +469,3 @@ + signature += ['G_GNUC_INTERNAL'] + elif visibility == Visibility.EXTERN: + signature += ['extern'] Hmm, should this be G_MODULE_EXPORT instead? Thinking about bug #778287.
Created attachment 354920 [details] [review] Rewrite glib-genmarshal in Python We're in the process or rewriting other tools in Python to reduce the number of dependencies of GLib. Additionally, making glib-genmarshal a Python script reduces the complexity when cross-compiling, as we don't need a native build to generate the marshallers.
(In reply to Philip Withnall from comment #8) > Review of attachment 354902 [details] [review] [review]: > > docs/reference/gobject/glib-genmarshal.xml needs updating to list the `-` = > read from stdin thing, if nothing else. Indeed. Question: do you prefer incremental patches and then a squash, or should I squash the changes and re-attach? > ::: gobject/glib-genmarshal.in > @@ +76,3 @@ > +STD_PREFIX = 'g_cclosure_marshal' > + > +# Keep it in sync with gmarshal.list > > Hmm, looks like we might be able to drop gmarshal.list with the combination > of this patch and bug #722047. Indeed; plus, gmarshal.list is only used to generate the list of marshallers that are part of our ABI, but if we want to add a new marshaller to our API we will have to write it by hand, because gmarshal.[ch] are not generated any more. > Do we want to provide different exit statuses for different errors? The C > version used statuses 0, 1, 2 and EXIT_FAILURE. I'm not really sure if that buys us anything, even from a compatibility standpoint. > @@ +469,3 @@ > + signature += ['G_GNUC_INTERNAL'] > + elif visibility == Visibility.EXTERN: > + signature += ['extern'] > > Hmm, should this be G_MODULE_EXPORT instead? Thinking about bug #778287. Doesn't using G_MODULE_EXPORT automatically add a dependency on gmodule-2.0?
Created attachment 354925 [details] [review] Rewrite glib-genmarshal in Python We're in the process or rewriting other tools in Python to reduce the number of dependencies of GLib. Additionally, making glib-genmarshal a Python script reduces the complexity when cross-compiling, as we don't need a native build to generate the marshallers.
Created attachment 354926 [details] [review] Add comments to the IN_ARGS and OUT_ARGS dictionaries Explain what each key does, to improve maintainability.
Created attachment 354927 [details] [review] Use a single write() Instead of multiple write() calls, build up the buffer using a formatted string.
Created attachment 354928 [details] [review] docs: Update the glib-genmarshal man page
Newer version after code review. Changes for attachment 354925 [details] [review]: - add '-D' and '-U' command line arguments - add more comments and checks - run pylint and fix the crappy Python I invariably end up writing
The -D/-U options were added because they allow replacing the custom "inject string into the glib-genmarshal output" that GTK+ uses in its marshaller generation rules.
Created attachment 354940 [details] [review] Rewrite glib-genmarshal in Python We're in the process or rewriting other tools in Python to reduce the number of dependencies of GLib. Additionally, making glib-genmarshal a Python script reduces the complexity when cross-compiling, as we don't need a native build to generate the marshallers.
Created attachment 354941 [details] [review] Add comments to the IN_ARGS and OUT_ARGS dictionaries Explain what each key does, to improve maintainability.
Created attachment 354942 [details] [review] Use a single write() Instead of multiple write() calls, build up the buffer using a formatted string.
Created attachment 354943 [details] [review] docs: Update the glib-genmarshal man page
Created attachment 354944 [details] [review] Re-enable signal tests when cross-compiling The glib-genmarshal tool has been rewritten in Python, which means we can run it when cross-compiling.
Review of attachment 354941 [details] [review]: ++
Review of attachment 354942 [details] [review]: ++
Review of attachment 354943 [details] [review]: ::: docs/reference/gobject/glib-genmarshal.xml @@ +363,3 @@ +<listitem><para> +Includes the given file in the C source file. This option is only useful when +using the <option>--body</option> option. ‘Includes’ as in ‘add a #include’ or as in ‘actually substitutes the contents of HEADER’? @@ +372,3 @@ +Defines a C pre-processor symbol with the given value, or "1" if the +value is unset. You can use this option multiple times. This option is only +useful when using the <option>--body</option> option. Should clarify that this results in a #define at the top of the C file. Should also clarify that the #defines and #undefs are done in the same order as specified on the command line, albeit with all the #defines done first, then all the #undefs. I think it would make more sense to allow them to be intermingled. i.e. -D V1 -D V2 -U V2 -D V3 -U V4 would result in: #define V1 1 #define V2 1 #undef V2 #define V3 1 #undef V4 @@ +380,3 @@ +<listitem><para> +Undefines a C pre-processor symbol. You can use this option multiple times. +This option is only useful when using the <option>--body</option> option. Should clarify how these interact with ordering, as above.
Review of attachment 354944 [details] [review]: Nitpicks. ::: gobject/tests/Makefile.am @@ +52,3 @@ + --quiet \ + --header \ + $(srcdir)/marshalers.list Could use $< instead of $(srcdir)/marshalers.list. @@ +62,3 @@ + --quiet \ + --body \ + $(srcdir)/marshalers.list Could use $< instead of $(srcdir)/marshalers.list.
(In reply to Philip Withnall from comment #24) > Review of attachment 354943 [details] [review] [review]: > > ::: docs/reference/gobject/glib-genmarshal.xml > @@ +363,3 @@ > +<listitem><para> > +Includes the given file in the C source file. This option is only useful > when > +using the <option>--body</option> option. > > ‘Includes’ as in ‘add a #include’ or as in ‘actually substitutes the > contents of HEADER’? The former. Will clarify with a reference to `#include`. > @@ +372,3 @@ > +Defines a C pre-processor symbol with the given value, or "1" if the > +value is unset. You can use this option multiple times. This option is only > +useful when using the <option>--body</option> option. > > Should clarify that this results in a #define at the top of the C file. Okay. > Should also clarify that the #defines and #undefs are done in the same order > as specified on the command line, albeit with all the #defines done first, > then all the #undefs. I think it would make more sense to allow them to be > intermingled. i.e. -D V1 -D V2 -U V2 -D V3 -U V4 would result in: I can probably implement it. > @@ +380,3 @@ > +<listitem><para> > +Undefines a C pre-processor symbol. You can use this option multiple times. > +This option is only useful when using the <option>--body</option> option. > > Should clarify how these interact with ordering, as above. Yep.
Review of attachment 354940 [details] [review]: ::: gobject/glib-genmarshal.in @@ +129,3 @@ + +def print_warning(msg, fatal=False): + '''Print a warning, and optionall terminate''' optionally @@ +155,3 @@ + outfile.write('\n') + # XXX: Compatibility with the old C-based tool; since we're using GObject + # types and macros, we should be including this unconditionally. Why does the comment need to be an ‘XXX’? If it’s for backwards compatibility, this code path is going to be there forever. @@ +183,3 @@ + symbol = s[0] + # XXX: Python in its most successful imitation of Perl outside + # of list comprehension Why does this need an ‘XXX’?
(In reply to Philip Withnall from comment #27) > Review of attachment 354940 [details] [review] [review]: > @@ +155,3 @@ > + outfile.write('\n') > + # XXX: Compatibility with the old C-based tool; since we're using > GObject > + # types and macros, we should be including this unconditionally. > > Why does the comment need to be an ‘XXX’? If it’s for backwards > compatibility, this code path is going to be there forever. It's an XXX to raise it as an issue in code review. :-) Keeping it is perfectly fine by me, but it can also be a chance to change the behaviour. > @@ +183,3 @@ > + symbol = s[0] > + # XXX: Python in its most successful imitation of Perl outside > + # of list comprehension > > Why does this need an ‘XXX’? For some Pythonista to come up with a nicer implementation. :-)
(In reply to Emmanuele Bassi (:ebassi) from comment #15) > Newer version after code review. > > Changes for attachment 354925 [details] [review] [review]: > > … > - run pylint and fix the crappy Python I invariably end up writing Probably makes sense to also run it under python3-pep8 too, if you haven’t already. (In reply to Emmanuele Bassi (:ebassi) from comment #10) > (In reply to Philip Withnall from comment #8) > > Review of attachment 354902 [details] [review] [review] [review]: > > > > docs/reference/gobject/glib-genmarshal.xml needs updating to list the `-` = > > read from stdin thing, if nothing else. > > Indeed. > > Question: do you prefer incremental patches and then a squash, or should I > squash the changes and re-attach? The incremental patches approach is quite good, thanks. > > ::: gobject/glib-genmarshal.in > > @@ +76,3 @@ > > +STD_PREFIX = 'g_cclosure_marshal' > > + > > +# Keep it in sync with gmarshal.list > > > > Hmm, looks like we might be able to drop gmarshal.list with the combination > > of this patch and bug #722047. > > Indeed; plus, gmarshal.list is only used to generate the list of marshallers > that are part of our ABI, but if we want to add a new marshaller to our API > we will have to write it by hand, because gmarshal.[ch] are not generated > any more. ⇒ Follow up patch to drop gmarshal.list. > > Do we want to provide different exit statuses for different errors? The C > > version used statuses 0, 1, 2 and EXIT_FAILURE. > > I'm not really sure if that buys us anything, even from a compatibility > standpoint. Sure. > > @@ +469,3 @@ > > + signature += ['G_GNUC_INTERNAL'] > > + elif visibility == Visibility.EXTERN: > > + signature += ['extern'] > > > > Hmm, should this be G_MODULE_EXPORT instead? Thinking about bug #778287. > > Doesn't using G_MODULE_EXPORT automatically add a dependency on gmodule-2.0? Hmm, it does, unfortunately. I think we do want something like that, though, otherwise the generated marshaller files will produce functions which aren’t exported properly when compiling with non-default symbol visibility. We do want a way to add an __attribute__((visibility("blah"))) to them. That said, the old C-based marshaller only supported `extern`, so we should be fine with just supporting that to begin with.
(In reply to Emmanuele Bassi (:ebassi) from comment #28) > (In reply to Philip Withnall from comment #27) > > Review of attachment 354940 [details] [review] [review] [review]: > > > @@ +155,3 @@ > > + outfile.write('\n') > > + # XXX: Compatibility with the old C-based tool; since we're using > > GObject > > + # types and macros, we should be including this unconditionally. > > > > Why does the comment need to be an ‘XXX’? If it’s for backwards > > compatibility, this code path is going to be there forever. > > It's an XXX to raise it as an issue in code review. :-) > > Keeping it is perfectly fine by me, but it can also be a chance to change > the behaviour. Let’s keep it as-is. > > @@ +183,3 @@ > > + symbol = s[0] > > + # XXX: Python in its most successful imitation of Perl outside > > + # of list comprehension > > > > Why does this need an ‘XXX’? > > For some Pythonista to come up with a nicer implementation. :-) Looks good enough to me*. *I am not a Pythonista, but I am the person who’s reviewing the code.
Created attachment 354949 [details] [review] Rewrite glib-genmarshal in Python We're in the process or rewriting other tools in Python to reduce the number of dependencies of GLib. Additionally, making glib-genmarshal a Python script reduces the complexity when cross-compiling, as we don't need a native build to generate the marshallers.
Created attachment 354950 [details] [review] Add comments to the IN_ARGS and OUT_ARGS dictionaries Explain what each key does, to improve maintainability.
Created attachment 354951 [details] [review] Use a single write() Instead of multiple write() calls, build up the buffer using a formatted string.
Created attachment 354952 [details] [review] docs: Update the glib-genmarshal man page
Created attachment 354953 [details] [review] Re-enable signal tests when cross-compiling The glib-genmarshal tool has been rewritten in Python, which means we can run it when cross-compiling.
Created attachment 354954 [details] [review] Remove unused marshallers-related files We don't use gmarshal.list any more, and the generated gmarshal.strings file is not used after the Python port of glib-genmarshal.
New version: - CR changes - passed python3-pep8 as well as pylint on glib-genmarshal - could not find a way to interleave -D/-U arguments because ArgumentParser does not let me do that - updated documentation - dropped gmarshal.list and related junk; I've left the mentions in makefile.msc.in because that's going to be dropped anyway, and it's broken already - the signals test still builds fine with no changes Aside from a review from somebody with more idiomatic knowledge of Python than I do, I think this is pretty much as far as I can take this. I'll do a full local rebuild of everything with the new glib-genmarshal to check for obvious crack.
Review of attachment 354949 [details] [review]: ++
Review of attachment 354949 [details] [review]: ++ ::: gobject/glib-genmarshal.in @@ +151,3 @@ + +def print_warning(msg, fatal=False): + '''Print a warning, and optionall terminate''' Actually, this ‘optionally’ hasn’t been fixed yet. Please fix it before pushing.
Review of attachment 354950 [details] [review]: Looks good to squash into the main patch and push.
Review of attachment 354951 [details] [review]: Looks good to squash into the main patch and push.
Review of attachment 354952 [details] [review]: Looks good to squash into the main patch and push.
Review of attachment 354953 [details] [review]: Don’t squash this one into the main patch. Looks good to push with or without the nitpick from the previous review (about using $<) fixed. ::: gobject/tests/Makefile.am @@ +52,3 @@ + --quiet \ + --header \ + $(srcdir)/marshalers.list Not a fan of $<?
Review of attachment 354954 [details] [review]: Don’t squash this one into the main patch either, once its review issue has been fixed. ::: gobject/Makefile.am @@ +145,3 @@ # that don't serve as direct make target sources, i.e. they don't have # their own .lo rules and don't get publically installed +gobject_extra_sources = You could drop this variable entirely now, I think?
(In reply to Emmanuele Bassi (:ebassi) from comment #37) > New version: > > - CR changes > - passed python3-pep8 as well as pylint on glib-genmarshal > - could not find a way to interleave -D/-U arguments because > ArgumentParser does not let me do that > - updated documentation > - dropped gmarshal.list and related junk; I've left the mentions in > makefile.msc.in because that's going to be dropped anyway, and it's broken > already > - the signals test still builds fine with no changes Thanks, it looks good to me apart from the need to squash some of the commits and fix a couple of nitpicks (don’t need to do an extra round of review for those). > I'll do a full local rebuild of everything with the new glib-genmarshal to > check for obvious crack. Find any crack?
(In reply to Philip Withnall from comment #45) > (In reply to Emmanuele Bassi (:ebassi) from comment #37) > > New version: > > > > - CR changes > > - passed python3-pep8 as well as pylint on glib-genmarshal > > - could not find a way to interleave -D/-U arguments because > > ArgumentParser does not let me do that > > - updated documentation > > - dropped gmarshal.list and related junk; I've left the mentions in > > makefile.msc.in because that's going to be dropped anyway, and it's broken > > already > > - the signals test still builds fine with no changes > > Thanks, it looks good to me apart from the need to squash some of the > commits and fix a couple of nitpicks (don’t need to do an extra round of > review for those). Thanks. > > I'll do a full local rebuild of everything with the new glib-genmarshal to > > check for obvious crack. > > Find any crack? Yes, a couple of issues, but I was able to rebuild everything up to gnome-shell.
I'll have to refrain pushing these for a bit, because I found out that a handful of projects in the GNOME moduleset use: glib-genmarshal --head --body list > header as a way to hack around the missing prototypes. I always thought that --head and --body were mutually exclusive, and considering the generated code in that case, I think it's entirely appropriate for them to be exclusive. I'll submit patches to change the projects in question, before landing these.
(In reply to Emmanuele Bassi (:ebassi) from comment #47) > I'll have to refrain pushing these for a bit, because I found out that a > handful of projects in the GNOME moduleset use: > > glib-genmarshal --head --body list > header > > as a way to hack around the missing prototypes. I always thought that --head > and --body were mutually exclusive, and considering the generated code in > that case, I think it's entirely appropriate for them to be exclusive. I'll > submit patches to change the projects in question, before landing these. Nice one
I've thought about it some more, this afternoon, and decided to allow calling `--header --body` as a (deprecated) alias for `--body --prototypes`. The new glib-genmarshal will (never fatally) warn about it, and do the right thing. This means that `--prototypes` cannot be the default, which also means that bug 781755 will regress. For that, I'm going to port projects to use `--body --prototypes` or `--body --include-header=marshallers.h`, depending to what's more appropriate.
Attachment 354949 [details] pushed as 93f16a4 - Rewrite glib-genmarshal in Python Attachment 354953 [details] pushed as f7643a7 - Re-enable signal tests when cross-compiling Attachment 354954 [details] pushed as 9c66e65 - Remove unused marshallers-related files I've pushed to master, and fixed a couple of modules that were relying on the prototypes being generated by default.