Closed
Bug 573137
Opened 14 years ago
Closed 14 years ago
Upgrade to zlib 1.2.5
Categories
(Core :: Graphics: ImageLib, enhancement)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: djeter, Assigned: djeter)
References
()
Details
Attachments
(5 files, 8 obsolete files)
4.08 KB,
patch
|
Details | Diff | Splinter Review | |
2.55 KB,
patch
|
benjamin
:
review+
benjamin
:
approval2.0+
|
Details | Diff | Splinter Review |
403.73 KB,
patch
|
joe
:
review+
joe
:
approval2.0+
|
Details | Diff | Splinter Review |
4.99 KB,
text/plain
|
Details | |
4.08 KB,
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.3a6pre) Gecko/20100617 SeaMonkey/2.1a2pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.3a6pre) Gecko/20100617 SeaMonkey/2.1a2pre zlib-1.2.5 has been released. These changes should be picked up. Reproducible: Always
Assignee | ||
Comment 1•14 years ago
|
||
Patch to upgrade modules/zlib to version 1.2.5
Attachment #452348 -
Flags: review?
Comment 2•14 years ago
|
||
Glenn, does this resolve 283214? Also, not sure that Imagelib is really the right component for this. Also not sure who should be reviewing this. Maybe biesi?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•14 years ago
|
||
It doesn't resolve bug #283214 but it would allow us to create a libpr0n patch for it.
Assignee | ||
Comment 4•14 years ago
|
||
Fixed a nit: forgot the mozzconf.h include in zconf.h
Attachment #452348 -
Attachment is obsolete: true
Attachment #452348 -
Flags: review?
Assignee | ||
Comment 5•14 years ago
|
||
Fixed a nit: Use correct Makefile.in
Attachment #452422 -
Attachment is obsolete: true
Assignee | ||
Comment 6•14 years ago
|
||
Last patch forgot to add some files, this patch includes those files.
Attachment #452465 -
Attachment is obsolete: true
Comment 7•14 years ago
|
||
Comment on attachment 452490 [details] [diff] [review] zlib_patch.diff, v4 Technically, I'm the module owner of this code.
Attachment #452490 -
Flags: review?(joe)
Comment 8•14 years ago
|
||
Comment on attachment 452490 [details] [diff] [review] zlib_patch.diff, v4 >diff -u -N -r a/mozilla/modules/zlib/src/ChangeLog.moz b/mozilla/modules/zlib/src/ChangeLog.moz >--- a/mozilla/modules/zlib/src/ChangeLog.moz 2010-04-09 22:49:19 -0500 >+++ b/mozilla/modules/zlib/src/ChangeLog.moz 2010-06-17 17:01:37 -0500 >@@ -42,3 +42,8 @@ > > - 13 September 2009 > Don't enable zlib's debug output when the Mozilla build is in debug mode (bug 431950) >+ >+- 17 June 2010 >+ Sync'ed with 1.2.5 release >+ (keeping '#include "mozzconf.h"' in zconf.h) >+ See bugs #299445 and #300349 >\ No newline at end of file "mozconf.h", and add a newline. This is otherwise a rubber-stamp r+. This should go through a run through the tryserver to make sure it doesn't introduce new bugs.
Attachment #452490 -
Flags: review?(joe) → review+
Assignee | ||
Comment 9•14 years ago
|
||
Non-static builds appear to be busted with 5 unresolved externals in zlib.def... The 5 symbols were added in zlib-1.2.4, so I'll run through a couple builds with those symbols commented out and see if that was the problem. I should have a fixed patch soon.
Assignee | ||
Comment 10•14 years ago
|
||
This patch addresses the nits in comment 8 and removes the 5 symbols that caused the non-static builds to fail.
Updated•14 years ago
|
Assignee: nobody → djeter
blocking2.0: ? → final+
Comment 12•14 years ago
|
||
In mozzconf.h the comment /* New as of libpng-1.2.4 */ should be /* New as of zlib-1.2.4 */ Re: comment #8, I think "mozzconf.h" was correct in ChangeLog.moz
Assignee | ||
Comment 13•14 years ago
|
||
This patch addresses the nits in the comment above.
Attachment #456144 -
Attachment is obsolete: true
Comment 14•14 years ago
|
||
Some Mozilla makefiles define -DZLIB_INTERNAL: http://mxr.mozilla.org/mozilla-central/search?string=-DZLIB_INTERNAL -DZLIB_INTERNAL defines the ZLIB_INTERNAL macro as 1. In zlib 1.2.5, the value of ZLIB_INTERNAL may be defined as __attribute__((visibility ("hidden"))) for GCC 3.3 or later. See gzguts.h and zutil.h. Please watch out for this change. Daniel (djeter), have you tested your patch on Linux or Mac OS X? I think Mozilla makefiles should not define -DZLIB_INTERNAL.
Comment 15•14 years ago
|
||
Re: the ZLIB_DEBUG change to zutil.h: I described an alternative solution in bug 431950 comment 7 that doesn't require patching zutil.h.
Assignee | ||
Comment 16•14 years ago
|
||
I've only tested the patch on Windows.
Comment 17•14 years ago
|
||
Thanks for the reply. That makes sense, as I expect that we will need to deal with the -DZLIB_INTERNAL issue for platforms that use GCC, such as Linux and Mac OS X.
Comment 18•14 years ago
|
||
I recreated a patch that summarizes Mozilla's local changes to zlib 1.2.3.
Assignee | ||
Comment 19•14 years ago
|
||
Over the weekend, I set up a VM to test the patch on Linux. So far, I have made compiles pre-patch and post-patch with GCC versions 4.3.5, 4.4.4 and 4.5.1... All of the builds completed and so far have run successfully for me. I don't have a way to test the patch on Mac OS X, though I can set up more Linux VM's that use older versions of GCC and test the patch there to see if any problems come up.
Comment 20•14 years ago
|
||
joedrew: I just updated the zlib in NSS to zlib 1.2.5, so I can review and check in djeter's patch. Two questions for you: - algorithms.txt became a doc/ directory with four files. Do you want * just doc/algorihtms.txt, * all four files in doc/, or * none? - zlib 1.2.5 added zlib.3.pdf, which is the PDF version of zlib.3. I assume you don't want it. (I actually think we don't need zlib.3 either.) djeter: thank you for your work on this bug. Since this kind of patch can't be reviewed by inspection, I duplicated your work and compared the results last weekend. The differences are minor. I'll describe them later. The main issue is the -DZLIB_INTERNAL problem I described in comment 14. Although your test build on Linux worked, there are compiler warnings like this: adler32.cgcc -o adler32.o -c -I../../../dist/system_wrappers -include /usr/local/google/home/wtc/mozilla-central.djeter/src/config/gcc_hidden.h -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -DZLIB_INTERNAL -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -I/usr/local/google/home/wtc/mozilla-central.djeter/src/modules/zlib/src -I. -I../../../dist/include -I../../../dist/include/nsprpub -I/usr/local/google/home/wtc/mozilla-central.djeter/ff-dbg/dist/include/nspr -I/usr/local/google/home/wtc/mozilla-central.djeter/ff-dbg/dist/include/nss -fPIC -Wall -W -Wno-unused -Wpointer-arith -Wcast-align -W -pedantic -Wno-long-long -fno-strict-aliasing -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -include ../../../mozilla-config.h -DMOZILLA_CLIENT -MD -MF .deps/adler32.pp /usr/local/google/home/wtc/mozilla-central.djeter/src/modules/zlib/src/adler32.c In file included from /usr/local/google/home/wtc/mozilla-central.djeter/src/modules/zlib/src/adler32.c:8:/usr/local/google/home/wtc/mozilla-central.djeter/src/modules/zlib/src/zutil.h:17:1: warning: "ZLIB_INTERNAL" redefined The fix is to remove -DZLIB_INTERNAL from Mozilla makefiles.
Comment 21•14 years ago
|
||
wtc: I actually am not choosy about what you include or don't include. If I was going to be choosy, though, I might drop all the docs, including the man page and pdf.
Comment 22•14 years ago
|
||
joedrew: I agree. I'll drop all the docs. I am researching -DZLIB_INTERNAL now. I've tracked it down to bug 272783 and bug 273876. I hope bsmedberg still remembers it as he added it six years ago.
Comment 23•14 years ago
|
||
Assuming the OS2 code doesn't get enough attention, this patch removes two pieces of code that cancel each other.
Attachment #471006 -
Flags: review?(benjamin)
Comment 24•14 years ago
|
||
Ben: this patch reverts some of your zlib-related changes in Dec 2004 for libxul (bug 272783 and bug 273876). ZLIB_INTERNAL should only be defined when compiling the files inside zlib. Files that use zlib should not be compiled with -DZLIB_INTERNAL. With zlib 1.2.5, -DZLIB_INTERNAL causes compiler warnings on Linux (shown at the end of comment 20) because zlib 1.2.5 (zutil.h) defines ZLIB_INTERNAL as #if ((__GNUC__-0) * 10 + __GNUC_MINOR__-0 >= 33) && !defined(NO_VIZ) # define ZLIB_INTERNAL __attribute__((visibility ("hidden"))) #else # define ZLIB_INTERNAL #endif This patch is a prerequisite for the zlib 1.2.5 update, which has blocking2.0=final+ status. Therefore I'm requesting approval2.0.
Attachment #471011 -
Flags: review?(benjamin)
Attachment #471011 -
Flags: approval2.0?
Comment 25•14 years ago
|
||
In comment 20 I truncated the compiler warning. The complete warning has two lines: /usr/local/google/home/wtc/mozilla-central.djeter/src/modules/zlib/src/zutil.h:17:1: warning: "ZLIB_INTERNAL" redefined <command-line>: warning: this is the location of the previous definition
Comment 26•14 years ago
|
||
Comment on attachment 471006 [details] [diff] [review] Optional patch: remove code that defines ZLIB_DLL and then undefines it http://mxr.mozilla.org/mozilla-central/search?string=zlib_dll lists several other places where we define ZLIB_DLL and presumably shouldn't any more, right? Previously ZLIB_INTERNAL was needed to control dllimport/dllexport definitions, so if we're not building ZLIB_DLL in any configuration, we don't need the ZLIB_INTERNAL stuff either. But it's not clear to me how this affects the non-libxul configuration, where I thought we *were* still building a separate mozzlib.dll
Comment 27•14 years ago
|
||
Ben: Thank you for your comments. I did a lot of MXR searches to research -DZLIB_DLL and -DZLIB_INTERNAL. I'm afraid that I may have missed something, but I think the following is what led to the current mess. 1. Several Mozilla makefiles defines ZLIB_DLL on Windows for all build configurations. For example, netwerk/streamconv/converters/Makefile.in has: ifeq ($(OS_ARCH),WINNT) DEFINES += -DZLIB_DLL endif 2. The ZLIB_DLL definitions in these makefiles weren't updated when the libxul configuration was added. I think only xpinstall/src/Makefile.in got it right: ifeq ($(OS_ARCH)$(MOZ_ENABLE_LIBXUL),WINNT) DEFINES += -DZLIB_DLL endif 3. As a result, we added -DZLIB_INTERNAL for the libxul configuration to suppress MSVC linker warnings caused by dllimport. If my reconstruction of past events is correct, I think the correct fix is to define -DZLIB_DLL only for the non-libxul configuration, and remove -DZLIB_INTERNAL. Note: did xul.dll ever export the zlib functions as part of the libxul API? That would be a reason to compile all libxul files as zlib-internal.
Comment 28•14 years ago
|
||
Here is my edited version of djeter's patch. I made the following changes. 1. Nit: remove the blank line at the end of ChangeLog.moz. Joe, I expect that you will update ChangeLog.moz when you commit this patch. 2. mozzconf.h: rename more new functions of zlib 1.2.4. I discovered them by inspecting zconf.h. 3. zconf.h: resurrect Mozilla's local change for __OS2__. 4. zlib.def: I generated this with this sed command: sed 's/^ / MOZ_Z_/g' zlib-1.2.5/win32/zlib.def 5. zutil.h: I applied Mozilla's local changes using the 'patch' command. djeter must have applied the changes manually so that there were a TAB character and Windows line endings (CRLF). Joe, this patch is equivalent to my review+ on djeter's patch. I will attach a "patch interdiff" patch to help you review this patch.
Attachment #452490 -
Attachment is obsolete: true
Attachment #459985 -
Attachment is obsolete: true
Attachment #471149 -
Flags: review?(joe)
Comment 29•14 years ago
|
||
Comment 30•14 years ago
|
||
wtc, I believe your analysis is correct. We may have exported the zlib symbols from libxul at some point, but we don't now and shouldn't.
Updated•14 years ago
|
Attachment #471011 -
Flags: review?(benjamin)
Attachment #471011 -
Flags: review+
Attachment #471011 -
Flags: approval2.0?
Attachment #471011 -
Flags: approval2.0+
Comment 31•14 years ago
|
||
Comment on attachment 471006 [details] [diff] [review] Optional patch: remove code that defines ZLIB_DLL and then undefines it I'm less-sure about this one, can you re-request review if a windows non-libxul build works?
Attachment #471006 -
Flags: review?(benjamin)
Comment 32•14 years ago
|
||
Comment on attachment 471149 [details] [diff] [review] zlib_patch.diff, v6 by Daniel Jeter II (pushed to mozilla-central) Wan-Teh, it's not clear to me what you want me to do with Changelog.moz? Everything else looks OK though.
Attachment #471149 -
Flags: review?(joe) → review+
Comment 33•14 years ago
|
||
Comment on attachment 471011 [details] [diff] [review] Prerequisite patch: remove -DZLIB_INTERNAL (pushed to mozilla-central) Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/983c38bb0f03
Attachment #471011 -
Attachment description: Prerequisite patch: remove -DZLIB_INTERNAL → Prerequisite patch: remove -DZLIB_INTERNAL (pushed to mozilla-central)
Comment 34•14 years ago
|
||
(In reply to comment #32) Joe: please replace the ChangeLog.moz entry when you commit the patch. At least the date needs to be updated.
Comment 35•14 years ago
|
||
Ben, please review this patch. This patch assumes that we're building zlib as a DLL only if neither BUILD_STATIC_LIBS (--enable-static) nor MOZ_ENABLE_LIBXUL (--enable-libxul) is set. Is this correct? This patch moves the common code into config/config.mk. Makefiles that need to define ZLIB_DLL just need to set the make variable INCLUDE_ZLIB_H to 1. (Is the variable name OK?) This approach is difficult to maintain because failure to define ZLIB_DLL is just a tiny performance penalty. I used MXR to find some other makefiles that should also define ZLIB_DLL, but it was a painful process. So I'm wondering if we should instead define ZLIB_DLL in mozzconf.h as follows: #if defined(WIN32) || defined(OS2) #if !defined(MOZ_STATIC_BUILD) && !defined(MOZ_ENABLE_LIBXUL) #define ZLIB_DLL 1 #endif #endif Would you prefer this alternative approach?
Attachment #471006 -
Attachment is obsolete: true
Attachment #471895 -
Flags: review?(benjamin)
Comment 36•14 years ago
|
||
Comment on attachment 471895 [details] [diff] [review] Optional patch: define ZLIB_DLL only when we're building or using zlib as a DLL I'd just skip the INCLUDE_ZLIB_H bit... can't we can just do this globally?
Comment 37•14 years ago
|
||
Comment on attachment 471895 [details] [diff] [review] Optional patch: define ZLIB_DLL only when we're building or using zlib as a DLL Ben: we can define ZLIB_DLL globally. We can also just give up the tiny performance gain and not worry about ZLIB_DLL. One reason I'm proposing the latter is that I don't fully understand all the possible build configurations. Do you know what modules/zlib/standalone/Makefile.in is used for? In the two build configurations I tested (libxul and non-libxul), modules/zlib/standalone/Makefile.in builds a static library mozz_s.lib, but the static library is not being used.
Comment 38•14 years ago
|
||
Hrm, I thought standalone got removed a long time ago: IIRC it was only used by the ancient xpinstall code which we no longer build.
Updated•14 years ago
|
Attachment #471149 -
Flags: approval2.0?
Updated•14 years ago
|
Attachment #471149 -
Flags: approval2.0? → approval2.0+
Comment 39•14 years ago
|
||
Comment on attachment 471149 [details] [diff] [review] zlib_patch.diff, v6 by Daniel Jeter II (pushed to mozilla-central) Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/fb3a132de4ec http://hg.mozilla.org/mozilla-central/rev/2e571d635a60 The second changeset reverted one of our changes to zutil.h that defines HAVE_VSNPRINTF for recent versions of MSVC, because that issue has been addressed (in a different way), described by this entry in ChangeLog: Changes in 1.2.3.5 (8 Jan 2010) ... - Don't use _vsnprintf on later versions of MSVC [Lowman]
Attachment #471149 -
Attachment description: zlib_patch.diff, v6 by Daniel Jeter II → zlib_patch.diff, v6 by Daniel Jeter II (pushed to mozilla-central)
Comment 40•14 years ago
|
||
This patch is for future reference. I generated our zlib.def using this sed command: sed 's/^ / MOZ_Z_/g' zlib-1.2.5/win32/zlib.def
Attachment #471895 -
Attachment is obsolete: true
Attachment #471895 -
Flags: review?(benjamin)
Comment 41•14 years ago
|
||
Marked the bug fixed. I will deal with -DZLIB_DLL in a new bug report after Firefox 4.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
Updated•14 years ago
|
Target Milestone: mozilla2.0 → mozilla2.0b7
Comment 42•14 years ago
|
||
About dropping the docs, should we also drop docs from the next libpng update (especially CHANGES which is always a major part of the patch)?
Comment 43•14 years ago
|
||
I should have noted in comment 39 that I removed algorithm.txt and zlib.3 before pushing the zlib_patch.diff, v6 patch: http://hg.mozilla.org/mozilla-central/rev/6066d938a0dd I think ChangeLog, FAQ, and INDEX can also be removed.
You need to log in
before you can comment on or make changes to this bug.
Description
•