GNOME Bugzilla – Bug 689654
Warn if we're importing an unversioned namespace
Last modified: 2017-04-19 21:59:10 UTC
In introspection, libraries are a pair of (namespace, version). But it's possible to request "the latest" version of a namespace, and this is in fact what many projects do. The problem is that for say GTK+ which has both 2.0 and 3.0 currently, if ever a 4.0 was introduced, suddenly everyone's scripts would explode if GTK+ 4.0 happened to be installed. Let's start warning about unversioned imports, and get people to do: imports.gi.versions.Gtk = '3.0'; const Gtk = imports.gi.Gtk;
Created attachment 230695 [details] [review] Warn if we're importing an unversioned namespace
We need a more convenient syntax in this case. I don't want a warning if I'm just writing a dummy script, or playing with the gjs REPL.
(In reply to comment #2) > We need a more convenient syntax in this case. I don't want a warning if I'm > just writing a dummy script, or playing with the gjs REPL. Suggestions? We could also just make it warn for Gtk and Clutter - the two things most likely to get a second version. But that's kind of ugly.
What about warning if the major version > 1?
(In reply to comment #4) > What about warning if the major version > 1? That would work for Gtk, but not for Clutter (presently at 1.0, but I would not be surprised if there was a 2.0). Also, just a bit too magical. One other option I considered (and actually started implementing) - typelibs can contain a "default version". In gtk/Makefile.am, we do SCANNER_OPTIONS += --default-version=3.0. Then at typelib resolution time, we load the typelibs in reverse order until we find one with a default. If found, we use that. The reason we'd hardcode --default-version=3.0 instead of just --default-version is so it's less likely that when Gtk is forked for 4.0, they change that 3.0 to 4.0, rather than forgetting to delete --default-version.
Yet another alternative is something like: const Gtk = imports.gi.versions.get('Gtk', '3.0');
Could we change it so that the warning is only logged if there was more than one version of that namespace available?
Review of attachment 230695 [details] [review]: Cosimo and I discussed that warning only if multiple versions available is the desired behaviour. I'll mark the patch needs-work accordingly and fix it up one of these days.
Created attachment 349206 [details] [review] repo: Warn when importing a namespace with >1 version In introspection, libraries are a pair of (namespace, version). But it's possible to request "the latest" version of a namespace, and this is in fact what many projects do. The problem is that for say GTK+ which has 2.0, 3.0, and is just releasing 4.0, most people's scripts imported gi.Gtk and expected to receive 3.0, and will explode when installing 4.0. Let's start warning about unversioned imports when there are multiple versions available, and get people to do: imports.gi.versions.Gtk = '3.0'; const Gtk = imports.gi.Gtk; (Original patch by Colin Walters; revised by Philip Chimento.)
Review of attachment 349206 [details] [review]: ::: gi/repo.cpp @@ +109,3 @@ + JS_ReportWarning(context, warn_text); + } + GjsAutoChar warn_text = g_strdup_printf("Requiring %s but it has %u " Why not just use g_list_free_full()?
Created attachment 349269 [details] [review] repo: Warn when importing a namespace with >1 version In introspection, libraries are a pair of (namespace, version). But it's possible to request "the latest" version of a namespace, and this is in fact what many projects do. The problem is that for say GTK+ which has 2.0, 3.0, and is just releasing 4.0, most people's scripts imported gi.Gtk and expected to receive 3.0, and will explode when installing 4.0. Let's start warning about unversioned imports when there are multiple versions available, and get people to do: imports.gi.versions.Gtk = '3.0'; const Gtk = imports.gi.Gtk; (Original patch by Colin Walters; revised by Philip Chimento.)
Review of attachment 349269 [details] [review]: Thanks, looks good now.
Attachment 349269 [details] pushed as 0154c9d - repo: Warn when importing a namespace with >1 version
I made a couple of errors here: we get the warning when importing a nonexistent namespace, and we also get it when running testCairo.js.
Created attachment 349967 [details] [review] repo: Don't warn about 0 namespace versions If there are 0 namespace versions, we just need to error out. Don't print the warning about there being more than one version. Also, select the correct version in testCairo.js so we don't get the warning there either.
Review of attachment 349967 [details] [review]: Makes sense, thanks.
Attachment 349967 [details] pushed as 1e0bbae - repo: Don't warn about 0 namespace versions