After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 772386 - Remove gjs-internals-1.0 API
Remove gjs-internals-1.0 API
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
1.46.x
Other All
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks: 742249
 
 
Reported: 2016-10-04 01:41 UTC by Philip Chimento
Modified: 2016-10-20 22:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
extensionUtils: Remove ShellJS library (9.53 KB, patch)
2016-10-08 00:35 UTC, Philip Chimento
committed Details | Review
module: Get rid of internals API (26.40 KB, patch)
2016-10-08 01:34 UTC, Philip Chimento
committed Details | Review
build: Switch to using AX_PKG_CHECK_MODULES (3.76 KB, patch)
2016-10-08 01:34 UTC, Philip Chimento
committed Details | Review
js: Remove deprecated API that is now not public (16.34 KB, patch)
2016-10-08 01:34 UTC, Philip Chimento
committed Details | Review
WIP - Remove unused API (21.73 KB, patch)
2016-10-08 01:34 UTC, Philip Chimento
none Details | Review
extensionUtils: Use a unique 'subdir' to create new importers (1.13 KB, patch)
2016-10-17 21:22 UTC, Florian Müllner
none Details | Review
build: Remove use of gjs-internals-1.0.pc (1.40 KB, patch)
2016-10-18 00:39 UTC, Philip Chimento
committed Details | Review
extensionUtils: Use a unique 'subdir' to create new importers (1.32 KB, patch)
2016-10-18 09:08 UTC, Florian Müllner
committed Details | Review
tests: Move gjs_crash_after_timeout to test utils (10.03 KB, patch)
2016-10-20 21:39 UTC, Philip Chimento
committed Details | Review
libgjs: Remove unused API (26.77 KB, patch)
2016-10-20 21:40 UTC, Philip Chimento
committed Details | Review

Description Philip Chimento 2016-10-04 01:41:49 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
Comment 1 Cosimo Cecchi 2016-10-05 20:14:08 UTC
[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.
Comment 2 Philip Chimento 2016-10-08 00:35:41 UTC
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.
Comment 3 Philip Chimento 2016-10-08 00:55:36 UTC
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)
Comment 4 Philip Chimento 2016-10-08 01:34:31 UTC
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.
Comment 5 Philip Chimento 2016-10-08 01:34:34 UTC
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.
Comment 6 Philip Chimento 2016-10-08 01:34:38 UTC
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.
Comment 7 Philip Chimento 2016-10-08 01:34:43 UTC
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.
Comment 8 Cosimo Cecchi 2016-10-10 23:13:37 UTC
(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?
Comment 9 Florian Müllner 2016-10-15 20:59:17 UTC
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 ...
Comment 10 Philip Chimento 2016-10-17 19:35:41 UTC
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 11 Philip Chimento 2016-10-17 19:39:18 UTC
Comment on attachment 337202 [details] [review]
extensionUtils: Remove ShellJS library

Attachment 337202 [details] pushed as ed99bef - extensionUtils: Remove ShellJS library
Comment 12 Florian Müllner 2016-10-17 21:22:56 UTC
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.
Comment 13 Philip Chimento 2016-10-18 00:38:26 UTC
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.
Comment 14 Philip Chimento 2016-10-18 00:39:52 UTC
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.
Comment 15 Cosimo Cecchi 2016-10-18 00:47:04 UTC
Review of attachment 337204 [details] [review]:

This looks good to me.
Comment 16 Cosimo Cecchi 2016-10-18 00:50:39 UTC
Review of attachment 337205 [details] [review]:

Looks good to me.
Comment 17 Cosimo Cecchi 2016-10-18 00:54:16 UTC
Review of attachment 337206 [details] [review]:

Sure
Comment 18 Florian Müllner 2016-10-18 09:08:52 UTC
Created attachment 337921 [details] [review]
extensionUtils: Use a unique 'subdir' to create new importers

Sure, that works for me as well ...
Comment 19 Florian Müllner 2016-10-18 09:09:02 UTC
Review of attachment 337910 [details] [review]:

Sure
Comment 20 Philip Chimento 2016-10-18 17:17:51 UTC
Review of attachment 337921 [details] [review]:

Yes, I tested this with two extensions this time :-)
Comment 21 Florian Müllner 2016-10-18 17:20:03 UTC
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 22 Philip Chimento 2016-10-18 17:23:18 UTC
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
Comment 23 Philip Chimento 2016-10-18 18:12:19 UTC
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
Comment 24 Philip Chimento 2016-10-18 18:20:36 UTC
Cosimo (or other GJS reviewers), before I put more effort into the last patch, what do you think of it?
Comment 25 Cosimo Cecchi 2016-10-19 23:38:42 UTC
(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.
Comment 26 Philip Chimento 2016-10-20 21:39:42 UTC
Created attachment 338130 [details] [review]
tests: Move gjs_crash_after_timeout to test utils

This function doesn't belong in libgjs.
Comment 27 Philip Chimento 2016-10-20 21:40:05 UTC
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.
Comment 28 Philip Chimento 2016-10-20 21:41:36 UTC
OK, here's another -400 LOC
Comment 29 Cosimo Cecchi 2016-10-20 21:49:51 UTC
Review of attachment 338130 [details] [review]:

Sure thing.
Comment 30 Cosimo Cecchi 2016-10-20 21:52:04 UTC
Review of attachment 338131 [details] [review]:

Looks good!
Comment 31 Philip Chimento 2016-10-20 22:09:05 UTC
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