Since r220515: <http://trac.webkit.org/changeset/220515> the Debian bot (Jessie) is failing to build with this error: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/4385/steps/compile-webkit/logs/stdio On Debian Jessie there is no libgeoclue-2 or libgeoclue-2-dev packages. However there is a geoclue-2 package that provides the DBus interfaces and a pkg-config file: # dpkg -L geoclue-2.0 /. /usr /usr/lib /usr/lib/pkgconfig /usr/lib/pkgconfig/geoclue-2.0.pc /usr/lib/geoclue-2.0 /usr/lib/geoclue-2.0/geoclue /usr/lib/geoclue-2.0/demos /usr/lib/geoclue-2.0/demos/where-am-i /usr/share /usr/share/doc /usr/share/doc/geoclue-2.0 /usr/share/doc/geoclue-2.0/changelog.Debian.gz /usr/share/doc/geoclue-2.0/copyright /usr/share/polkit-1 /usr/share/polkit-1/rules.d /usr/share/polkit-1/rules.d/geoclue-2.0.rules /usr/share/dbus-1 /usr/share/dbus-1/system-services /usr/share/dbus-1/system-services/org.freedesktop.GeoClue2.service /usr/share/dbus-1/interfaces /usr/share/dbus-1/interfaces/org.freedesktop.GeoClue2.Agent.xml /usr/share/dbus-1/interfaces/org.freedesktop.GeoClue2.xml /usr/share/applications /usr/share/applications/geoclue-where-am-i.desktop /var /var/lib /var/lib/geoclue /var/lib/polkit-1 /var/lib/polkit-1/localauthority /var/lib/polkit-1/localauthority/10-vendor.d /var/lib/polkit-1/localauthority/10-vendor.d/geoclue-2.0.pkla /lib /lib/systemd /lib/systemd/system /lib/systemd/system/geoclue.service /etc /etc/geoclue /etc/geoclue/geoclue.conf /etc/dbus-1 /etc/dbus-1/system.d /etc/dbus-1/system.d/org.freedesktop.GeoClue2.conf /etc/dbus-1/system.d/org.freedesktop.GeoClue2.Agent.conf # apt-cache policy geoclue-2.0 geoclue-2.0: Installed: 2.1.10-2 Candidate: 2.1.10-2 Version table: *** 2.1.10-2 0 500 http://ftp.fr.debian.org/debian/ jessie/main amd64 Packages 100 /var/lib/dpkg/status # cat /usr/lib/pkgconfig/geoclue-2.0.pc prefix=/usr exec_prefix=${prefix} datarootdir=${prefix}/share datadir=${datarootdir} dbus_interface=${datadir}/dbus-1/interfaces/org.freedesktop.GeoClue2.xml agent_dbus_interface=${datadir}/dbus-1/interfaces/org.freedesktop.GeoClue2.Agent.xml Name: Geoclue Description: The Geoinformation Service Version: 2.1.10 This seems enough to build with ENABLE_GEOLOCATION, as I tested to build WebKit there with this patch http://sprunge.us/MPea and it built fine
(In reply to Carlos Alberto Lopez Perez from comment #0) > On Debian Jessie there is no libgeoclue-2 or libgeoclue-2-dev packages. > > However there is a geoclue-2 package that provides the DBus interfaces and a > pkg-config file: That seems like a packaging bug, since the pkg-config file should be in a -dev package. But where it is should not harm anything. > This seems enough to build with ENABLE_GEOLOCATION, as I tested to build > WebKit there with this patch http://sprunge.us/MPea and it built fine The problem is the FindGeoclue2 find module seems to be broken. If the pkg-config file is installed (I presume it is?), then the find module needs to set GEOCLUE2_FOUND=1.
I found another issue with a different find module on Tuesday. FindWebP failed to find libwebp on my test system and printed an error, but I guess it must have set WEBP_FOUND=1 or something because the build proceeded even though it was REQUIRED. I don't understand or trust these find modules. There is a pkg-config module for CMake, which works by trusting pkg-config like all the other packages on your computer do, and is way simpler. We could delete our find modules and use that instead. Konstantin has some reason for not wanting to do this.
WEBP_FOUND is properly set by the module here. However, REQUIRED doesn't result in fatal error
OTOH, having FATAL_ERROR in find_package() is not really user friendly: you need to run cmake over and over to find out all missing dependencies. FeatureSummary lists all missing dependencies at the end.
We don't seem to be using any geoclue header, right? > > This seems enough to build with ENABLE_GEOLOCATION, as I tested to build > > WebKit there with this patch http://sprunge.us/MPea and it built fine > > The problem is the FindGeoclue2 find module seems to be broken. If the > pkg-config file is installed (I presume it is?), then the find module needs > to set GEOCLUE2_FOUND=1. The issue is with the name it tries to find. It does: pkg_check_modules(GEOCLUE2 libgeoclue-2.0) It should do: pkg_check_modules(GEOCLUE2 geoclue-2.0) Because Jessie has a geoclue-2.0.pc file but not a libgeoclue-2.0.pc file. This later one comes with the package libgeoclue-2-dev that is not available on Jessie.
(In reply to Carlos Alberto Lopez Perez from comment #5) > The issue is with the name it tries to find. > > It does: pkg_check_modules(GEOCLUE2 libgeoclue-2.0) > > It should do: pkg_check_modules(GEOCLUE2 geoclue-2.0) I'm not sure. $ pkg-config --cflags --libs geoclue-2.0 $ pkg-config --cflags --libs libgeoclue-2.0 -I/usr/include/libgeoclue-2.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/gio-unix-2.0/ -pthread -lgeoclue-2 -lgio-2.0 -lgobject-2.0 -lglib-2.0 So if we do not link to libgeoclue AND we don't need any of its headers, it's OK. I guess if you say the build succeeds, then that must be the case. So we must be able to remove our use of GEOCLUE2_CFLAGS and GEOCLUE2_LIBS then.
(In reply to Konstantin Tokarev from comment #4) > OTOH, having FATAL_ERROR in find_package() is not really user friendly: you > need to run cmake over and over to find out all missing dependencies. > FeatureSummary lists all missing dependencies at the end. We should consider switching to that. Are you saying that REQUIRED by *design* does not result in a fatal error, rather than some bug? Our options parsing has been broken for this entire time? If so, maybe it's time to make use of FeatureSummary like you suggest.
(In reply to Michael Catanzaro from comment #6) > (In reply to Carlos Alberto Lopez Perez from comment #5) > > The issue is with the name it tries to find. > > > > It does: pkg_check_modules(GEOCLUE2 libgeoclue-2.0) > > > > It should do: pkg_check_modules(GEOCLUE2 geoclue-2.0) > > I'm not sure. > > $ pkg-config --cflags --libs geoclue-2.0 > > $ pkg-config --cflags --libs libgeoclue-2.0 > -I/usr/include/libgeoclue-2.0 -I/usr/include/glib-2.0 > -I/usr/lib64/glib-2.0/include -I/usr/include/gio-unix-2.0/ -pthread > -lgeoclue-2 -lgio-2.0 -lgobject-2.0 -lglib-2.0 > > So if we do not link to libgeoclue AND we don't need any of its headers, > it's OK. I guess if you say the build succeeds, then that must be the case. > > So we must be able to remove our use of GEOCLUE2_CFLAGS and GEOCLUE2_LIBS > then. I did some greps and I don't see where we are including the libgeoclue headers. Why we require it? Maybe this is a run-time dependency rather than a build one?
How is the Geoclue2Interface.h generation handled if ENABLE_GEOLOCATION is true, but no geoclue package is installed? https://trac.webkit.org/browser/trunk/Source/WebCore/PlatformGTK.cmake#L183
>Are you saying that REQUIRED by *design* does not result in a fatal error, rather than some bug? No, by design it should result in fatal error. However, I've found why it doesn't happen for WebP and GeoClue: package names are case-sensitive, so WebP != WEBP, and WEBP_FOUND being false is ignored because cmake looks for WebP_FOUND. We either need to rename FindWebP.cmake to FindWEBP.cmake and use find_package(WEBP), or use "WebP" spelling consistently. Same for other packages with mixed case names. As for FeatureSummary, I've added it to Qt port because of user request, it provides nice summary about what packages (required and optional) were found and which were not, and allows to add hints to user what is the purpose of these dependencies (so that user can know e.g. that disabling Geolocation is a way to get rid of GeoClue). See cmake documentation for more details. It can also replace our home-grown "Enabled features" list.
(In reply to Konstantin Tokarev from comment #10) > >Are you saying that REQUIRED by *design* does not result in a fatal error, rather than some bug? > > No, by design it should result in fatal error. However, I've found why it > doesn't happen for WebP and GeoClue: package names are case-sensitive, so > WebP != WEBP, and WEBP_FOUND being false is ignored because cmake looks for > WebP_FOUND. We either need to rename FindWebP.cmake to FindWEBP.cmake and > use find_package(WEBP), or use "WebP" spelling consistently. Same for other > packages with mixed case names. Wow. Just wow. Anyway, this is a separate bug report now. Bug #175427. > As for FeatureSummary, I've added it to Qt port because of user request, it > provides nice summary about what packages (required and optional) were found > and which were not, and allows to add hints to user what is the purpose of > these dependencies (so that user can know e.g. that disabling Geolocation is > a way to get rid of GeoClue). See cmake documentation for more details. It > can also replace our home-grown "Enabled features" list. OK, we should definitely consider doing that in the future.
(In reply to Zan Dobersek from comment #9) > How is the Geoclue2Interface.h generation handled if ENABLE_GEOLOCATION is > true, but no geoclue package is installed? > https://trac.webkit.org/browser/trunk/Source/WebCore/PlatformGTK.cmake#L183 That should be a fatal error, because it is supposed to be REQUIRED. There should be no way to get that far in an ENABLE_GEOLOCATION build without geoclue2 installed. Bug #175427.
(In reply to Zan Dobersek from comment #9) > How is the Geoclue2Interface.h generation handled if ENABLE_GEOLOCATION is > true, but no geoclue package is installed? > https://trac.webkit.org/browser/trunk/Source/WebCore/PlatformGTK.cmake#L183 This needs geoclue-2.0 pkg-config file not libgeoclue-2.0 one. Therefore FindGeoclue2 is looking for the wrong pkg-config file. This is on Debian Jessie: $ pkg-config --variable dbus_interface geoclue-2.0 /usr/share/dbus-1/interfaces/org.freedesktop.GeoClue2.xml It works fine
Created attachment 317807 [details] Patch Tested on Jessie. Works OK.
Created attachment 317808 [details] Patch Remove libgeoclue-2-dev from install-dependencies
Comment on attachment 317808 [details] Patch Clearing flags on attachment: 317808 Committed r220529: <http://trac.webkit.org/changeset/220529>
All reviewed patches have been landed. Closing bug.
Also need to stop using the CFLAGS/LIBS
Reopening to attach patch
Created attachment 317818 [details] Patch
Created attachment 317819 [details] Patch
Comment on attachment 317819 [details] Patch You are missing one ${GEOCLUE_INCLUDE_DIRS} at Source/WebKit/PlatformGTK.cmake
Created attachment 317820 [details] Patch
Comment on attachment 317820 [details] Patch Rejecting attachment 317820 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 317820, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 220532 = 769b5779384cb6500775be0340814fcc6aa84b24 r220533 = 5693a3b3a327bdb93e96d79539b30bb309a1bd07 r220534 = eb6e66c7224b0f60cdc20f1ac591c1fbf36f9326 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://webkit-queues.webkit.org/results/4290924
Committed r220540: <http://trac.webkit.org/changeset/220540>