GNOME Bugzilla – Bug 765735
GI_TYPELIB_PATH may override g_irepository_prepend_search_path
Last modified: 2016-05-02 14:37:15 UTC
In Mutter we've recently folded in our own internal copies of Clutter/Cogl, with future refactors in mind. This has brought in a situation in gnome-shell with .typelib finding though, even though gnome-shell uses g_irepository_prepend_search_path() to add the path to our internal copies' typelib files, it is easily overriden through the GI_TYPELIB_PATH environment variable. This gets most noticeable running inside jhbuild, GI_TYPELIB_PATH is set to point to jhbuild/system paths, those get favored over application mandated paths, so the lookups for Clutter/Cogl typelibs happen in system/jhbuild dirs, rather than our internal copy. This leads to the expected warnings (and eventual crash) when trying to create the fundamental clutter classes again. At #gnome-shell we found this to be made deliberately in https://bugzilla.gnome.org/show_bug.cgi?id=562914 , however our consensus was that it's more sensible the other way around, the application should have the last say precisely for cases like this. The current approach may sound convenient for noinstall executables, tests and whatnot, although IMO if a g_irepository_prepend_search_path() in the application is preventing its tests to lookup the right folder, the application itself should be setting its own environment variables for this purpose, instead of relying on a global switch. I did a patch for this, but it's basically a revert of commit e81c4681cc88a00fcd841c5a68d860d3714b55d7
That original patch had a rationale/use case, so if you're arguing for a revert, you should be describing how you're not going to break it. Concretely, wouldn't inverting this break jhbuild use cases? Another messy case is `make check`. Which can actually be recursive, a component may want to work running `make check` inside jhbuild. I'm not sure how to get out of this without adding new API to g-i. I guess a simple alternative would be for g-s to g_setenv (GI_TYPELIB_PATH) early on in main.c...although nevermind that would break apps executed directly. Hard to see a way out of this without adding some sort of new API to g-i.
(In reply to Colin Walters from comment #1) > That original patch had a rationale/use case, so if you're arguing for a > revert, you should be describing how you're not going to break it. Sure, what was it? :). Didn't see any other related bug reports that might be fixed by this patch, and the original bug/patch mention no specific use cases. > > Concretely, wouldn't inverting this break jhbuild use cases? Don't think so, I'd expect apps using g_irepository_prepend_search_path to provide a path inside their --prefix, hence jhbuild's. AFAICS, this ordering detail is mainly useful when you want to run stuff completely in-tree, so you can override your app lookup dirs without touching too much of the app itself. But again, this is just a problem if your app also calls g_irepository_prepend_search_path(), and seems easily doable in apps themselves, eg. https://git.gnome.org/browse/gnome-music/tree/gnome-music.in#n22. > > Another messy case is `make check`. Which can actually be recursive, a > component may want to work running `make check` inside jhbuild. > > I'm not sure how to get out of this without adding new API to g-i. I agree that we either make a potential break, or we add new API. I do wonder how many things will legitimately break though, and whether it makes sense to fix those in place. I see the API addition way quite unfortunate too, prepend_search_path() is taken, and the current function is a huge misnomer as it currently behaves :(. > > I guess a simple alternative would be for g-s to g_setenv (GI_TYPELIB_PATH) > early on in main.c...although nevermind that would break apps executed > directly. Yeah, definitely not an option here... > > Hard to see a way out of this without adding some sort of new API to g-i.
(In reply to Carlos Garnacho from comment #2) > AFAICS, this ordering detail is mainly useful when you want to run stuff > completely in-tree, so you can override your app lookup dirs without > touching too much of the app itself. But again, this is just a problem if > your app also calls g_irepository_prepend_search_path(), and seems easily > doable in apps themselves, eg. > https://git.gnome.org/browse/gnome-music/tree/gnome-music.in#n22. Hmm...if we change this so that prepend_search_path() overrides the environment variable, it seems like for applications that want to run uninstalled (also the case for `make check`), they'll have to change their code to set an environment variable for the case when they're uninstalled, and if set, *don't* call g_irepository_prepend_search_path(). Right?
(In reply to Colin Walters from comment #1) > Concretely, wouldn't inverting this break jhbuild use cases? Anything build inside jhbuild has the same prefix of gobject-introspection, and the scanner adds the gobject-introspection libdir into the search paths. The only possible breaking scenario would be running the scanner coming from a system-installed gobject-introspection into a jhbuild root — something I consider very much of an edge case. > I guess a simple alternative would be for g-s to g_setenv (GI_TYPELIB_PATH) > early on in main.c...although nevermind that would break apps executed > directly. There are also the threading considerations that apply to setenv(). > Hard to see a way out of this without adding some sort of new API to g-i. "really_prepend_search_path_sorry_about_that()"? This does not really fix the `make check` or the `run uninstalled` use cases for applications that use the API. We could work around the `make check` case with an ad hoc envvar injected into the TEST_ENVIRONMENT. The "run app uninstalled" case is so deeply uninteresting I'm not sure we want to consider it at all; we *already* have a ton of ancillary built files that do not really work uninstalled, like GSettings schemas.
(In reply to Emmanuele Bassi (:ebassi) from comment #4) > (In reply to Colin Walters from comment #1) > > > Concretely, wouldn't inverting this break jhbuild use cases? > > Anything build inside jhbuild has the same prefix of gobject-introspection, > and the scanner adds the gobject-introspection libdir into the search paths. > > The only possible breaking scenario would be running the scanner coming from > a system-installed gobject-introspection into a jhbuild root — something I > consider very much of an edge case. It's an edge case, but we wouldn't even be breaking it if we allowed GI_TYPELIB_PATH to override the default libdir location: prepended + GI_TYPELIB_PATH + default
(In reply to Colin Walters from comment #3) > Hmm...if we change this so that prepend_search_path() overrides the > environment variable, it seems like for applications that want to run > uninstalled (also the case for `make check`), they'll have to change their > code to set an environment variable for the case when they're uninstalled, > and if set, *don't* call g_irepository_prepend_search_path(). > > Right? I was rather thinking in having those add their own ad-hoc envvar/checks so they're able to call that function with the in-tree prefix wherever necessary. Most surely this isn't the first implementation detail the testsuite gets to know about the code it's testing :). (In reply to Emmanuele Bassi (:ebassi) from comment #4) > (In reply to Colin Walters from comment #1) > > > Concretely, wouldn't inverting this break jhbuild use cases? > > Anything build inside jhbuild has the same prefix of gobject-introspection, > and the scanner adds the gobject-introspection libdir into the search paths. > > The only possible breaking scenario would be running the scanner coming from > a system-installed gobject-introspection into a jhbuild root — something I > consider very much of an edge case. Not even that, GI_TYPELIB_PATH still would take precedence over the g-i libdir, so the lookup order in that case would be: - App provided search path (assuming it was built on jhbuild prefix: /$jhuild/lib/app/priv/) - GI_TYPELIB_PATH from jhbuild (/$jhbuild/lib/.... , /usr/lib/...) - g-i libdir (/usr/lib/...) So lookups of common typelib files will still happen in jhbuild before resorting to the system libdir.
(In reply to Florian Müllner from comment #5) > (In reply to Emmanuele Bassi (:ebassi) from comment #4) > > (In reply to Colin Walters from comment #1) > > > > > Concretely, wouldn't inverting this break jhbuild use cases? > > > > Anything build inside jhbuild has the same prefix of gobject-introspection, > > and the scanner adds the gobject-introspection libdir into the search paths. > > > > The only possible breaking scenario would be running the scanner coming from > > a system-installed gobject-introspection into a jhbuild root — something I > > consider very much of an edge case. > > It's an edge case, but we wouldn't even be breaking it if we allowed > GI_TYPELIB_PATH to override the default libdir location: > > prepended + GI_TYPELIB_PATH + default This is AFAICS what happens if reverting the patch in question, before any element is prepended through code, the search path list is initialized with the latter two in precisely that order.
(In reply to Emmanuele Bassi (:ebassi) from comment #4) > The "run app uninstalled" case is so deeply > uninteresting I'm not sure we want to consider it at all; we *already* have > a ton of ancillary built files that do not really work uninstalled, like > GSettings schemas. It is a maintenance pain to have work, but people have been doing it forever. Before GSettings, GConf was a pain point here too. It can shave off potentially a few seconds from the build -> run cycle, and that's potentially worthwhile for people. I'm a bit more concerned too about `make check`, but the set of programs that call prepend_search_path() (i.e. have a private typelib) is relatively small.
Anyways, Carlos, can you post the patch?
One option would be to split GI_TYPELIB_PATH into two variables: one module-specific that scripts can use for tests/uninstalled executables, and one for tools like jhbuild that change the generic environment. (Although of course: "You have a problem with an env variable. You add an additional env variable. Now you have two problems.")
Created attachment 327057 [details] [review] girepository: Merge overrides with the regular search path This reverts commit e81c4681cc88a00fcd841c5a68d860d3714b55d7 The GI_TYPELIB_PATH envvar will still allow overriding the default typelib dir (based on gobject-introspection libdir), but applications will have the last say about typelib lookup directories. The resulting lookup order is now: - Paths added through g_irepository_prepend_search_path() - Paths in GI_TYPELIB_PATH - The default gobject introspection lookup dir This makes g_irespository_prepend_search_path() work as announced despite environment variables. If any application was relying on GI_TYPELIB_PATH overriding the paths of this function call (for e.g. make check, or to be able to run code inside the project tree), it is encouraged to set up a similar envvar for their application specific lookup dir, or perform this override through other means.
Review of attachment 327057 [details] [review]: It seems likely in fact this will break gjs, which does use prepend_search_path for `GjsPrivate`, and has `make check`. Right? Although the gjs tests are broken for me at the moment anyways without this patch, sigh. Anyways...I'm okay with applying this patch and trying to fix up dependent modules. (It looks like we can stop copying the search path list now too?)
(In reply to Colin Walters from comment #12) > Review of attachment 327057 [details] [review] [review]: > > It seems likely in fact this will break gjs, which does use > prepend_search_path for `GjsPrivate`, and has `make check`. > > Right? Right. AFAICS there's already some handling of uninstalled tests at: https://git.gnome.org/browse/gjs/tree/installed-tests/gjs-unit.cpp#n163 It seems the prepend_search_path() in the uninstalled tests case was removed early in gjs development in bug #557579. Since then, tests rely on the envvar being set at: https://git.gnome.org/browse/gjs/tree/Makefile-test.am#n111 However, it does not seem to suffice adding back an append_search_path() call here, the search path manipulation in gjs-unit.cpp seems to be done prior to the default GjsPrivate path initialization in gjs_context_class_init(), so this last one would prevail. I guess either initializing the GjsContext class early or adding the extra envvar would work here. > > Although the gjs tests are broken for me at the moment anyways without this > patch, sigh. > > Anyways...I'm okay with applying this patch and trying to fix up dependent > modules. Thanks :), much appreciated. I do hope the fallout is mostly restricted to gjs. AFAICS the usecase I see most often in other projects is being able to introspect git submodules (eg. libgd), so it's likely they won't bother testing those. > > (It looks like we can stop copying the search path list now too?) Indeed, I've also removed the build_search_path_with_overrides() function and ensured init_globals() is called in place of the previous callers.
Attachment 327057 [details] pushed as bd475c0 - girepository: Merge overrides with the regular search path
Filed bug #765905 for the gjs bits.