Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(24)

Issue 9555002: Avoid zlib symbol conflicts in open-vcdiff (Closed)

Created:
8 years, 9 months ago by Xianzhu
Modified:
8 years, 8 months ago
CC:
chromium-reviews
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Fix tab char #

Total comments: 2

Patch Set 3 : Depend on zlib #

Total comments: 4

Patch Set 4 : Don't exclude zlib.h and zconf.h #

Total comments: 1

Patch Set 5 : Depend on zlib (need patch in http://code.google.com/p/open-vcdiff/issues/detail?id=34) #

Patch Set 6 : Roll open-vcdiff 40:42 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -4 lines) Patch
M DEPS View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M sdch/sdch.gyp View 1 2 3 4 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 23 (0 generated)
Xianzhu
8 years, 9 months ago (2012-02-29 22:17:29 UTC) #1
tonyg
Just one question, but I'm probably not a great reviewer for sdch. Can you annotate ...
8 years, 9 months ago (2012-03-01 11:32:53 UTC) #2
Xianzhu
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 ...
8 years, 9 months ago (2012-03-01 17:44:44 UTC) #3
Mark Mentovai
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” ...
8 years, 9 months ago (2012-03-01 19:46:34 UTC) #4
Xianzhu
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 ...
8 years, 9 months ago (2012-03-01 21:28:55 UTC) #5
Mark Mentovai
Please be sure you’re #including the copy of the header that goes along with the ...
8 years, 9 months ago (2012-03-01 21:33:41 UTC) #6
Xianzhu
Then I'd prefer the method used in Patch Set 2, otherwise it's seems unavoidable to ...
8 years, 9 months ago (2012-03-01 21:38:06 UTC) #7
Mark Mentovai
Then we’re back to unnecessarily building two copies of adler32. That’s bad too. I’m afraid ...
8 years, 9 months ago (2012-03-01 21:57:45 UTC) #8
Xianzhu
Should the change be upstreamed to sdch, or do we allow local changes? On Thu, ...
8 years, 9 months ago (2012-03-01 22:02:18 UTC) #9
Mark Mentovai
We pull sdch/open-vcdiff directly from the upstream. It’s always preferable to make the changes upstream ...
8 years, 9 months ago (2012-03-01 22:06:04 UTC) #10
wtc
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 ...
8 years, 9 months ago (2012-03-01 22:38:29 UTC) #11
Mark Mentovai
We can’t just svn remove outright because we pull directly from the upstream via DEPS. ...
8 years, 9 months ago (2012-03-01 22:41:09 UTC) #12
Xianzhu
On 2012/03/01 22:41:09, Mark Mentovai wrote: > We can’t just svn remove outright because we ...
8 years, 9 months ago (2012-03-01 23:17:04 UTC) #13
Mark Mentovai
That won’t work. You can use None to exclude a repository listed in DEPS from ...
8 years, 9 months ago (2012-03-01 23:21:26 UTC) #14
Xianzhu
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 ...
8 years, 8 months ago (2012-03-29 17:02:17 UTC) #15
Lincoln
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 ...
8 years, 8 months ago (2012-04-03 21:39:56 UTC) #16
Xianzhu
8 years, 8 months ago (2012-04-03 22:34:51 UTC) #17
Xianzhu
Hi, reviewers, Could you review the last change set? It rolls in the latest open-vcdiff ...
8 years, 8 months ago (2012-04-03 22:42:49 UTC) #18
Lincoln
lgtm
8 years, 8 months ago (2012-04-03 22:54:54 UTC) #19
wtc
Patch set 6 LGTM. Do you have the URL for the upstream open-vcdiff CL? I'm ...
8 years, 8 months ago (2012-04-03 23:31:48 UTC) #20
Xianzhu
On 2012/04/03 23:31:48, wtc wrote: > Patch set 6 LGTM. > > Do you have ...
8 years, 8 months ago (2012-04-04 00:05:29 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/9555002/15001
8 years, 8 months ago (2012-04-04 00:06:39 UTC) #22
commit-bot: I haz the power
8 years, 8 months ago (2012-04-04 03:26:08 UTC) #23
Change committed as 130552

Powered by Google App Engine
This is Rietveld 408576698