Bug 175426 – REGRESSION(r220515) [GTK][CMake] Build with ENABLE_GEOLOCATION fails on Debian Jessie

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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.

[Konstantin Tokarev]

WEBP_FOUND is properly set by the module here. However, REQUIRED doesn't result in fatal error

[Konstantin Tokarev]

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.

[Carlos Alberto Lopez Perez]

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.

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Carlos Alberto Lopez Perez]

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

[Zan Dobersek]

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

[Konstantin Tokarev]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Carlos Alberto Lopez Perez]

(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

[Carlos Alberto Lopez Perez]

Created attachment 317807 [details]
Patch

Tested on Jessie. Works OK.

[Carlos Alberto Lopez Perez]

Created attachment 317808 [details]
Patch

Remove libgeoclue-2-dev from install-dependencies

[WebKit Commit Bot]

Comment on attachment 317808 [details]
Patch

Clearing flags on attachment: 317808

Committed r220529: <http://trac.webkit.org/changeset/220529>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Also need to stop using the CFLAGS/LIBS

[Michael Catanzaro]

Reopening to attach patch

[Michael Catanzaro]

Created attachment 317818 [details]
Patch

[Michael Catanzaro]

Created attachment 317819 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 317819 [details]
Patch

You are missing one ${GEOCLUE_INCLUDE_DIRS} at Source/WebKit/PlatformGTK.cmake

[Michael Catanzaro]

Created attachment 317820 [details]
Patch

[WebKit Commit Bot]

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

[Michael Catanzaro]

Committed r220540: <http://trac.webkit.org/changeset/220540>