Closed Bug 573137 Opened 10 years ago Closed 10 years ago
Upgrade to zlib 1
4.08 KB, patch
|Details | Diff | Splinter Review|
2.55 KB, patch
|Details | Diff | Splinter Review|
403.73 KB, patch
|Details | Diff | Splinter Review|
4.99 KB, text/plain
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
Patch to upgrade modules/zlib to version 1.2.5
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
It doesn't resolve bug #283214 but it would allow us to create a libpr0n patch for it.
Fixed a nit: forgot the mozzconf.h include in zconf.h
Fixed a nit: Use correct Makefile.in
Attachment #452422 - Attachment is obsolete: true
Last patch forgot to add some files, this patch includes those files.
Attachment #452465 - Attachment is obsolete: true
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 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+
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.
This patch addresses the nits in comment 8 and removes the 5 symbols that caused the non-static builds to fail.
Marking blocking 2.0? because this blocks bug 581181.
blocking2.0: --- → ?
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
This patch addresses the nits in the comment above.
Attachment #456144 - Attachment is obsolete: true
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.
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.
I've only tested the patch on Windows.
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.
I recreated a patch that summarizes Mozilla's local changes to zlib 1.2.3.
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.
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.
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.
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.
Assuming the OS2 code doesn't get enough attention, this patch removes two pieces of code that cancel each other.
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.
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 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
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.
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.
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.
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?
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 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)
(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.
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?
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 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.
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.
Attachment #471149 - Flags: approval2.0? → approval2.0+
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 220.127.116.11 (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)
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
Marked the bug fixed. I will deal with -DZLIB_DLL in a new bug report after Firefox 4.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
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)?
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.
4 years ago
Depends on: 301646
You need to log in before you can comment on or make changes to this bug.