Avoid zlib symbol conflicts in open-vcdiff

Avoid zlib symbol conflicts in open-vcdiff

Roll open-vcdiff from 40 to 42 to include the changes about zlib files (http://code.google.com/p/open-vcdiff/source/detail?r=41)

Exclude zlib files when building open-vcdiff for Chromium to avoid link warning due to name conflict with zlib.

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

[tonyg, 2012-03-01 11:32:53]

Just one question, but I'm probably not a great reviewer for sdch. Can you
annotate the code and add a reviewer who is more familiar with sdch?

https://chromiumcodereview.appspot.com/9555002/diff/1002/sdch/sdch.gyp
File sdch/sdch.gyp (right):

https://chromiumcodereview.appspot.com/9555002/diff/1002/sdch/sdch.gyp#newcode53
sdch/sdch.gyp:53: 'Z_PREFIX',
Sorry for the naive question, but why is there a local zlib in the first place.
If it is unmodified can we remove it? If it is heavily modified, should it just
be renamed so as to not conflict in the first place?

[Xianzhu, 2012-03-01 17:44:44]

https://chromiumcodereview.appspot.com/9555002/diff/1002/sdch/sdch.gyp
File sdch/sdch.gyp (right):

https://chromiumcodereview.appspot.com/9555002/diff/1002/sdch/sdch.gyp#newcode53
sdch/sdch.gyp:53: 'Z_PREFIX',
On 2012/03/01 11:32:54, tonyg wrote:
> Sorry for the naive question, but why is there a local zlib in the first
place.
> If it is unmodified can we remove it? If it is heavily modified, should it
just
> be renamed so as to not conflict in the first place?

Good question. I just compared sdch/open-vcdiff/src/adler32.c and
third_party/zlib/adler32.c and I believe the differences won't have any effect.
I'll try to let sdch depend on zlib.

[Mark Mentovai, 2012-03-01 19:46:34]

https://chromiumcodereview.appspot.com/9555002/diff/5001/sdch/sdch.gyp
File sdch/sdch.gyp (right):

https://chromiumcodereview.appspot.com/9555002/diff/5001/sdch/sdch.gyp#newcode18
sdch/sdch.gyp:18: # We use third_party/zlib instead.
1. Don’t use “we” in comments.

2. I’d group the three commented-out files together, way at the bottom of the
list.

https://chromiumcodereview.appspot.com/9555002/diff/5001/sdch/sdch.gyp#newcode22
sdch/sdch.gyp:22: 'open-vcdiff/src/checksum.h',
This file says

#include "zlib.h"

How are you sure that it will include the zlib.h in third_party/zlib as opposed
to the one in sdch/open-vcdiff/src? I don’t think it’ll include the right one.

[Xianzhu, 2012-03-01 21:28:55]

https://chromiumcodereview.appspot.com/9555002/diff/5001/sdch/sdch.gyp
File sdch/sdch.gyp (right):

https://chromiumcodereview.appspot.com/9555002/diff/5001/sdch/sdch.gyp#newcode18
sdch/sdch.gyp:18: # We use third_party/zlib instead.
On 2012/03/01 19:46:34, Mark Mentovai wrote:
> 1. Don’t use “we” in comments.
>
Done.

> 2. I’d group the three commented-out files together, way at the bottom of the
> list.

https://chromiumcodereview.appspot.com/9555002/diff/5001/sdch/sdch.gyp#newcode22
sdch/sdch.gyp:22: 'open-vcdiff/src/checksum.h',
On 2012/03/01 19:46:34, Mark Mentovai wrote:
> This file says
> 
> #include "zlib.h"
> 
> How are you sure that it will include the zlib.h in third_party/zlib as
opposed
> to the one in sdch/open-vcdiff/src? I don’t think it’ll include the right one.

Yes, it will include the one in sdch/open-vcdiff/src. As adler32 is the only
function needed from zlib and it's unlikely that future zlib would break binary
compatibility, do you think we can just leave the headers here? I'm trying not
to change open-vcdiff source code.

[Mark Mentovai, 2012-03-01 21:33:41]

Please be sure you’re #including the copy of the header that goes along with the
object code you’re actually linking with.

I know you say “I only need one declaration, and it’s the same!” That may be
true now, but it may not be true in perpetuity. Maybe someone will make local
changes to third_party/zlib in the future affecting symbol names or other
attributes. Not using the right header will cause a build break in sdch. That’s
very sloppy.

[Xianzhu, 2012-03-01 21:38:06]

Then I'd prefer the method used in Patch Set 2, otherwise it's seems
unavoidable to change sdch code to let it include the correct header file.
Does Patch Set 2 look good to you?

On Thu, Mar 1, 2012 at 1:33 PM, <mark@chromium.org> wrote:

> Please be sure you’re #including the copy of the header that goes along
> with the
> object code you’re actually linking with.
>
> I know you say “I only need one declaration, and it’s the same!” That may
> be
> true now, but it may not be true in perpetuity. Maybe someone will make
> local
> changes to third_party/zlib in the future affecting symbol names or other
> attributes. Not using the right header will cause a build break in sdch.
> That’s
> very sloppy.
>
>
https://chromiumcodereview.**appspot.com/9555002/<https://chromiumcodereview....
>

[Mark Mentovai, 2012-03-01 21:57:45]

Then we’re back to unnecessarily building two copies of adler32. That’s bad too.

I’m afraid my recommendation is to modify open-vcdiff to allow it to be built
with an external zlib.

[Xianzhu, 2012-03-01 22:02:18]

Should the change be upstreamed to sdch, or do we allow local changes?

On Thu, Mar 1, 2012 at 1:57 PM, <mark@chromium.org> wrote:

> Then we’re back to unnecessarily building two copies of adler32. That’s
> bad too.
>
> I’m afraid my recommendation is to modify open-vcdiff to allow it to be
> built
> with an external zlib.
>
>
https://chromiumcodereview.**appspot.com/9555002/<https://chromiumcodereview....
>

[Mark Mentovai, 2012-03-01 22:06:04]

We pull sdch/open-vcdiff directly from the upstream. It’s always preferable to
make the changes upstream if they’re amenable.

[wtc, 2012-03-01 22:38:29]

Review comments on Patch Set 4:

https://chromiumcodereview.appspot.com/9555002/diff/8001/sdch/sdch.gyp
File sdch/sdch.gyp (right):

https://chromiumcodereview.appspot.com/9555002/diff/8001/sdch/sdch.gyp#newcode52
sdch/sdch.gyp:52: 'open-vcdiff/src',

mark: this include_dirs setting should ensure that the
local zconf.h and zlib.h are used.

This also means this include_dirs setting is harmful if
we want to use system zlib.

Also, the include_dirs for direct_dependent_settings
causes other projects to pick up the zlib headers from
this header.  I doubt that's what we want.

So I think the safest solution is to svn remove adler32.c
zconf.h zlib.h, and document this local modification in
a README.chromium file.

[Mark Mentovai, 2012-03-01 22:41:09]

We can’t just svn remove outright because we pull directly from the upstream via
DEPS. We’d need to switch to a local mirror with our own local modifications.

[Xianzhu, 2012-03-01 23:17:04]

On 2012/03/01 22:41:09, Mark Mentovai wrote:
> We can’t just svn remove outright because we pull directly from the upstream
via
> DEPS. We’d need to switch to a local mirror with our own local modifications.

How about excluding the three files in DEPS, like (my 'gclient sync' always
stuck this afternoon, so haven't verified if it works):
  "src/sdch/open-vcdiff/src/adler32.c": None,
  "src/sdch/open-vcdiff/src/zlib.h": None,
  "src/sdch/open-vcdiff/src/zconf.h": None,

[Mark Mentovai, 2012-03-01 23:21:26]

That won’t work. You can use None to exclude a repository listed in
DEPS from being checked out, but not to exclude files from a
repository that’s present in DEPS and being checked out.

[Xianzhu, 2012-03-29 17:02:17]

Hi, openvcdiff,

Could you have a look at
http://code.google.com/p/open-vcdiff/issues/detail?id=34? It blocks upstreaming
this change for Android.

Thanks,
Xianzhu

[Lincoln, 2012-04-03 21:39:56]

On 2012/03/29 17:02:17, Xianzhu wrote:
> Could you have a look at
> http://code.google.com/p/open-vcdiff/issues/detail?id=34? It blocks
upstreaming
> this change for Android.

Version 0.8.3 (r42) of open-vcdiff has been checked in and includes your patch. 
Thanks much.

[Xianzhu, 2012-04-03 22:42:49]

Hi, reviewers,

Could you review the last change set? It rolls in the latest open-vcdiff changes
so that we can safely exclude the zlib files in it and depend on
third_party/zlib.

Thanks,
Xianzhu

[Lincoln, 2012-04-03 22:54:54]

lgtm

[wtc, 2012-04-03 23:31:48]

Patch set 6 LGTM.

Do you have the URL for the upstream open-vcdiff CL?  I'm
curious what the upstream changes are.

[Xianzhu, 2012-04-04 00:05:29]

On 2012/04/03 23:31:48, wtc wrote:
> Patch set 6 LGTM.
> 
> Do you have the URL for the upstream open-vcdiff CL?  I'm
> curious what the upstream changes are.

Thanks for review.

Link to the CL: http://code.google.com/p/open-vcdiff/source/detail?r=41
Also added the link in description of this CL.

[commit-bot: I haz the power, 2012-04-04 00:06:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/9555002/15001

[commit-bot: I haz the power, 2012-04-04 03:26:08]

Change committed as 130552