Allow building with system-provided sqlite. Last change (tm).

Allow building with system-provided sqlite. Last change (tm).

This is upstreaming a Gentoo Linux patch.

This patch prevents having multiple conflicting copies of sqlite3Preload at link time, and also fixes a dependency problem that could result in a header mismatch. The gyp file change ensures that we propagate the USE_SYSTEM_SQLITE preprocessor symbol to all targets that need it.

BUG=22208
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=61489

[Paweł Hajdan Jr., 2010-10-01 13:21:37]

The stubs.cc is needed to ensure we only have one copy of sqlite3Preload. What
we were getting before was multiple copies of sqlite3Preload, resulting in a
link error.

The gyp change is needed to get USE_SYSTEM_SQLITE preprocessor define in all
targets that do in fact depend on it. Otherwise the compile breaks with
-Duse_system_sqlite if the contents of the preprocessed directory are removed.

Without the above change we would get a header mismatch: some parts of chrome
would try to use the bundled header even when building with system sqlite.

[Evan Martin, 2010-10-01 16:46:50]

On 2010/10/01 13:21:37, Paweł Hajdan Jr. wrote:
> The stubs.cc is needed to ensure we only have one copy of sqlite3Preload. What
> we were getting before was multiple copies of sqlite3Preload, resulting in a
> link error.

Can you put this info in the commit message?

LGTM to save another review round-trip if I understood you, please wait for the
round-trip if I didn't understand the change :)

[Evan Martin, 2010-10-01 16:47:10]

(not sure why previous message didn't include review comemts)

http://codereview.chromium.org/3519002/diff/1/2
File chrome/chrome.gyp (right):

http://codereview.chromium.org/3519002/diff/1/2#newcode1021
chrome/chrome.gyp:1021: 'common',
Is this intentionally part of your change?

http://codereview.chromium.org/3519002/diff/1/5
File third_party/sqlite/stubs.cc (right):

http://codereview.chromium.org/3519002/diff/1/5#newcode3
third_party/sqlite/stubs.cc:3: int sqlite3Preload(sqlite3 *db) {
If I follow you correctly, what you're saying is we explicitly want to have this
with non-static linkage so that it conflicts with the other sqlite3Preload. 
Otherwise, I'd say we should just add a "static" to the function declaration in
the header file.

If my understanding is correct, can you add a comment above this saying
something about this?
Also, this file needs a license header.

[Scott Hess - ex-Googler, 2010-10-01 17:08:30]

http://codereview.chromium.org/3519002/diff/1/5
File third_party/sqlite/stubs.cc (right):

http://codereview.chromium.org/3519002/diff/1/5#newcode3
third_party/sqlite/stubs.cc:3: int sqlite3Preload(sqlite3 *db) {
On 2010/10/01 16:47:10, Evan Martin wrote:
> If I follow you correctly, what you're saying is we explicitly want to have
this
> with non-static linkage so that it conflicts with the other sqlite3Preload. 
> Otherwise, I'd say we should just add a "static" to the function declaration
in
> the header file.

Hmm.  I didn't read it that way, I read it as the various call sites each
defining the function and conflicting with themselves.  In which case your
"static" solution would resolve that.

I think a stub file is a reasonable solution if it prevents us from somehow
including both USE_SYSTEM_SQLITE and !USE_SYSTEM_SQLITE libraries.  Being robust
in the face of people making changes with subtle effects is always good.  But I
don't think we can accomplish that, since these things are in different
libraries, so the first one past the gate will be chosen.

Why don't we #if !defined(USE_SYSTEM_SQLITE) at the call sites, instead?  Then
if the define is not used consistently, there will be a link error on the final
binary.

[Paweł Hajdan Jr., 2010-10-04 14:07:37]

I've tested it on my Gentoo VM, and just adding "static" works great. PTAL.

http://codereview.chromium.org/3519002/diff/1/2
File chrome/chrome.gyp (right):

http://codereview.chromium.org/3519002/diff/1/2#newcode1021
chrome/chrome.gyp:1021: 'common',
On 2010/10/01 16:47:10, Evan Martin wrote:
> Is this intentionally part of your change?

Yes, and it's needed to prevent a compile failure. See the comment "The gyp
change is needed ..." above.

[Evan Martin, 2010-10-04 16:11:06]

LGTM, with the description of what you're doing (the first message in this
thread) included in the commit message

[Paweł Hajdan Jr., 2010-10-04 16:29:57]

Done.

On Mon, Oct 4, 2010 at 18:11, <evan@chromium.org> wrote:

> LGTM, with the description of what you're doing (the first message in this
> thread) included in the commit message
>
>
> http://codereview.chromium.org/3519002/show
>

[Evan Martin, 2010-10-04 17:05:15]

LGTM++

[Scott Hess - ex-Googler, 2010-10-04 17:09:54]

lgtm.