GNOME Bugzilla – Bug 703571
Avoid doubling up on geocode-glib in include path
Last modified: 2013-07-12 20:18:10 UTC
Previously, both the `pkg-config --cflags geocode-glib-1.0` and the path in the modified (not all) include files both contained the geocode-glib sub-directory. This only worked when geocode-glib/*.h was installed in /usr/include (or similar system location). When you actually rely on what pkg-config provides, it will fail.... Example from building geoclue2 with geocode-glib installed in /opt/geoclue-glib/ : make[3]: Entering directory `/home/gem/opt/maps/geoclue2/src' gcc -DHAVE_CONFIG_H -I. -I.. -pthread -I/opt/geocode-glib/include/geocode-glib -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/gio-unix-2.0/ -DLOCALEDIR="\"/opt/geoclue2/share/locale\"" -g -O2 -w -MT geoclue-gclue-locator.o -MD -MP -MF .deps/geoclue-gclue-locator.Tpo -c -o geoclue-gclue-locator.o `test -f 'gclue-locator.c' || echo './'`gclue-locator.c In file included from gclue-locator.h:6:0, from gclue-locator.c:4: /opt/geocode-glib/include/geocode-glib/geocode-glib.h:28:43: fatal error: geocode-glib/geocode-location.h: No such file or directory #include <geocode-glib/geocode-location.h> ^ compilation terminated. make[3]: *** [geoclue-gclue-locator.o] Error 1 Use "..." style include rather then just stripping <geocode-glib/...> down to <...> to be consistent with style in geocode-glib/geocode-ipclient.h, geocode-glib/geocode-place.h and geocode-glib/geocode-glib-private.h.
Created attachment 248343 [details] [review] Avoid doubling up on geocode-glib in include path
Review of attachment 248343 [details] [review]: I'd rather do the reverse, keep the namespaced includes, and have the files installed under: /usr/include/geocode-glib-1.0/geocode-glib/geocode-glib.h /usr/include/geocode-glib-1.0 would be the in the pkg-config includes. The versioning of the includes directory allows parallel installation, in case we change the API.
Created attachment 248378 [details] [review] Make include directory API versioned Note that this will break users of geocode-glib which did not use the geocode-glib/ subdirectory prefix in the include path (like geoclue2), according to how the pkg-config file was layed out before. Remaining cleanup is to unify the internal include paths inside geocode-glib to be either "..." or <geocode-glib/...>. While forcing geocode-glib uses to update their include paths and internally cleaning up the include paths, this might also be a good time to drop the filename prefix on header files since both subdirectory and filename prefix in include paths seems redundant.
Please comment on the new patch and if ok I'll follow up with an additional patch to clean up the (internal) include paths in the header files (including or excluding renaming them, however you like it).
Review of attachment 248378 [details] [review]: Looks good.
Created attachment 248558 [details] [review] Make include directory API versioned Note that this will break users of geocode-glib which did not use the geocode-glib/ subdirectory prefix in the include path (like geoclue2), according to how the pkg-config file was layed out before. Remaining cleanup is to unify the internal include paths inside geocode-glib to be either "..." or <geocode-glib/...>. While forcing geocode-glib uses to update their include paths and internally cleaning up the include paths, this might also be a good time to drop the filename prefix on header files since both subdirectory and filename prefix in include paths seems redundant.
Created attachment 248559 [details] [review] Unify include paths to be the same everywhere Change from "geocode-*.h" and <geocode-*.h> to <geocode-glib/geocode-*.h> where it wasn't using that style already.
Created attachment 248560 [details] [review] Symlink geocode-glib.h into subdir to avoid breakage Atleast geoclue2 does #include <geocode-glib.h> which will break with the new api-versioned include path with additional subdirectory. This is a workaround to avoid causing breakage until everyone has updated their include paths.
Rebased on current master (no real changes to first patch) with two additional patches. Please tell me if you see any reason it's not ok to push those as well. (Might drop the last one if I can sync with zeenix about pushing a fix for geocode2 at the same time, but haven't been able to reach him yet.... fwiw, Empathy seems fine without it and I don't know of any other users of geocode-glib to check.)
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Can you please avoid committing patches when they haven't been reviewed?
Comment on attachment 248560 [details] [review] Symlink geocode-glib.h into subdir to avoid breakage I'd rather we fixed the (very few) users of the API. geocode-glib is unstable right now so breakage is expected.
I've reverted that last patch.
Ok... zeenix is on vacation right now, so geoclue2 probably won't be fixed for some time...
The "Symlink geocode-glib.h into subdir to avoid breakage" patch doesn't appear to have been reverted and it's breaking DESTDIR installs (eg: jhbuild). Someone should actually revert that...
zeenix should be back next week, anyway
The revert is now pushed (my dead SSD probably has that patch...). geoclue2 will be broken until somebody pushes a (trivial) fix.