Make the build work with speech-dispatcher >= 0.8.

Make the build work with speech-dispatcher >= 0.8.

speech-dispatcher 0.8 has changed the location where the libspeechd.h header
file is installed, and users are expected to reference it from the top-level
directory, ie.

  #include <speech-dispatcher/libspeechd.h>

Version 0.8 also installs a .pc pkg-config file.

Since speech-dispatcher 0.7 is still very widespread, we cannot assume this new
structure and .pc file are always available, so resort to adding the
"speech-dispatcher/" prefix to the #include based on whether `pkg-config
--exists' does not fail.

Arguably, we should also start calling `pkg-config --{cflags,libs-only-l,etc}',
but not having the .pc file always available would make the changes more
complicated. Since everything has worked so far without that, keeping the
status quo for now should be fine.

BUG=
TEST=Build with speech-dispatcher >= 0.8 installed.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195204

[Dirk Pranke, 2013-04-16 19:04:37]

I've reuploaded this as https://codereview.chromium.org/14018024 so that I could
get try jobs going. I'm not sure why they're not working on this issue.

[Paweł Hajdan Jr., 2013-04-16 19:23:42]

https://codereview.chromium.org/14296005/diff/2001/build/linux/system.gyp
File build/linux/system.gyp (right):

https://codereview.chromium.org/14296005/diff/2001/build/linux/system.gyp#new...
build/linux/system.gyp:491: 'header_prefix': '<!(<(pkg-config) --exists
speech-dispatcher && echo "speech-dispatcher/" || echo)',
I'm not really happy about this. Auto-detection for compile-time things is sort
of fine (don't confuse with automagic dependencies which are bad,
http://www.gentoo.org/proj/en/qa/automagic.xml), but it may encourage actual
automagic dependencies in Chromium (I've seen that happen, and the pkg-config
logic was looking very similar).

Please rather use --bundled-header option for generate_library_loader.py . You
can find usage examples in this file (system.gyp).

[Raphael Kubo da Costa (rakuco), 2013-04-16 20:35:53]

On 2013/04/16 19:23:42, Paweł Hajdan Jr. wrote:
> but it may encourage actual
> automagic dependencies in Chromium (I've seen that happen, and the pkg-config
> logic was looking very similar).

Could you expand a bit on this part? I'm still familiarizing myself with the gyp
idioms, and I'm wondering what kind of calls may end up creating automagic
dependencies.

> Please rather use --bundled-header option for generate_library_loader.py . You
> can find usage examples in this file (system.gyp).

I thought of doing something similar before, but thought it would be too
complicated a solution for this specific issue; do you mean I should add
libspeechd.h to third_party/speech-dispatcher?

[Paweł Hajdan Jr., 2013-04-16 20:52:17]

On 2013/04/16 20:35:53, Raphael Kubo da Costa (rakuco) wrote:
> On 2013/04/16 19:23:42, Paweł Hajdan Jr. wrote:
> > but it may encourage actual
> > automagic dependencies in Chromium (I've seen that happen, and the
pkg-config
> > logic was looking very similar).
> 
> Could you expand a bit on this part? I'm still familiarizing myself with the
gyp
> idioms, and I'm wondering what kind of calls may end up creating automagic
> dependencies.

Have you read http://www.gentoo.org/proj/en/qa/automagic.xml ? It provides a
good explanation what automagic dependencies are and why they're bad. There is
nothing gyp-specific about that, once you understand the underlying issue. Of
couse if you have more questions after reading that doc, sure, I can answer
them.

Generally when you're running a command at configure time and enable/disable
dependencies based on the result, it's called an automagic dependency (as
opposed to the user explicitly specifying what dependencies should be used).

Again, this is not the case of an automagic dependency, but it is very similar
in style and so I'd prefer to avoid it.

> > Please rather use --bundled-header option for generate_library_loader.py .
You
> > can find usage examples in this file (system.gyp).
> 
> I thought of doing something similar before, but thought it would be too
> complicated a solution for this specific issue; do you mean I should add
> libspeechd.h to third_party/speech-dispatcher?

Yes.

[Raphael Kubo da Costa (rakuco), 2013-04-16 21:00:50]

This isn't strictly related to this CL, but here we go.

On 2013/04/16 20:52:17, Paweł Hajdan Jr. wrote:
> Have you read http://www.gentoo.org/proj/en/qa/automagic.xml ? It provides a
> good explanation what automagic dependencies are and why they're bad. There is
> nothing gyp-specific about that, once you understand the underlying issue. Of
> couse if you have more questions after reading that doc, sure, I can answer
> them.

Yes, I have and I know what automagic dependencies are. What got me wondering is
that I didn't really see occurrences of that in the gyp files I've been reading
-- in the chromium tree the dependencies seem to be either required or have the
possibility of being disabled with a gyp definition. I was looking for an
example of an automagic dependency popping up in the tree so I know what to
avoid or look for when reviewing stuff.

[Paweł Hajdan Jr., 2013-04-16 21:14:02]

On 2013/04/16 21:00:50, Raphael Kubo da Costa (rakuco) wrote:
> Yes, I have and I know what automagic dependencies are. What got me wondering
is
> that I didn't really see occurrences of that in the gyp files I've been
reading
> -- in the chromium tree the dependencies seem to be either required or have
the
> possibility of being disabled with a gyp definition. I was looking for an
> example of an automagic dependency popping up in the tree so I know what to
> avoid or look for when reviewing stuff.

We're pretty good at keeping them _out_ of the tree. :) I think your findings
above are correct.

[Raphael Kubo da Costa (rakuco), 2013-04-16 22:01:52]

On 2013/04/16 19:23:42, Paweł Hajdan Jr. wrote:
> Please rather use --bundled-header option for generate_library_loader.py . You
> can find usage examples in this file (system.gyp).

Oh, one thing that has also occurred to me: how is this supposed to handle the
problem of the header being in different locations depending on the version? It
will work fine if I add libspeechd.h to third_party, but only if
linux_link_libspeechd is not set; if the user chooses not to use our bundled
header, the issue will persist.

[Paweł Hajdan Jr., 2013-04-16 22:28:16]

On 2013/04/16 22:01:52, Raphael Kubo da Costa (rakuco) wrote:
> Oh, one thing that has also occurred to me: how is this supposed to handle the
> problem of the header being in different locations depending on the version?
It
> will work fine if I add libspeechd.h to third_party, but only if
> linux_link_libspeechd is not set; if the user chooses not to use our bundled
> header, the issue will persist.

If you want to address this case (pretty cool!), please just modify your patch
to:

1. Rename header_prefix to libspeechd_prefix (with a default value of '', like
'libspeechd_prefix%': '')

2. Then it'll be possible to compile with 0.8 just by passing
-Dlibspeechd_prefix=speech-dispatcher/ to gyp.

[Raphael Kubo da Costa (rakuco), 2013-04-17 09:26:16]

Alright, new patch version up.

I'm now shipping a copy of libspeechd.h from speech-dispatcher 0.7.1 (so I've
added a few OWNERS for third_party/) and also allowing users to specify the
"libspeechd_h_prefix" variable if they prefer to.

Not sure if README.chromium looks OK, or if there's a problem with shipping a
GPLv2 header.

[Paweł Hajdan Jr., 2013-04-17 17:30:48]

+dannyb for third_party change

Please make sure tools/checklicenses/checklicenses.py passes with this change.

https://codereview.chromium.org/14296005/diff/13001/build/linux/system.gyp
File build/linux/system.gyp (right):

https://codereview.chromium.org/14296005/diff/13001/build/linux/system.gyp#ne...
build/linux/system.gyp:493: # TODO: Once we do not need to support
speech-dispatcher < 0.8 we
TODO(phajdan.jr)

[Raphael Kubo da Costa (rakuco), 2013-04-17 18:15:42]

On 2013/04/17 17:30:48, Paweł Hajdan Jr. wrote:
> +dannyb for third_party change
> 
> Please make sure tools/checklicenses/checklicenses.py passes with this change.

I had to add an exception to the list since that header is GPLv2+ (not sure how
OK this is, but including the system header should be identical in terms of
license compliance I guess).

> TODO(phajdan.jr)

Done.

[cpu_(ooo_6.6-7.5), 2013-04-17 21:19:26]

lgtm once Daniel oks license.

[Daniel Berlin, 2013-04-17 21:23:00]

I'm okay with this, since nothing here is going to make it into a compiled
binary anyway :)

[Paweł Hajdan Jr., 2013-04-17 21:42:53]

https://codereview.chromium.org/14296005/diff/20001/build/linux/system.gyp
File build/linux/system.gyp (right):

https://codereview.chromium.org/14296005/diff/20001/build/linux/system.gyp#ne...
build/linux/system.gyp:513: '--bundled-header',
'"third_party/speech-dispatcher/libspeechd.h"',
nit: Please move the second argument to the next line to fit within 80 cols.

https://codereview.chromium.org/14296005/diff/20001/tools/checklicenses/check...
File tools/checklicenses/checklicenses.py (right):

https://codereview.chromium.org/14296005/diff/20001/tools/checklicenses/check...
tools/checklicenses/checklicenses.py:364: 'third_party/speech-dispatcher': [
List just the header here. We can't make a broad exception for everything.

[Raphael Kubo da Costa (rakuco), 2013-04-18 07:25:15]

All done, and the CQ is finally back.

[commit-bot: I haz the power, 2013-04-18 07:25:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raphael.kubo.da.costa@intel.com/142960...

[commit-bot: I haz the power, 2013-04-18 07:25:55]

Presubmit check for 14296005-26001 failed and returned exit status 1.

INFO:root:Found 6 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/third_party/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/tools/PRESUBMIT.py

** Presubmit Messages **
Your #include order seems to be broken. Send mail to
marja@chromium.org if this is not the case.
      third_party/speech-dispatcher/libspeechd.h:29 \
      third_party/speech-dispatcher/libspeechd.h:30

If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    tools/checklicenses/checklicenses.py

Presubmit checks took 7.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Raphael Kubo da Costa (rakuco), 2013-04-18 10:39:17]

+thestig for the checklicenses LGTM in case he's up before phajdan.jr

[Paweł Hajdan Jr., 2013-04-18 17:38:44]

LGTM, thanks for the patch!

[commit-bot: I haz the power, 2013-04-18 17:40:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raphael.kubo.da.costa@intel.com/142960...

[commit-bot: I haz the power, 2013-04-18 18:00:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-18 18:36:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raphael.kubo.da.costa@intel.com/142960...

[commit-bot: I haz the power, 2013-04-19 00:15:03]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-19 07:48:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raphael.kubo.da.costa@intel.com/142960...

[commit-bot: I haz the power, 2013-04-19 15:16:07]

Change committed as 195204

[sL1pKn07, 2013-04-22 23:49:51]

fail build with use system libspeechd

[sL1pKn07, 2013-04-23 00:02:30]

On 2013/04/22 23:49:51, sL1pKn07 wrote:
> fail build with use system libspeechd

with:

linux_link_libspeechd=1 \

  g++ '-D_FILE_OFFSET_BITS=64' '-DUSE_LINUX_BREAKPAD' '-DNO_TCMALLOC'
'-DCHROMIUM_BUILD' '-DUSE_DEFAULT_RENDER_THEME=1' '-DUSE_LIBJPEG_TURBO=1'
'-DUSE_NSS=1' '-DENABLE_ONE_CLICK_SIGNIN' '-DGTK_DISABLE_SINGLE_INCLUDES=1'
'-DENABLE_REMOTING=1' '-DENABLE_WEBRTC=1' '-DUSE_PROPRIETARY_CODECS'
'-DENABLE_CONFIGURATION_POLICY' '-DENABLE_INPUT_SPEECH' '-DENABLE_NOTIFICATIONS'
'-DENABLE_GPU=1' '-DENABLE_EGLIMAGE=1' '-DENABLE_TASK_MANAGER=1'
'-DENABLE_EXTENSIONS=1' '-DENABLE_PLUGIN_INSTALLATION=1' '-DENABLE_PLUGINS=1'
'-DENABLE_SESSION_SERVICE=1' '-DENABLE_THEMES=1' '-DENABLE_BACKGROUND=1'
'-DENABLE_AUTOMATION=1' '-DENABLE_GOOGLE_NOW=1' '-DENABLE_LANGUAGE_DETECTION=1'
'-DENABLE_PRINTING=1' '-DENABLE_CAPTIVE_PORTAL_DETECTION=1'
'-DENABLE_MANAGED_USERS=1' '-DNDEBUG' '-DNVALGRIND'
'-DDYNAMIC_ANNOTATIONS_ENABLED=0' -Iout/Release/obj/gen/shim_headers/nspr/target
-Iout/Release/obj/gen/shim_headers/libevent/target -I.  -fstack-protector
--param=ssp-buffer-size=4 -pthread -fno-exceptions -fno-strict-aliasing
-Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe
-Wno-unused-local-typedefs -fPIC -pthread -I/usr/include/glib-2.0
-I/usr/lib/glib-2.0/include -pthread -I/usr/include/gtk-2.0
-I/usr/lib/gtk-2.0/include -I/usr/include/pango-1.0 -I/usr/include/atk-1.0
-I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libdrm
-I/usr/include/libpng15 -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng15
-I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/pango-1.0
-I/usr/include/freetype2 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include
-Wno-format -Wno-unused-result -O2 -fno-ident -fdata-sections
-ffunction-sections -march=x86-64 -fno-rtti -fno-threadsafe-statics
-fvisibility-inlines-hidden -Wno-deprecated -MMD -MF
out/Release/.deps/out/Release/obj.target/libspeechd/geni/libspeechd_loader.o.d.raw
 -c -o out/Release/obj.target/libspeechd/geni/libspeechd_loader.o
out/Release/obj.target/libspeechd/geni/libspeechd_loader.cc
In file included from
out/Release/obj.target/libspeechd/geni/libspeechd_loader.cc:4:0:
./out/Release/obj/gen/library_loaders/libspeechd.h:7:24: error fatal:
libspeechd.h: no such file or directory
 #include <libspeechd.h>
                        ^

[Lei Zhang, 2013-04-23 00:11:49]

On 2013/04/22 23:49:51, sL1pKn07 wrote:
> fail build with use system libspeechd

Please file a bug on crbug.com and include:
1) the linux distro you are using
2) the libspeechd package you have installed

[Raphael Kubo da Costa (rakuco), 2013-04-23 09:25:11]

<sL1pKn07@gmail.com> writes:

> On 2013/04/22 23:49:51, sL1pKn07 wrote:
>> fail build with use system libspeechd
> In file included from
> out/Release/obj.target/libspeechd/geni/libspeechd_loader.cc:4:0:
> ./out/Release/obj/gen/library_loaders/libspeechd.h:7:24: error fatal:
> libspeechd.h: no such file or directory
>  #include <libspeechd.h>

Hmm, too bad the original commit message ended up being picked up.

From the error message, I assume you have speech-dispatcher 0.8
installed. If you want to use your system version, you need to set the
`libspeechd_h_prefix' variable to something like "speech-dispatcher/".

[sL1pKn07, 2013-04-23 16:25:37]

On 2013/04/23 09:25:11, Raphael Kubo da Costa (rakuco) wrote:
> <mailto:sL1pKn07@gmail.com> writes:
> 
> > On 2013/04/22 23:49:51, sL1pKn07 wrote:
> >> fail build with use system libspeechd
> > In file included from
> > out/Release/obj.target/libspeechd/geni/libspeechd_loader.cc:4:0:
> > ./out/Release/obj/gen/library_loaders/libspeechd.h:7:24: error fatal:
> > libspeechd.h: no such file or directory
> >  #include <libspeechd.h>
> 
> Hmm, too bad the original commit message ended up being picked up.
> 
> From the error message, I assume you have speech-dispatcher 0.8
> installed. If you want to use your system version, you need to set the
> `libspeechd_h_prefix' variable to something like "speech-dispatcher/".

thanks, works!

greetings