Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1414393011:
Fix Linux unbundled zlib.gyp's duplicate target. (Closed)
DescriptionFix Linux unbundled zlib.gyp's duplicate target.
The target already exists in third_party/zlib/google/zip.gyp.
Also:
- Remove stale reference to use_system_zlib
- Fix build warnings in spdy_framer.cc.
TBR=bnc@chromium.org
Committed: https://crrev.com/94a6d7f376c8e51586e8273577bb018ecdc6f1df
Cr-Commit-Position: refs/heads/master@{#363834}
Patch Set 1 #
Messages
Total messages: 36 (13 generated)
thestig@chromium.org changed reviewers: + phajdan.jr@chromium.org
Curious: what prompted this change? Since this is not officially supported, I'd prefer not to change anything here since the current solution works for me. I'll be the first one to make a patch when it stops working. Also, to potentially reduce review latency: if you'd still like to land this, could you explain how this was tested? We have no trybot for this for obvious reasons. The way I test is with most contents of third_party/zlib actually removed, to make sure there is no way they can be used.
On 2015/11/06 15:29:46, Paweł Hajdan Jr. wrote: > Curious: what prompted this change? I wanted to fix the zlib portion of https://crbug.com/541704 so I was just curious how this worked. I've never tried any of the unbundle scripts, so hey, why not? > Since this is not officially supported, I'd prefer not to change anything here > since the current solution works for me. I'll be the first one to make a patch > when it stops working. Ok, I'm letting you know it stopped working. > Also, to potentially reduce review latency: if you'd still like to land this, > could you explain how this was tested? We have no trybot for this for obvious > reasons. I ran the unbundle script, ran gclient runhooks, and kicked off a build with Ninja. Then Ninja immediately complained about duplicate targets. Even with this CL, on Ubuntu I cannot get the final link to work, because there's no proper minizip package. There may be one on Gentoo or elsewhere. > The way I test is with most contents of third_party/zlib actually removed, to > make sure there is no way they can be used. I did not do that, but I think if I did remove all of third_party/zlib, gyp would fail because the "zip" target has moved to third_party/zlib/google/zip.gyp and there's other gyp files that refer to it.
On 2015/11/06 at 18:06:32, thestig wrote: > > Since this is not officially supported, I'd prefer not to change anything here > > since the current solution works for me. I'll be the first one to make a patch > > when it stops working. > > Ok, I'm letting you know it stopped working. Just to confirm, did this previously work for you and then stopped? It worked for me just fine for the last dev channel release (48.0.2552.0). I'm just wondering if it's a very recent regression. > > Also, to potentially reduce review latency: if you'd still like to land this, > > could you explain how this was tested? We have no trybot for this for obvious > > reasons. > > I ran the unbundle script, ran gclient runhooks, and kicked off a build with Ninja. Then Ninja immediately complained about duplicate targets. > > Even with this CL, on Ubuntu I cannot get the final link to work, because there's no proper minizip package. There may be one on Gentoo or elsewhere. Yes, there is one on Gentoo. I don't really see though how changes made in this CL are related to minizip. I think the ninja warnings you've mentioned are non-fatal, right? > > The way I test is with most contents of third_party/zlib actually removed, to > > make sure there is no way they can be used. > > I did not do that, but I think if I did remove all of third_party/zlib, gyp would fail because the "zip" target has moved to third_party/zlib/google/zip.gyp and there's other gyp files that refer to it. I'd typically preserve .gyp (and .gn) files - see remove_bundled_libraries.py. In general, you might be hitting an unsupported use case of tools that are not officially supported anyway.
On 2015/11/09 10:18:58, Paweł Hajdan Jr. wrote: > Just to confirm, did this previously work for you and then stopped? This is the first and only time I've tried this. So I don't know if it ever worked. > It worked for me just fine for the last dev channel release (48.0.2552.0). I'm > just wondering if it's a very recent regression. What OS did you build on? third_party/zlib/google/zip.gyp has existed for a long time. I don't see how this could have worked properly. By "worked properly" I mean it builds without ninja complaining about duplicate target names. > Yes, there is one on Gentoo. I don't really see though how changes made in this > CL are related to minizip. > > I think the ninja warnings you've mentioned are non-fatal, right? They are non-fatal, but the warning basically says "the output is invalid". To be specific, I started at ToT and ran: $ ./build/linux/unbundle/replace_gyp_files.py -Duse_system_zlib=1 $ GYP_DEFINES='component=static_library use_goma=1' gclient runhooks $ ninja -C out/Debug chrome ninja: Entering directory `out/Debug' ninja: warning: multiple rules generate obj/third_party/zlib/google/zip.zip.o. builds involving this target will not be correct; continuing anyway [-w dupbuild=warn] ninja: warning: multiple rules generate obj/third_party/zlib/google/zip.zip_internal.o. builds involving this target will not be correct; continuing anyway [-w dupbuild=warn] ninja: warning: multiple rules generate obj/third_party/zlib/google/zip.zip_reader.o. builds involving this target will not be correct; continuing anyway [-w dupbuild=warn] > > > The way I test is with most contents of third_party/zlib actually removed, > to > > > make sure there is no way they can be used. > > > > I did not do that, but I think if I did remove all of third_party/zlib, gyp > would fail because the "zip" target has moved to third_party/zlib/google/zip.gyp > and there's other gyp files that refer to it. > > I'd typically preserve .gyp (and .gn) files - see remove_bundled_libraries.py. > In general, you might be hitting an unsupported use case of tools that are not > officially supported anyway. I ran remove_bundled_libraries.py, but the list of files was long and scary. I tried again just now, and only removed the relevant files in third_party/zlib. Now, third_party/zlib/ contains: third_party/zlib/ third_party/zlib/BUILD.gn third_party/zlib/contrib third_party/zlib/contrib/minizip third_party/zlib/google third_party/zlib/google/test third_party/zlib/google/test/data third_party/zlib/google/test/data/test third_party/zlib/google/test/data/test/foo third_party/zlib/google/test/data/test/foo/bar third_party/zlib/google/zip.gyp third_party/zlib/zlib.gyp and the ninja warning above still applies.
Description was changed from ========== Fix Linux unbundled zlip.gyp's duplicate target. The target already exists in third_party/zlib/google/zip.gyp. Also: - Remove stale reference to use_system_zlib - Fix build warnings in spdy_framer.cc. ========== to ========== Fix Linux unbundled zlib.gyp's duplicate target. The target already exists in third_party/zlib/google/zip.gyp. Also: - Remove stale reference to use_system_zlib - Fix build warnings in spdy_framer.cc. ==========
So how about I drop build/linux/unbundle/zlib.gyp and make a CL out of the rest. I don't think there's any contention on those parts, so we can make progress.
On 2015/11/20 at 02:01:27, thestig wrote: > So how about I drop build/linux/unbundle/zlib.gyp and make a CL out of the rest. I don't think there's any contention on those parts, so we can make progress. I'm actually not convinced by the change to net/spdy/spdy_framer.cc . In my testing, it was not needed for system zlib. I'm worried it might break it then.
On 2015/11/20 14:03:33, Paweł Hajdan Jr. wrote: > On 2015/11/20 at 02:01:27, thestig wrote: > > So how about I drop build/linux/unbundle/zlib.gyp and make a CL out of the > rest. I don't think there's any contention on those parts, so we can make > progress. > > I'm actually not convinced by the change to net/spdy/spdy_framer.cc . In my > testing, it was not needed for system zlib. I'm worried it might break it then. Your test is not working right then. IsCookieEmpty() is defined in an anonymous namespace in spdy_framer.cc, so all the callers are in the file. They are in fact all in SpdyFramer::WriteHeaderBlockToZ(), which is in turn inside a #if !defined(USE_SYSTEM_ZLIB) section.
On 2015/11/21 at 01:50:58, thestig wrote: > Your test is not working right then. I'm not enabling warnings. That could explain it. But it's valid for packaging, since the toolchain is likely to be different from one used for developing Chromium. > IsCookieEmpty() is defined in an anonymous namespace in spdy_framer.cc, so all the callers are in the file. They are in fact all in SpdyFramer::WriteHeaderBlockToZ(), which is in turn inside a #if !defined(USE_SYSTEM_ZLIB) section. Ah, that makes sense. Maybe I was too skeptical about this patch. How about we give it a try, and if I hit issues during packaging, I might revert/change parts of it? Based on above reasoning, it's actually unlikely. In that case, LGTM, and sorry about the delay.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414393011/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414393011/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
thestig@chromium.org changed reviewers: + bnc@chromium.org, scottmg@chromium.org
bnc: net/spdy scottmg: build/ stamp
build lgtm (do you want to add yourself there?)
On 2015/12/05 00:58:49, scottmg wrote: > build lgtm > > (do you want to add yourself there?) Meh, I only change it about once a month. I'm already in 43 OWNERS files. I think that's enough.
On 2015/12/05 01:02:26, Lei Zhang wrote: > On 2015/12/05 00:58:49, scottmg wrote: > > build lgtm > > > > (do you want to add yourself there?) > > Meh, I only change it about once a month. I'm already in 43 OWNERS files. I > think that's enough. It was worth a try. :)
Description was changed from ========== Fix Linux unbundled zlib.gyp's duplicate target. The target already exists in third_party/zlib/google/zip.gyp. Also: - Remove stale reference to use_system_zlib - Fix build warnings in spdy_framer.cc. ========== to ========== Fix Linux unbundled zlib.gyp's duplicate target. The target already exists in third_party/zlib/google/zip.gyp. Also: - Remove stale reference to use_system_zlib - Fix build warnings in spdy_framer.cc. TBR=bnc@chromium.org ==========
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414393011/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414393011/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
net LGTM. Sorry for the late response.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414393011/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414393011/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414393011/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414393011/1
Message was sent while issue was closed.
Description was changed from ========== Fix Linux unbundled zlib.gyp's duplicate target. The target already exists in third_party/zlib/google/zip.gyp. Also: - Remove stale reference to use_system_zlib - Fix build warnings in spdy_framer.cc. TBR=bnc@chromium.org ========== to ========== Fix Linux unbundled zlib.gyp's duplicate target. The target already exists in third_party/zlib/google/zip.gyp. Also: - Remove stale reference to use_system_zlib - Fix build warnings in spdy_framer.cc. TBR=bnc@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix Linux unbundled zlib.gyp's duplicate target. The target already exists in third_party/zlib/google/zip.gyp. Also: - Remove stale reference to use_system_zlib - Fix build warnings in spdy_framer.cc. TBR=bnc@chromium.org ========== to ========== Fix Linux unbundled zlib.gyp's duplicate target. The target already exists in third_party/zlib/google/zip.gyp. Also: - Remove stale reference to use_system_zlib - Fix build warnings in spdy_framer.cc. TBR=bnc@chromium.org Committed: https://crrev.com/94a6d7f376c8e51586e8273577bb018ecdc6f1df Cr-Commit-Position: refs/heads/master@{#363834} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/94a6d7f376c8e51586e8273577bb018ecdc6f1df Cr-Commit-Position: refs/heads/master@{#363834} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
