GNOME Bugzilla – Bug 794284
Support whitespace stripping for JSON resources
Last modified: 2018-03-14 15:08:26 UTC
When adding a JSON file to a GResource we may want to strip the whitespace to reduce its size.
Created attachment 369610 [details] [review] Support whitespace stripping for JSON resources Similarly to how glib-compile-resources can call xmllint to eliminate whitespace in XML files to reduce their size inside a GResource, we can use json-glib-format to achieve the same result. The mechanism for using json-glib-format is the same, with a separate environment variable if we want to direct glib-compile-resources to a version of json-glib-format that is not the one in the PATH.
Small caveat: json-glib-format started supporting the `--output` argument in json-glib 1.5, which is the current unstable release.
This has significant overlap with bug #783148, which used to cover CSS and JS. I’ve just changed it to cover CSS only; let’s talk about JS here.
Review of attachment 369610 [details] [review]: ::: docs/reference/gio/glib-compile-resources.xml @@ +220,3 @@ +<term><envar>JSON_GLIB_FORMAT</envar></term> +<listitem><para> +The full path to the json-glib-format executable. This is used to preprocess s/json-glib-format/<command>json-glib-format</command>/ (and, not your bug, but the same for `xmllint` and `gdk-pixbuf-pixdata` would also be good). ::: gio/glib-compile-resources.c @@ +343,3 @@ + + tmp_file = g_strdup ("resource-XXXXXXXX"); + if ((fd = g_mkstemp (tmp_file)) == -1) Use g_file_open_tmp() and it’ll do the error reporting for you. @@ +348,3 @@ + + g_set_error (error, G_IO_ERROR, g_io_error_from_errno (errsv), + _("Failed to create temp file: %s"), s/temp/temporary/ @@ +783,3 @@ + jsonformat = g_find_program_in_path ("json-glib-format"); + if (jsonformat == NULL) + g_printerr ("JSON_GLIB_FORMAT not set and json-glib-format not found in path; skipping json pre-processing.\n"); Should this be marked for translation?
Note that the current patch will compound the (IMO) problem expressed in https://bugzilla.gnome.org/show_bug.cgi?id=794285
(In reply to Philip Withnall from comment #3) > This has significant overlap with bug #783148, which used to cover CSS and > JS. I’ve just changed it to cover CSS only; let’s talk about JS here. JS is one thing, JSON is another. At this point, we might end up with a `preprocess="command --options=@OUTPUT@ @INPUT@"` generic handler. (In reply to Philip Withnall from comment #4) > Review of attachment 369610 [details] [review] [review]: > > ::: docs/reference/gio/glib-compile-resources.xml > @@ +220,3 @@ > +<term><envar>JSON_GLIB_FORMAT</envar></term> > +<listitem><para> > +The full path to the json-glib-format executable. This is used to preprocess > > s/json-glib-format/<command>json-glib-format</command>/ (and, not your bug, > but the same for `xmllint` and `gdk-pixbuf-pixdata` would also be good). Sure. > ::: gio/glib-compile-resources.c > @@ +343,3 @@ > + > + tmp_file = g_strdup ("resource-XXXXXXXX"); > + if ((fd = g_mkstemp (tmp_file)) == -1) > > Use g_file_open_tmp() and it’ll do the error reporting for you. I just copy-pasted the xmllint bits; I can do a second pass that moves everything to g_file_open_tmp() everywhere. > @@ +348,3 @@ > + > + g_set_error (error, G_IO_ERROR, g_io_error_from_errno > (errsv), > + _("Failed to create temp file: %s"), > > s/temp/temporary/ Same as above. > @@ +783,3 @@ > + jsonformat = g_find_program_in_path ("json-glib-format"); > + if (jsonformat == NULL) > + g_printerr ("JSON_GLIB_FORMAT not set and json-glib-format not found in > path; skipping json pre-processing.\n"); > > Should this be marked for translation? The XMLLINT bit isn't, so I guess it'll need to be translated too.
Created attachment 369654 [details] [review] Use g_file_open_tmp() instead of hand-rolling it The glib-compile-resources tool has hand-rolled "open a temporary file" code paths. Since error handling is hard, let's rely on GLib API that is meant to do that consistently for us.
Created attachment 369656 [details] [review] docs: Use appropriate tags for commands The glib-compile-resources documentation refers to various command line tools, and those should be wrapped in the <command> DocBook tag.
To avoid extraneous changes in attachment 369610 [details] [review], I've decided to do the documentation updates and the code refactor as separate commits.
Review of attachment 369654 [details] [review]: r+, modulo a commit message change (which can be done when merging). ::: gio/glib-compile-resources.c @@ +319,3 @@ + if (!g_subprocess_wait_check (proc, NULL, error)) + { + g_object_unref (proc); There seems to be a lot of whitespace noise in this patch. Is that deliberate? I’m fine (overjoyed, even) with converting tabs to spaces, but at least mention it in the commit message.
Review of attachment 369656 [details] [review]: ::: docs/reference/gio/glib-compile-resources.xml @@ +202,3 @@ +The full path to the <command>xmllint</command> executable. This is used to +preprocess resources with the <literal>xml-stripblanks</literal> preprocessing +option. If this environment variable is not set, xmllint is searched in the There’s an `xmllint` here too. @@ +212,3 @@ +The full path to the <command>gdk-pixbuf-pixdata</command> executable. This is +used to preprocess resources with the <literal>to-pixdata</literal> preprocessing +option. If this environment variable is not set, gdk-pixbuf-pixdata is searched Same here. @@ +222,3 @@ +The full path to the <command>json-glib-format</command> executable. This is used +to preprocess resources with the <literal>json-stripblanks</literal> preprocessing +option. If this environment variable is not set, json-glib-format is searched in Same here. Could also fix s/searched/searched for/.
(In reply to Philip Withnall from comment #10) > Review of attachment 369654 [details] [review] [review]: > > r+, modulo a commit message change (which can be done when merging). > > ::: gio/glib-compile-resources.c > @@ +319,3 @@ > + if (!g_subprocess_wait_check (proc, NULL, error)) > + { > + g_object_unref (proc); > > There seems to be a lot of whitespace noise in this patch. Is that > deliberate? I’m fine (overjoyed, even) with converting tabs to spaces, but > at least mention it in the commit message. Yeah, I noticed that after attaching the patch; it was a side effect of moving code that had mixed tabs/spaces around.
Attachment 369610 [details] pushed as 7fd17c4 - Support whitespace stripping for JSON resources Attachment 369654 [details] pushed as 7240287 - Use g_file_open_tmp() instead of hand-rolling it Attachment 369656 [details] pushed as c5c3fa8 - docs: Use appropriate tags for commands