Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade zlib to 1.2.6 #12800

Closed
jdemeyer opened this issue Apr 3, 2012 · 26 comments
Closed

Upgrade zlib to 1.2.6 #12800

jdemeyer opened this issue Apr 3, 2012 · 26 comments

Comments

@jdemeyer
Copy link

jdemeyer commented Apr 3, 2012

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

CC: @orlitzky

Component: packages: standard

Author: Jeroen Demeyer

Reviewer: Michael Orlitzky, Julien Puydt, Leif Leonhardy

Merged: sage-5.0.beta14

Issue created by migration from https://trac.sagemath.org/ticket/12800

@jdemeyer jdemeyer added this to the sage-5.0 milestone Apr 3, 2012
@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

jdemeyer commented Apr 3, 2012

Author: Jeroen Demeyer

@orlitzky
Copy link
Contributor

comment:2

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.

@orlitzky
Copy link
Contributor

comment:3

Alternately, fix the patch routine =)

@jdemeyer
Copy link
Author

comment:4

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

@SnarkBoojum
Copy link
Mannequin

SnarkBoojum mannequin commented Apr 11, 2012

comment:5

The spkg compiled fine with sage-4.8 on ARM.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Apr 11, 2012

Attachment: zlib-1.2.6.reviewer-ll.patch.gz

Reviewer patch. Apply to Jeroen's spkg.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Apr 11, 2012

Reviewer: Michael Orlitzky, Julien Puydt, Leif Leonhardy

@nexttime
Copy link
Mannequin

nexttime mannequin commented Apr 11, 2012

comment:6

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).

@nexttime nexttime mannequin added s: needs work and removed s: needs review labels Apr 11, 2012
@nexttime
Copy link
Mannequin

nexttime mannequin commented Apr 11, 2012

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.)

@jdemeyer
Copy link
Author

Attachment: zlib-1.2.6.diff.gz

Diff for the zlib spkg. For review only

@jdemeyer
Copy link
Author

comment:8

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

@SnarkBoojum
Copy link
Mannequin

SnarkBoojum mannequin commented Apr 11, 2012

comment:9

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...

@jdemeyer
Copy link
Author

comment:10

Replying to @SnarkBoojum:

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.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Apr 11, 2012

comment:11

Replying to @jdemeyer:

Replying to @SnarkBoojum:

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
    ...

@nexttime
Copy link
Mannequin

nexttime mannequin commented Apr 11, 2012

comment:12

P.S.:

$ ls patches/
*.patch

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

@nexttime
Copy link
Mannequin

nexttime mannequin commented Apr 11, 2012

comment:13

Replying to @nexttime:

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.

@jdemeyer
Copy link
Author

comment:14

Replying to @nexttime:

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).

@nexttime
Copy link
Mannequin

nexttime mannequin commented Apr 11, 2012

comment:15

Replying to @jdemeyer:

Replying to @nexttime:

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 ... ].

@SnarkBoojum
Copy link
Mannequin

SnarkBoojum mannequin commented Apr 11, 2012

comment:16

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.

@orlitzky
Copy link
Contributor

comment:17

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.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Apr 11, 2012

comment:18

Replying to @orlitzky:

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.

@jdemeyer
Copy link
Author

Merged: sage-5.0.beta14

@jhpalmieri
Copy link
Member

comment:20

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?

@jdemeyer
Copy link
Author

comment:21

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.

@jhpalmieri
Copy link
Member

comment:22

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants