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?

(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)

(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.

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.

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?

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..

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.

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

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.

(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

Created attachment 295342 [details] [review]
patch v3

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.

Thanks.

I wouldn't mind this being a more visible warning from the start e.g. add PyGIWarning(Warning) and use that.

Thanks guys! :)

(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.

Created attachment 298345 [details] [review]
Add gi.PyGIWarning and use it instead

Any thoughts on this?

Review of attachment 298345 [details] [review]:

Seems fine to me, thanks.

Thanks.

https://git.gnome.org/browse/pygobject/commit/?id=2048dc8d1d708abce7037f96483c6d776567d6b5