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 727379 - Warn on import without require_version()
Warn on import without require_version()
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: general
3.12.x
Other All
: Normal enhancement
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2014-03-31 10:42 UTC by Christoph Reiter (lazka)
Modified: 2015-06-02 08:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch v1 (11.87 KB, patch)
2014-09-06 20:07 UTC, Christoph Reiter (lazka)
reviewed Details | Review
patch v2 (12.66 KB, patch)
2015-01-24 19:22 UTC, Christoph Reiter (lazka)
none Details | Review
patch v3 (12.73 KB, patch)
2015-01-24 19:53 UTC, Christoph Reiter (lazka)
committed Details | Review
Add gi.PyGIWarning and use it instead (3.92 KB, patch)
2015-03-02 20:09 UTC, Christoph Reiter (lazka)
committed Details | Review

Description Christoph Reiter (lazka) 2014-03-31 10:42:48 UTC
There are now a few typelibs with multiple versions: Cogl, CoglPango, Gtk, Unity, Gst.

Not specifying the version will break applications in the future.

To reduce the number of warnings I suggest to warn on import if the namespace to load has no version set and no other namespace depending on it enforces one.

This should warn:

>>> from gi.repository import GLib

This shouldn't since Gtk-3.0 enforces GLib-2.0:

>>> import gi
>>> gi.require_version("Gtk", "3.0")
>>> from gi.repository import GLib

Case to consider: The easiest way to handle this would be to recursively require_version() all dependencies in require_version(). The following example would fail with the second require_version, but currently only does on import:

>>> gi.require_version("CoglPango", "2.0")
>>> gi.require_version("Cogl", "1.0")
Comment 1 Simon Feltman 2014-04-28 06:26:15 UTC
I think the recursive require version makes sense. I'm not totally sold on enforcing require_version all the time though. What about an import without require_version loads the latest version by default, then does a require_version for all of its dependencies?
Comment 2 Christoph Reiter (lazka) 2014-05-02 18:31:18 UTC
(In reply to comment #1)
> [..] I'm not totally sold on
> enforcing require_version all the time though. What about an import without
> require_version loads the latest version by default, then does a
> require_version for all of its dependencies?

In which case would this generate a warning?

(To clarify, I don't want to enforce require_version, I just want to know if I forgot calling it and my code will not break in the future. So I'm OK with this generating a ImportWarning for example, which isn't shown by default)
Comment 3 Simon Feltman 2014-05-03 01:40:56 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > [..] I'm not totally sold on
> > enforcing require_version all the time though. What about an import without
> > require_version loads the latest version by default, then does a
> > require_version for all of its dependencies?
> 
> In which case would this generate a warning?

What I was thinking of is already handled properly:

>>> import gi
>>> from gi.repository import Gtk
>>> gi.require_version('Gdk', '2.0')
ValueError: Namespace Gdk is already loaded with version 3.0

> (To clarify, I don't want to enforce require_version, I just want to know if I
> forgot calling it and my code will not break in the future. So I'm OK with this
> generating a ImportWarning for example, which isn't shown by default)

ImportWarning sounds good then.
Comment 4 Christoph Reiter (lazka) 2014-09-06 20:07:44 UTC
Created attachment 285588 [details] [review]
patch v1

Emit an ImportWarning if a gi.repository import could potentially load a different version due to a missing gi.require_version()

Turned out that the import machinery is smart enough to load only versions that are possible given the already happened imports. So for a GTK+3 app one require_version() at the start immediately followed by a Gtk import will get rid of the warning (which only shows with -Wall) and prevent breakage for when GTK+4 comes around.
Comment 5 Simon Feltman 2014-09-06 22:14:17 UTC
Note the patches in bug 656314. I didn't look at your patch too deeply yet but were you able to get around requiring loading the typelib and overrides for dependencies?
Comment 6 Christoph Reiter (lazka) 2014-09-07 06:50:36 UTC
Yeah, this only checks after import/typelib loading where dependencies are available and doesn't change require_version at all. So no functional change.

Should also work with patches from bug 656314

btw, I don't mind this being pushed to the next cycle..
Comment 7 Egmont Koblinger 2014-11-06 13:22:10 UTC
We were bitten by this issue at https://bugzilla.redhat.com/show_bug.cgi?id=1114379.

Vte suddenly released a new version, breaking the API and hence bumping the major version number. Installing this new version along with the old one broke the python vte apps that happily used the old version before.

This is just unacceptable.

Libc linker has for decades solved the problem of installing multiple parallel versions and using the proper one. Glib/Gtk's organization of files extended this to development files too, the developer can choose which one to work against. Then python-gir comes along, drops this princible and comes up with a terrible design that's bound to break.

The cost of a developer having to put the version number in the files is really negligible - although they won't if they are not forced to (let alone not even warned). The cost of distributions having to fix this madness is quite high. The cost of things breaking on end users' systems when installing a new package and them having to fix/workaround this is extremely huge.

I disagree with this ticket being an "enhancement" request. I think it's a critical bug. A deprecation warning should be added as soon as possible, and one or two major releases later it should be disallowed to import without version number.

This is the only way we can prevent future breakages. If it's not done, installing a new major version of a library along with the existing one will cause apps to break all the time when a library releases a new incompatible version. Gir must not let this happen.
Comment 8 Christoph Reiter (lazka) 2015-01-24 19:22:48 UTC
Created attachment 295341 [details] [review]
patch v2

Synced with trunk.

Three small changes:

1) Don't emit a warning if a previous import which generated a warning depended on the newly imported one. Previously you got two warnings for "import Gtk, Gdk"; now it's only one for "Gtk"

While both imports can result in arbitrary versions loaded, fixing the first one will also fix the second one, so the second warning just adds noise.

2) Added a small test for _get_all_dependencies

3) Some cleanup, comments
Comment 9 Simon Feltman 2015-01-24 19:27:53 UTC
Review of attachment 285588 [details] [review]:

::: gi/importer.py
@@ +61,3 @@
+@contextmanager
+def _check_require_version(namespace, stacklevel,
+    while todo:

This seems a bit on the tricky side. If I understand correctly, _active and _implicit_required are not passed by the caller but are mutated within the function, this allows the mutated versions of the variables to be maintained throughout a recursive call chain.

I don't really see a difference between this and using globals except globals would be easier for the average programmer to understand how it works :)
(actually module level variables without the explicit global marking would suffice in this case) 

Or am I misunderstanding something?

::: gi/pygi-repository.c
@@ +280,3 @@
+
+    if (!PyArg_ParseTupleAndKeywords (args, kwargs,
+    static char *kwlist[] = { "namespace", NULL };

Looks like a copy paste bug. Should be ""s:Repository.get_dependencies"

@@ +298,3 @@
+    }
+
+    }

Another option is to use g_strfreev (namespaces) and remove the g_free() from the loop. But I'm fine either way.
Comment 10 Christoph Reiter (lazka) 2015-01-24 19:53:00 UTC
(In reply to comment #9)
> Review of attachment 285588 [details] [review]:
> 
> ::: gi/importer.py
> @@ +61,3 @@
> +@contextmanager
> +def _check_require_version(namespace, stacklevel,
> +    while todo:
> 
> This seems a bit on the tricky side. If I understand correctly, _active and
> _implicit_required are not passed by the caller but are mutated within the
> function, this allows the mutated versions of the variables to be maintained
> throughout a recursive call chain.
> 
> I don't really see a difference between this and using globals except globals
> would be easier for the average programmer to understand how it works :)
> (actually module level variables without the explicit global marking would
> suffice in this case) 
> 
> Or am I misunderstanding something?

No, that's it. I prefer this style in case a global is only used by one function since it makes moving the function to another file easier. Also it's faster, but that's not relevant here. I don't mind it being global, so OK.

> ::: gi/pygi-repository.c
> @@ +280,3 @@
> +
> +    if (!PyArg_ParseTupleAndKeywords (args, kwargs,
> +    static char *kwlist[] = { "namespace", NULL };
> 
> Looks like a copy paste bug. Should be ""s:Repository.get_dependencies"

Thanks

> @@ +298,3 @@
> +    }
> +
> +    }
> 
> Another option is to use g_strfreev (namespaces) and remove the g_free() from
> the loop. But I'm fine either way.

Sure
Comment 11 Christoph Reiter (lazka) 2015-01-24 19:53:41 UTC
Created attachment 295342 [details] [review]
patch v3
Comment 12 Simon Feltman 2015-01-24 20:34:10 UTC
Comment on attachment 295342 [details] [review]
patch v3

Pushed with a different commit message. I'm going to leave this open for now,
I think Egmont raises a valid concern and we might want to gradually change this
into a harder warning and finally and an exception after a few major releases.
Comment 13 Christoph Reiter (lazka) 2015-02-03 17:06:42 UTC
Thanks.

I wouldn't mind this being a more visible warning from the start e.g. add PyGIWarning(Warning) and use that.
Comment 14 Egmont Koblinger 2015-02-03 17:19:41 UTC
Thanks guys! :)
Comment 15 Simon Feltman 2015-03-01 05:01:19 UTC
(In reply to Christoph Reiter (lazka) from comment #13)
> Thanks.
> 
> I wouldn't mind this being a more visible warning from the start e.g. add
> PyGIWarning(Warning) and use that.

Sounds good to me.
Comment 16 Christoph Reiter (lazka) 2015-03-02 20:09:05 UTC
Created attachment 298345 [details] [review]
Add gi.PyGIWarning and use it instead
Comment 17 Christoph Reiter (lazka) 2015-06-01 13:39:53 UTC
Any thoughts on this?
Comment 18 Simon Feltman 2015-06-02 05:14:59 UTC
Review of attachment 298345 [details] [review]:

Seems fine to me, thanks.