GNOME Bugzilla – Bug 772386
Remove gjs-internals-1.0 API
Last modified: 2016-10-20 22:09:12 UTC
I'd like to stop exposing the API in <gjs/gjs-module.h> and remove the gjs-internals-1.0.pc file from GJS. The reason is because a lot of this API uses JSAPI from SpiderMonkey, and we will have to break API every time we upgrade GJS to a new SpiderMonkey ESR release, since SpiderMonkey doesn't make any API guarantees between releases. This is in my opinion one barrier that has made it difficult to upgrade in the past. In my opinion, we should discourage packages from dipping into the JSAPI themselves, and instead stick to evaluating scripts in a particular GjsContext, or doing everything through GObject introspection. Instead I'd prefer to expose only the API through <gjs/gjs.h>, which doesn't use any internals from SpiderMonkey. [1] This would require changes in gnome-shell, which is as far as I can see the only real user of gjs-internals-1.0. (A search on GitHub yielded a few other things; Buzztrax used it but only in their testing/playground repo, Buzztrax/design; and all the various Cinnamon and Mate forks of gnome-shell use it too of course. Presumably they can either import the eventual patchset or stick to an older GJS.) The workaround in src/shell-js.cpp in gnome-shell would have to go. Instead, I propose moving that code inside GJS, into libgjsprivate, and gnome-shell could access it through imports.gi.GjsPrivate. [1] https://git.gnome.org/browse/gjs/tree/gjs/context.h
[Adding Colin too] I'm very much in favor of this. It will make the maintenance of GJS a lot easier going forward, and the value of exposing such API is dubious at least.
Created attachment 337202 [details] [review] extensionUtils: Remove ShellJS library You can define a new importer object by importing a subdirectory in GJS. This is undocumented, but it is likely to at least hold until the whole thing moves to ES6 modules, after which we'll be able to do this purely in JS with Reflect.Loader. Since this was the only thing the ShellJS library did, we can remove it altogether. This allows us to discontinue use of the gjs-internals-1.0 embedder API.
Good news; turns out we don't need any private C code at all! You can define a new importer in GJS by importing a subdirectory on an existing importer. Patch for gnome-shell is attached. This is admittedly a hack, but perhaps not a worse one than the hack already being used. And I'd say the importer code in GJS is unlikely to change until we switch to ES6 modules (which are not even in the latest SpiderMonkey yet.) If it helps I am using this hack in jasmine-gjs as well [1] I tested this on the workspace-indicator extension [2]. With this patch I'm able to use the extension and its preferences window, whereas if I replace extension.imports with something that's not an importer such as null, I get the warning on the terminal that I would have expected. However, any suggestions for better testing are welcomed - I'm not too familiar with gnome-shell code. [1] https://github.com/ptomato/jasmine-gjs/blob/master/bin/jasmine.in#L25-L28 [2] https://git.gnome.org/browse/gnome-shell-extensions/tree/extensions/workspace-indicator I'll follow this up with some patches removing the API from GJS. (I haven't heard any arguments against on this bug or on the mailing list, but still I'd be curious what other people think)
Created attachment 337204 [details] [review] module: Get rid of internals API This removes gjs-internals-1.0.pc and gjs-module.h, and makes all the module header files private, that were previously installed. Removes "do not include separately" guards from those files, and makes sure they are included with quotes instead of angle brackets. Untangles any header inclusion problems resulting from the removal of gjs-module.h.
Created attachment 337205 [details] [review] build: Switch to using AX_PKG_CHECK_MODULES This macro can automatically collect Requires and Requires.private for pkg-config files. Now that we only have one pkg-config file, it's simple to switch to it.
Created attachment 337206 [details] [review] js: Remove deprecated API that is now not public Now this API is not public anymore, the deprecated functions can be removed entirely.
Created attachment 337207 [details] [review] WIP - Remove unused API I'm not sure if we really want to remove all this, but this is what's unused. I'm putting it up here to see what people think of removing it. I haven't looked at the util/ directory yet.
(In reply to Philip Chimento from comment #2) > Created attachment 337202 [details] [review] [review] > extensionUtils: Remove ShellJS library Florian, can you take a look at this one?
Review of attachment 337202 [details] [review]: The replacement is a bit magic, but removing code (including the --as-needed hacks) is definitively a plus. So let's go with this ...
Thanks Florian! OK, I'll go ahead then. It will probably take a little longer to land the GJS patches, so we'll still have a window to easily revert the change if it turns out to break some extension.
Comment on attachment 337202 [details] [review] extensionUtils: Remove ShellJS library Attachment 337202 [details] pushed as ed99bef - extensionUtils: Remove ShellJS library
Created attachment 337900 [details] [review] extensionUtils: Use a unique 'subdir' to create new importers (In reply to Philip Chimento from comment #10) > It will probably take a little longer to land the GJS patches, > so we'll still have a window to easily revert the change if it > turns out to break some extension. It turns out that this is the case :-( I only used a single extension for testing which works fine. However the "new" importer is always the same one, so the first initialized extension/preference widget ends up replacing any other extensions. I didn't investigate yet whether this is an issue in gjs or mozjs (and whether the update to 31 will fix it), but here's a work-around that's not too horrible IMHO.
Argh, I didn't test with more than one extension either. This is a bug in GJS (well, I'm not sure whether it's a bug, but it's a quirk of the import system in any case) and the mozjs upgrade will not fix it. I reported it a while ago (https://bugzilla.gnome.org/show_bug.cgi?id=735541) but Jasper preferred to wait for ES6 modules instead of fixing the quirky behaviour to be something else equally quirky. Your patch works fine (I tested with two extensions this time :-) but this also works and is a bit more error-proof, seems to me: imports.searchPath = [extension.dir.get_parent().get_path()]; // ... extension.imports = imports[extension.uuid]; I'll also attach a patch for configure.ac because I made some modifications there too that got lost in a rebase, and I didn't notice on the original patch.
Created attachment 337910 [details] [review] build: Remove use of gjs-internals-1.0.pc Instead use gjs-1.0.pc, the gjs-internals-1.0 API is not stable across SpiderMonkey versions and is going away.
Review of attachment 337204 [details] [review]: This looks good to me.
Review of attachment 337205 [details] [review]: Looks good to me.
Review of attachment 337206 [details] [review]: Sure
Created attachment 337921 [details] [review] extensionUtils: Use a unique 'subdir' to create new importers Sure, that works for me as well ...
Review of attachment 337910 [details] [review]: Sure
Review of attachment 337921 [details] [review]: Yes, I tested this with two extensions this time :-)
Comment on attachment 337921 [details] [review] extensionUtils: Use a unique 'subdir' to create new importers Attachment 337921 [details] pushed as d769b72 - extensionUtils: Use a unique 'subdir' to create new importers
Comment on attachment 337910 [details] [review] build: Remove use of gjs-internals-1.0.pc Attachment 337910 [details] pushed as 7a29cc4 - build: Remove use of gjs-internals-1.0.pc
Attachment 337204 [details] pushed as d3e2f86 - module: Get rid of internals API Attachment 337205 [details] pushed as f0bbf8f - build: Switch to using AX_PKG_CHECK_MODULES Attachment 337206 [details] pushed as ad59f15 - js: Remove deprecated API that is now not public
Cosimo (or other GJS reviewers), before I put more effort into the last patch, what do you think of it?
(In reply to Philip Chimento from comment #24) > Cosimo (or other GJS reviewers), before I put more effort into the last > patch, what do you think of it? I'm all for it. Not much value in keeping unused private API around.
Created attachment 338130 [details] [review] tests: Move gjs_crash_after_timeout to test utils This function doesn't belong in libgjs.
Created attachment 338131 [details] [review] libgjs: Remove unused API This removes API that was previously public, now private, and is not used anywhere inside libgjs.
OK, here's another -400 LOC
Review of attachment 338130 [details] [review]: Sure thing.
Review of attachment 338131 [details] [review]: Looks good!
Attachment 338130 [details] pushed as 8376451 - tests: Move gjs_crash_after_timeout to test utils Attachment 338131 [details] pushed as 3ce2032 - libgjs: Remove unused API