GNOME Bugzilla – Bug 722338
Provide a binary wrapper
Last modified: 2014-01-25 23:51:36 UTC
Currently our executable '${bindir}/gnome-maps' is a shall script that launches gjs-console with scripts etc passed to it. Trouble is that geoclue2 will need to identify apps with /proc/${PID}/exe (as opposed to /proc/${PID}/cmdline, which can be easily overwritten by apps themselves) and that points to the path of gjs-console binary rather. So geoclue2 will have no means of differentiating btwe gnome-weather and gnome-maps for example. Lets make gnome-maps into a real binary that uses libgjs to run its script(s). This will also resolve bug#707724 finally.
Created attachment 266840 [details] [review] Make gnome-maps a binary
Created attachment 266841 [details] [review] Make gnome-maps a binary Make gnome-maps a small C program that loads JS from an included GResource.
Couldn't come up with a good commit message. Also really unsure if I did something wrong with the autoconf stuff. I really don't know how that stuff works.
(In reply to comment #3) > Couldn't come up with a good commit message. Also really unsure if I did > something wrong with the autoconf stuff. I'll take a look. > I really don't know how that stuff > works. There is always time to learn things. :)
Review of attachment 266841 [details] [review]: ::: configure.ac @@ +23,3 @@ PKG_PROG_PKG_CONFIG([0.22]) +SHARED_PCS="gtk+-3.0 >= 3.11.4 Is Gtk actually required to build the binary? @@ +24,3 @@ +SHARED_PCS="gtk+-3.0 >= 3.11.4 + gjs-internals-1.0 >= 1.39.0 Why internals? @@ +25,3 @@ +SHARED_PCS="gtk+-3.0 >= 3.11.4 + gjs-internals-1.0 >= 1.39.0 + gobject-introspection-1.0 >= 0.10.1" * Can we use the same format as other projects here? You can look at gnome-boxes/configure.ac for example. ::: src/Makefile.am @@ +36,3 @@ + +CLEANFILES += \ + gnome-maps \ You need to use tabs, not spaces in Makefile.am. A bit annoying, I know. @@ +42,3 @@ + $(NULL) +nodist_gnome_maps_SOURCES = \ + $(top_builddir)/src/gnome-maps-data.c \ Would be nice to align these '\'. ::: src/gnome-maps.c @@ +1,1 @@ +/* -*- Mode: C; indent-tabs-mode: nil; js2-basic-offset: 4 -*- */ I'd rather name this main.c, as per conventions. @@ +47,3 @@ + &error)) + { + g_message("Failed to defined ARGV: %s", error->message); this should be critical. @@ +61,3 @@ + &error)) + { + g_message ("Execution of main.js threw exception: %s", error->message); Same here and I think we rather just show the error->message here. ::: src/gnome-maps.data.gresource.xml @@ +1,1 @@ +<?xml version="1.0" encoding="UTF-8"?> I would have put this file renaming in a separate patch
Review of attachment 266841 [details] [review]: ::: src/Makefile-js.am @@ +25,1 @@ config.js.in Some whitespace problems here, and maybe could align the backslashes.
Thanks for the reviews. New patch comes soon!
Review of attachment 266841 [details] [review]: Thanks for the review guys! ::: configure.ac @@ +23,3 @@ PKG_PROG_PKG_CONFIG([0.22]) +SHARED_PCS="gtk+-3.0 >= 3.11.4 Not sure. Will look into this. @@ +24,3 @@ +SHARED_PCS="gtk+-3.0 >= 3.11.4 + gjs-internals-1.0 >= 1.39.0 Yeah I was a bit unsure about that. After looking around it seems that I can just skip the "-internals". @@ +25,3 @@ +SHARED_PCS="gtk+-3.0 >= 3.11.4 + gjs-internals-1.0 >= 1.39.0 + gobject-introspection-1.0 >= 0.10.1" Sure! ::: src/Makefile-js.am @@ +25,1 @@ config.js.in Sure. Thanks! ::: src/Makefile.am @@ +36,3 @@ + +CLEANFILES += \ + gnome-maps \ Ah. I should set up my editor for this. @@ +42,3 @@ + $(NULL) +nodist_gnome_maps_SOURCES = \ + $(top_builddir)/src/gnome-maps-data.c \ Agree. ::: src/gnome-maps.c @@ +1,1 @@ +/* -*- Mode: C; indent-tabs-mode: nil; js2-basic-offset: 4 -*- */ Sure! The reason I did gnome-maps.gresource.xml => gnome-maps.data.gresource.xml was because there would be a name clash. Should I still do the rename? (but in a different patch as suggested). ::: src/gnome-maps.data.gresource.xml @@ +1,1 @@ +<?xml version="1.0" encoding="UTF-8"?> Yep!
Review of attachment 266841 [details] [review]: ::: configure.ac @@ +23,3 @@ PKG_PROG_PKG_CONFIG([0.22]) +SHARED_PCS="gtk+-3.0 >= 3.11.4 It was enough with gio actually.
Created attachment 266943 [details] [review] Makefile: Whitespace cleanup Indent our Makefiles using only tabs with width 8. Also keep all escaped new-lines aligned on column 72. 72 might seem like a high number but it ensures that the \'s will keep being aligned without adding realignment noise to patches in the future.
Created attachment 266944 [details] [review] Gitignore: ignore C-stuff Turning gnome-maps into a C binary produces more intermediate files. Let's ignore those.
Created attachment 266945 [details] [review] Rename data resource file Rename the data resource file so that the filename actually contains the word 'data', indicating what it is.
Created attachment 266946 [details] [review] Make gnome-maps a binary Make gnome-maps a small C program that loads JS from an included GResource.
- Split out the rename of the gresource. - Fixed the whitespace stuff in the Makefiles (while doing that I realized that our Makefiles were already full with spaces instead of tabs etc so cleaned that up in a patch also) - Added stuff to gitignore
Review of attachment 266943 [details] [review]: ack
Review of attachment 266944 [details] [review]: Apart from the order of the patch, looks good. ::: .gitignore @@ +42,3 @@ src/path.js +src/gnome-maps-js.[ch] +src/gnome-maps-data.[ch] This patch should come *after* the commit that addes these files.
Review of attachment 266945 [details] [review]: ack although in the log, i'd also mention the real reason for this change: we are about to add a seperate resource file for code.
Review of attachment 266946 [details] [review]: ack either way, assuming you tested it. ::: src/application.js @@ -78,3 @@ - let resource = Gio.Resource.load(Path.RESOURCE_DIR + '/gnome-maps.data.gresource'); - resource._register(); - How does this work now then? ::: src/main.c @@ +41,3 @@ + context = g_object_new (GJS_TYPE_CONTEXT, + "search-path", search_path, + NULL); There is a gjs_context_new_with_search_path() you want to rather use here.
Review of attachment 266944 [details] [review]: Yeah agree. I'll put the whole .gitignore after that commit.
Review of attachment 266945 [details] [review]: Sure!
Review of attachment 266946 [details] [review]: Thanks for the review! ::: src/application.js @@ -78,3 @@ - let resource = Gio.Resource.load(Path.RESOURCE_DIR + '/gnome-maps.data.gresource'); - resource._register(); - It is compiled into the gnome-maps binary as C-code so it is already "loaded". ::: src/main.c @@ +41,3 @@ + context = g_object_new (GJS_TYPE_CONTEXT, + "search-path", search_path, + NULL); Ah sure! This code is pretty much just copy-pasted from gnome-shell so I didn't really bother redoing anything.
Pushed these patches. Could you close it Zeeshan? (I will talk to av soon, promise! :))
(In reply to comment #22) > Pushed these patches. Could you close it Zeeshan? (I will talk to av soon, > promise! :)) Thanks, although this work is not so important anymore for the main reason I wanted this: bug#721887. Having said that, this still solves bug#707724 and maybe its a good idea to keep main binary as compiled binary.
Seems your patch introduces warnings: main.c: In function ‘main’: main.c:41:3: warning: passing argument 1 of ‘gjs_context_new_with_search_path’ from incompatible pointer type [enabled by default] context = gjs_context_new_with_search_path (search_path); ^ In file included from /home/zeenix/jhbuild/include/gjs-1.0/gjs/gjs.h:27:0, from main.c:25: /home/zeenix/jhbuild/include/gjs-1.0/gjs/context.h:48:13: note: expected ‘char **’ but argument is of type ‘const char **’ GjsContext* gjs_context_new_with_search_path (char **search_path); ^ CCLD gnome-maps Could you please fix those?
Colin beat me to it. :)