Great to see things moving!

We spoke briefly about this with Simon (in CC) at Guadec. Our feeling is that these overrides belong in gstreamer itself or in ad-hoc package, because shoving everything in pygobject is not a sustainable solution.

As a general rule I also suggest to keep override at a minimum.

The changes to the convert script are surely welcome.

I tottaly agree that none of the overrides should be in pygobject itself, but it is how it is, and I didn't find how that should be done otherwise. It seems that PyGobject is only watching 'gi/overrides' for overrides, so how are we supposed to do it right now?

Thanks for your fast response :)

There are other libraries which ship their own overrides, such as libdee. They call something like

$ python3 -c 'import gi; print(gi._overridesdir)'
/usr/lib/python3/dist-packages/gi/overrides

$ python -c 'import gi; print(gi._overridesdir)'
/usr/lib/python2.7/dist-packages/gi/overrides

in their build system to find the directory to place overrides in. This is also pointed out in the pkg-config file, but unfortunately pkg-config does not provide enough dynamism to query the path from it.

The problem I see is that we can't use it 'uninstalled', would it possible to have PyGObject configure the overrides paths from a list (probably through environment variables)?

It's not a PyGObject issue, it's a Python one. Python requires all modules of a particular namespace to be in the same directory, so you cannot e. g. have "gi" in /usr/share/python/..., and "gi.overrides.GStreamer" in a different one, as far as I know. This is ugly for local testing for sure. However, perhaps there is a trick with .pth files to do that, we used something similar in Debian when we had pysupport vs. pycentral.

I did not find an immediate solution, but I guess with some tricks in gi/module.py or gi/__init__.py it ought to be possible to search for overrides in a different directory. So that should either become a new bug, or we devote that aspect to this bug report, as Simon, Pablo, and I all agree that it would be better to keep overrides in their respective projects, like libdee does.

So if the directory issue is the blocker for this, then let's try to fix that instead?

You can use pkgutil to split a package over multiple paths, see:
http://docs.python.org/library/pkgutil.html

I've also run into a gi restriction which I don't think is needed. You cannot create overrides outside of a package named "gi.overrides". This restriction doesn't seem necessary. For instance, if I want to create custom overrides in a sub-package of an app, it must live in "gi.overrides". See: pygobject/gi/overrides/__init__.py line:~26

Created attachment 220080 [details] [review]
Add a way to specify folder where to look for overrides outside of PyGobject

All that should be needed is to put:

from pkgutil import extend_path
__path__ = extend_path(__path__, __name__)

Into gi/__init__.py as well as inside of the distributed overrides package __init__.py. For instance:

..../GStreamer/
  gi/
    __ini__.py  <-- pkgutil stuff in here
    overrides/
       __init__.py
       Gst.py

Then you can use pythons standard path mechanisms (Adding GStreamer to PYTHONPATH or installing it to site-packages)

Upon a little further testing the pkgutil code will also need to live in:
gi/overrides/__init__.py in both the pygobject and the override package.

That's pretty neat, also we will need the override (in overrides/__init__) function to be public it seems.

Also wanted to make sure that I am rigth to say that some glue code in C is needed (basically a call to pyg_register_gtype_custom) to use newly define fundamental types? If it is the case I think we will need to revive gst-python as I don't see that being done in GStreamer itself, anyway something to discuss in the GStreamer community.

I'm not sure if it is a dead end but you could look into using pythons ctypes. There are also a handful of type/registration methods exposed to python in GObject and _gobject which may also be useful.

*** Bug 681217 has been marked as a duplicate of this bug. ***

While pygobject should not ship Gst.py, the same should happen with Gtk and others eventually, there's no need for an additional PYGOBJECT_OVERRIDE path, it'll just create confusion.

I'm strongly against the proposed patch.

Whatever package that ships Gst.py should just install it into gi/overrides. For the uninstalled case, just set PYTHONPATH / gi.__path__. The application needs to be modified anyway to support that.

Hi Johan,
Are you against the usage of pkutil for path extension all together or just having a new env for package overriding (as was shown in the patch)?

pkutil.extend_path will allow PYTHONPATH to work for multiple gi/override directories (and I think it would be necessary for the uninstalled case). It definitally seems to be the cleanest approach as it allows usage of the builtin python package lookup techniques.

Simon: just using pkgutil would be okay I guess, if I understand correctly an actual requirement for uninstalled modules.

Adding an environment variable is pretty horrible, you'd need to convince me harder that it is actually needed. And if it's needed it should be called something terrible such as:

    PYGOBJECT_GI_OVERRIDE_UNINSTALLED_PATH

We decided it was actually not needed as we can cleanly get the same result using pkgutil.

I tried to find out how to make GstFraction working from python using the various ways described previously but didn't find any solution. The only thing that made it working is http://cgit.collabora.com/git/user/tsaunier/gst-python/commit/?id=63d865025ef3f640316d1a18edc844fb56a16df4 but it is not optimal at all fmpov.

Do you have any other suggestions? :)

Created attachment 220724 [details] [review]
Make it possible to add overrides from outside main gi.overrides

The little patch that let us have overrides from outside main gi,overrides, I tested it in an uninstalled environment with previouly pointed branch of gst-python.

Created attachment 220806 [details] [review]
Make it possible to add overrides from outside main gi.overrides

I pushed the gst-python branch could that last patch get into pygobject?

Created attachment 221844 [details] [review]
test case (failing)

Thanks for this! I'm trying to test this (in the form of a test case), but it doesn't seem to work for me. I create a new tests/gi/overrides/Regress.py and verify in the test suite that this gets loaded properly. But the override is never imported (Regress._overrides_module is None), the __import__() in gi/module.py DynamicModule._load() does not find it.

Can you demonstrate how you tested this?

Thanks!

Make sure you have:
tests/gi/__init__.py
tests/gi/overrides/__init__.py

Both of which include:
from pkgutil import extend_path
__path__ = extend_path(__path__, __name__)

From here the pygobject/tests folder needs to either be in PYTHONPATH or added to sys.path. It might be worth it to put an additional directory between tests and gi (pygobject/tests/regress/gi) so that pygobject/tests/regress can be added as a python path and the rest of the .py files in pygobject/tests/ won't be importable (unless you want that).

Created attachment 221962 [details] [review]
(broken) patch and test

Ah, thanks Simon. I keep forgetting to create the __init__.py files, silly me.

I'm a little further now. As you suggested I moved the test override into a subdirectory, to avoid having the test_*.py scripts become importable.

Now it seems to find the override, but fails with:

Traceback (most recent call last):

+ Trace 230716

That's with python 3.2. With python 2.7 I get

======================================================================
ERROR: test_separate_path (test_overrides.TestRegistry)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/martin/upstream/pygobject/tests/test_overrides.py", line 57, in test_separate_path
    self.assertEqual(Regress.REGRESS_OVERRIDE, 42)
  File "/home/martin/upstream/pygobject/gi/module.py", line 269, in __getattr__
    return getattr(self._introspection_module, name)
  File "/home/martin/upstream/pygobject/gi/module.py", line 115, in __getattr__
    self.__name__, name))
AttributeError: 'gi.repository.Regress' object has no attribute 'REGRESS_OVERRIDE'

Review of attachment 221962 [details] [review]:

The problem is due to the order in which tests are being run. test_everything is run before test_overrides.
The issue with this is test_everything uses "from gi.repository import Regress" without the "regress" override package in sys.path yet, this causes the bare, non-overridden Regress module from gi to be cached as Regress in sys.modules. By the time test_overrides is run, it doesn't matter what is added to sys.path because the module cached in sys.modules will always be used.

This speaks to an overall problem with how the unittests are run. It seems like it would be cleaner if the test runner started a new python process for each of the "test_*" files. Otherwise, you can run into issues like this or unknown cross dependencies between the tests. For instance, running "make check TEST_NAMES=test_overrides" shows a completely different problem with this patch because test_everything is not run first. Likewise modifying PYTHONPATH will actually fix all failure cases because this adds the new regress package to sys.path at python startup time.

The problem with "make check TEST_NAMES=test_overrides" is there is code sitting at the package level of pygobject/gi/overrides/__init__.py. Which cannot be reached if "regress/gi/overrides/__init__.py" is loaded first. In large systems that utilize pkgutil, it ends up being bad practice to put code in __init__.py files because of this. sys.path ordering problems can then be avoided by limiting __init__.py to pkgutil magic.

Notice what sys.path looks like during the different runs:

Python sys.path for test_overrides during "make check TEST_NAMES=test_overrides"
['regress', '../', '/home/simon/src/gnome3/pygobject/tests', ...]

Python sys.path for test_overrides during "make check": 
['regress', '../', '../', '../', '/home/simon/src/gnome3/pygobject/tests', ...]

The "tests" folder is already in sys.path so my previous recommendation of using a "regress" sub-folder as a cleaner setup doesn't even matter. This is being set through PYTHONPATH by Makefile.in

This long winded explanation actually has a simple fix. We can remove the regress sub-folder and rely on the tests folder already being in sys.path, or append it to sys.path. In general, setup of sys.path should probably be limited to runtests.py or the makefile setting PYTHON path, otherwise you can get a fairly mucky sys.path by the end of the test suite (notice the multiple '../' entries when running with "make check" and this was only after a few tests ran).

::: gi/overrides/__init__.py
@@ +5,3 @@
+# support overrides in different directories than our gi module
+from pkgutil import extend_path
+__path__ = extend_path(__path__, __name__)

This should probably go before any non-system imports.

::: tests/test_overrides.py
@@ +7,3 @@
 sys.path.insert(0, "../")
+# also catch tests/regress/gi/overrides/
+sys.path.insert(0, "regress")

We should move all modification to sys.path to runtests.py or Makefile.in which modifies PYTHONPATH. Also "regress" path should be appended or at least be ordered after "../" so code in pygobject/gi/overrides/__init__.py is ensured to be available.

Created attachment 222201 [details] [review]
sys.path cleanup

Additionally cleaned up sys.path entries, removing all occurances of "../" from being added. This is now setup once by runtests.py.
Also moved setting of PYTHONPATH to ["pygobject/tests",...] into runtests.py.

The only issues I see is if there are people relying on the potential ability to use:
 python tests/test_everything.py
vs:
 make check TEST_NAMES=test_everything

The "../" was only added to a few files: test_everything, test_overrides, test_gdbus, and test_pygtkcompat. So all of the other test_modules were actually relying on one of these to run first in order for pygobject/ to get added to sys.path.

Thanks Simon for the analysis!

I fully agree that the sys.path handling in tests/ is confusing and impractical. As it happens, I also was about to drop these for an entirely different reason: I want to change the tests so that they can also run against the system installed libraries.

I separated this part of the change and committed it as

http://git.gnome.org/browse/pygobject/commit/?id=c7c95a795eee499373499ea5b771447746317bfb

After that, I cleaned up the patch for this actual bug, tests work fine now:

http://git.gnome.org/browse/pygobject/commit/?id=b1a9848a7a7255e6b1ccd98712dd62b1514078b9

Thanks to everyone!

The initial comment also mentioned changes to the conversion script, if there are any, feel free to open a new bug with a patch. I do not see any problems in including those.

*** Bug 660647 has been marked as a duplicate of this bug. ***

Like the original poster, I'd like to be able to install a program with overrides in a local directory.  So I was happy to see that a fix for this request was committed last year.  But after struggling to get this to work I'm unconvinced that the fix here is practical.

The essential problem is that it seems to require that the overriding directory come *after* the directory where pygobject is installed in sys.path.  At least, the unit test in pyobject/tests/gi/overrides works that way; the setup code in pygobject/tests/runtest.py contains this:

  # we have to do this here instead of Makefile.am so that the implicitly added
  # directory for the source file comes after the builddir
  sys.path.insert(0, tests_builddir)
  sys.path.insert(0, builddir)

But as far as I can tell there's no practical way that I can set up environment variables or configuration files so that a directory with my local overrides comes after the main Python package dir (e.g. /usr/lib/python3/site-packages) in sys.path on a typical system.  I've tried various tricks and they all fail:

- A directory in PYTHONPATH is prepended, not appended, to sys.path.

- I tried setting PYTHONSTARTUP=~/.pythonrc, and in .pythonrc I called sys.path.append().  But it turns out that .pythonrc will run only in interactive sessions, and not when Python is embedded by a program.

- My system's built-in sys.path includes an element ~/.local/lib/python3.3/site-packages but that precedes the main site-packages dir.  I can put a .pth file in this local directory, but any path I list there will be inserted into the path, again preceding the main site-packages dir.

- On my system PYTHONPATH is empty when I log in, so I can't append to it by putting something like PYTHONPATH=$PYTHONPATH:~/local/dir in .gnomerc or a similar file.

- In theory I could set PYTHONPATH=/usr/lib/python3/site-packages:~/local/dir to force the main site-packages dir to come before mine.  But then both of those would be prepended to the other predefined path elements, changing the ordering of /usr/lib/python3/site-packages relative to the other built-in path elements.  That's dangerous (and in fact led to breakage when I tried it on my system).

In addition, it's slightly inconvenient to have to create two separate __init.py__ files in my local overrides directory with the pkgutil magic listed above.  Not a huge deal, but it would be really nice if I could simply configure packages with --prefix=~/local, build and have them Just Work if I set an environment variable correctly.

Anyway, I'm far from being a Python guru, so maybe there's something I've missed here.  Have others been able to get locally installed overrides working with the fix committed here?