Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#12800 closed defect (fixed)

Upgrade zlib to 1.2.6

Reported by: jdemeyer Owned by: tbd
Priority: major Milestone: sage-5.0
Component: packages: standard Keywords:
Cc: mjo Merged in: sage-5.0.beta14
Authors: Jeroen Demeyer Reviewers: Michael Orlitzky, Julien Puydt, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Changes in 1.2.6 (29 Jan 2012)
- Update the Pascal interface in contrib/pascal
- Fix function numbers for gzgetc_ in zlibvc.def files
- Fix configure.ac for contrib/minizip [Schiffer]
- Fix large-entry detection in minizip on 64-bit systems [Schiffer]
- Have ./configure use the compiler return code for error indication
- Fix CMakeLists.txt for cross compilation [McClure]
- Fix contrib/minizip/zip.c for 64-bit architectures [Dalsnes]
- Fix compilation of contrib/minizip on FreeBSD [Marquez]
- Correct suggested usages in win32/Makefile.msc [Shachar, Horvath]
- Include io.h for Turbo C / Borland C on all platforms [Truta]
- Make version explicit in contrib/minizip/configure.ac [Bosmans]
- Avoid warning for no encryption in contrib/minizip/zip.c [Vollant]
- Minor cleanup up contrib/minizip/unzip.c [Vollant]
- Fix bug when compiling minizip with C++ [Vollant]
- Protect for long name and extra fields in contrib/minizip [Vollant]
- Avoid some warnings in contrib/minizip [Vollant]
- Add -I../.. -L../.. to CFLAGS for minizip and miniunzip
- Add missing libs to minizip linker command
- Add support for VPATH builds in contrib/minizip
- Add an --enable-demos option to contrib/minizip/configure
- Add the generation of configure.log by ./configure
- Exit when required parameters not provided to win32/Makefile.gcc
- Have gzputc return the character written instead of the argument
- Use the -m option on ldconfig for BSD systems [Tobias]
- Correct in zlib.map when deflateResetKeep was added

In particular, it builds on the Skynet machine mark (SunOS 5.10-32) with GCC-4.7.0, unlike zlib-1.2.5.

spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/zlib-1.2.6.spkg

Attachments (2)

zlib-1.2.6.reviewer-ll.patch (2.1 KB) - added by leif 10 years ago.
Reviewer patch. Apply to Jeroen's spkg.
zlib-1.2.6.diff (3.2 KB) - added by jdemeyer 10 years ago.
Diff for the zlib spkg. For review only

Download all attachments as: .zip

Change History (24)

comment:1 Changed 10 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by mjo

  • Cc mjo added

We might as well just get of the commented-out lines. We're using version control, after all. If they're done in a separate commit, just those lines can be hg backouted.

comment:3 Changed 10 years ago by mjo

Alternately, fix the patch routine =)

comment:4 Changed 10 years ago by jdemeyer

Well, #12432 adds a patch again, so it doesn't really matter.

comment:5 Changed 10 years ago by Snark

The spkg compiled fine with sage-4.8 on ARM.

Changed 10 years ago by leif

Reviewer patch. Apply to Jeroen's spkg.

comment:6 Changed 10 years ago by leif

  • Reviewers set to Michael Orlitzky, Julien Puydt, Leif Leonhardy
  • Status changed from needs_review to needs_work

spkg-install needs some work; see my attached reviewer patch.

Doesn't make sense to comment out the patch loop; I wouldn't have deleted the patches/ directory either (but my patch doesn't restore it).

comment:7 in reply to: ↑ description Changed 10 years ago by leif

Replying to jdemeyer:

Changes in 1.2.6 (29 Jan 2012)
[...]

In particular, it builds on the Skynet machine mark (SunOS 5.10-32) with GCC-4.7.0, unlike zlib-1.2.5.

Well, none of the other changes seem to be relevant for Sage. (But regarding the size of the spkg, it IMHO isn't worth deleting some parts from the upstream tree.)

Changed 10 years ago by jdemeyer

Diff for the zlib spkg. For review only

comment:8 Changed 10 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Leif, I added almost all of your changes, the spkg is at the same location. I patched the patch loop in a different way, now it is:

# Apply all patches
for patch in ../patches/*.patch; do
    [ -f "$patch" ] || continue
    basename "$patch"
    if ! patch -p1 <"$patch"; then
        echo >&2 "Error: patch '$patch' failed to apply."
        exit 1
    fi
done

comment:9 follow-up: Changed 10 years ago by Snark

Does that loop works even if the ../patches/ directory is empty? Or does it give a file not found error? Not that it matters as long as there is a patch, but that's the first thing that came to my mind reading this...

comment:10 in reply to: ↑ 9 ; follow-up: Changed 10 years ago by jdemeyer

Replying to Snark:

Does that loop works even if the ../patches/ directory is empty? Or does it give a file not found error?

If ../patches is either empty or non-existent, then ../patches/* will expand literally to "../patches/*". Since "../patches/*" isn't a file, the line

[ -f "$patch" ] || continue

will ensure that there are no error messages.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 10 years ago by leif

Replying to jdemeyer:

Replying to Snark:

Does that loop works even if the ../patches/ directory is empty? Or does it give a file not found error?

If ../patches is either empty or non-existent, then ../patches/* will expand literally to "../patches/*". Since "../patches/*" isn't a file, the line

[ -f "$patch" ] || continue

will ensure that there are no error messages.

Yes, but it then would make more sense to break rather than continue.

But to perform some loop-invariant code motion, it would be better to just use

ls ../patches/*.patch &>/dev/null &&
echo "Applying patches to upstream source..." &&
for patch in ../patches/*.patch; do
    ...

comment:12 follow-up: Changed 10 years ago by leif

P.S.:

$ ls patches/
*.patch

Then your continue would be correct... ;-)

comment:13 in reply to: ↑ 12 Changed 10 years ago by leif

Replying to leif:

P.S.:

$ ls patches/
*.patch

Then your continue would be correct... ;-)

Although in that case also [ -f "$patch" ] yields true, so continue isn't reached.

comment:14 in reply to: ↑ 11 ; follow-up: Changed 10 years ago by jdemeyer

Replying to leif:

Yes, but it then would make more sense to break rather than continue.

I don't think there is anything wrong with my code. It's simple and actually catches more special cases than your proposals (such as patches/foo.patch being a directory).

comment:15 in reply to: ↑ 14 Changed 10 years ago by leif

Replying to jdemeyer:

Replying to leif:

Yes, but it then would make more sense to break rather than continue.

I don't think there is anything wrong with my code. It's simple and actually catches more special cases than your proposals (such as patches/foo.patch being a directory).

Well, I think the only "special cases" we want to (or have to) handle here are:

  • ../patches/ doesn't exist.
  • The directory exists, but it is empty, or at least doesn't contain files matching *.patch.

(I won't insist on changing the continue, but it seems more natural [and actually is more efficient] to place a single test outside the loop. Also, conditionally printing "Applying patches..." seems reasonable to me.)


If you wanted to go triple-safe, you'd have to use [ -r "$patch" ] instead of [ -f ... ].

comment:16 Changed 10 years ago by Snark

Debian maintainers manage their patches to upstream using quilt.

Debian has about fourty thousand packages, with eleven official ports and a few unofficial ones, so I guess they know what they're doing.

comment:17 follow-up: Changed 10 years ago by mjo

We could just apply the patches in a mercurial queue, since we ship a repo with every spkg. Anyway, this is pointless: I've built a fresh beta13 with this spkg, and it passes a ptestlong. I manually checked the error conditions. When you figure out what to do with the patch routine (delete it!), it's ready.

comment:18 in reply to: ↑ 17 Changed 10 years ago by leif

  • Status changed from needs_review to positive_review

Replying to mjo:

I've built a fresh beta13 with this spkg, and it passes a ptestlong. I manually checked the error conditions. When you figure out what to do with the patch routine (delete it!), it's ready.

Yep. I'm ok with the spkg as is, so setting it to positive review.

If anybody should disagree, feel free to revert that.

comment:19 Changed 10 years ago by jdemeyer

  • Merged in set to sage-5.0.beta14
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:20 Changed 10 years ago by jhpalmieri

I'm having problems with this on the OpenSolaris machine hawk: when trying to build sage-5.1.beta3, I see

gcc -O3  -D_LARGEFILE64_SOURCE=1 -o example64 example64.o -L. libz.a
gcc -O3  -D_LARGEFILE64_SOURCE=1 -o minigzip64 minigzip64.o -L. libz.a
ld: fatal: relocation error: R_386_GOTOFF: file deflate.lo: symbol _length_code: a GOT relative relocation must reference a local symbol
ld: fatal: relocation error: R_386_GOTOFF: file deflate.lo: symbol _dist_code: a GOT relative relocation must reference a local symbol
ld: fatal: relocation error: R_386_GOTOFF: file deflate.lo: symbol _dist_code: a GOT relative relocation must reference a local symbol
ld: fatal: relocation error: R_386_GOTOFF: file deflate.lo: symbol _length_code: a GOT relative relocation must reference a local symbol
ld: fatal: relocation error: R_386_GOTOFF: file deflate.lo: symbol _dist_code: a GOT relative relocation must reference a local symbol
ld: fatal: relocation error: R_386_GOTOFF: file deflate.lo: symbol _dist_code: a GOT relative relocation must reference a local symbol
ld: fatal: relocation error: R_386_GOTOFF: file deflate.lo: symbol _length_code: a GOT relative relocation must reference a local symbol
ld: fatal: relocation error: R_386_GOTOFF: file deflate.lo: symbol _dist_code: a GOT relative relocation must reference a local symbol
ld: fatal: relocation error: R_386_GOTOFF: file deflate.lo: symbol zcalloc: a GOT relative relocation must reference a local symbol
ld: fatal: relocation error: R_386_GOTOFF: file deflate.lo: symbol zcfree: a GOT relative relocation must reference a local symbol
collect2: ld returned 1 exit status
make[2]: *** [libz.so.1.2.6] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory `/export/home/palmieri/testing/clean/sage-5.1.beta3/spkg/build/zlib-1.2.6/src'
Error building zlib.

Any suggestions?

comment:21 Changed 10 years ago by jdemeyer

I don't have the same problem on hawk. Are you using /usr/local/bin/gcc, i.e. gcc 4.4.3 on hawk?

Compiling with CC=suncc should also work.

comment:22 Changed 10 years ago by jhpalmieri

I had some old environment variables set. Unsetting them seems to help.

Note: See TracTickets for help on using tickets.