Standard package zlib cannot be installed on some platforms, breaking Docker build

[saraedum]

Since 9.2.beta4 the docker build is failing:

[zlib-1.2.11.p0] Setting up build directory for zlib-1.2.11.p0
[zlib-1.2.11.p0] Finished extraction
[zlib-1.2.11.p0] Applying patches from ../patches...
[zlib-1.2.11.p0] Applying ../patches/cygwin-gzopen_w.patch
[zlib-1.2.11.p0] patching file gzguts.h
[zlib-1.2.11.p0] patching file win32/zlib.def
[zlib-1.2.11.p0] Hunk #1 FAILED at 91 (different line endings).
[zlib-1.2.11.p0] 1 out of 1 hunk FAILED -- saving rejects to file win32/zlib.def.rej
[zlib-1.2.11.p0] Error applying '../patches/cygwin-gzopen_w.patch'

See https://gitlab.com/sagemath/sage/-/pipelines for details. To see full logs, click on a build step; on the next page, browse the build artifacts on the right to see full output.

CC: @embray @tobiasdiez @dimpase @slel

Component: docker

Keywords: gitlab

Author: Dima Pasechnik

Branch/Commit: 77ab81f

Reviewer: Matthias Koeppe

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

[saraedum]

Changed keywords from none to gitlab

[mkoeppe]

comment:3

The patch has a mixture of LF and CRLF lineendings, to patch the file win32/zlib.def.rej, which has CRLF lineendings.

.gitattributes was added in #29733 and may have caused something to break here.

[vbraun]

comment:5

Surely thats a problem with how the docker image is built?

[saraedum]

comment:6

I think it means that you cannot build from a fresh git clone if you do not have zlib installed.

[saraedum]

comment:7

vbraun: What's the meaning of "blocker"? Us not being able to ship for one of the platforms sounds like a blocker to me.

[mkoeppe]

comment:8

I think it's a blocker because a standard package is broken on a supported platform

[dimpase]

comment:10

why is that a problem to fix this? I'll post a branch with uniform endings

[dimpase]

comment:11

it's actually "fun". The patch touches 2 files, one is OK, the other is with Windows (CRLF) line endings, and git version 2.20.1 on Linux produces a diff with mixed line endings!!!

$ git diff win32/zlib.def >/tmp/p2
dimpase@penguin:~/sage/upstream/zlib-1.2.11$ file /tmp/p2
/tmp/p2: unified diff output, ASCII text, with CRLF, LF line terminators

a bug in git? or maybe there is an option to use here...

[mkoeppe]

comment:12

I think something like -text to .gitattributes for this file could help

[dimpase]

comment:13

the problem is that applying a patch is a problem, nothing to do with git.
(unless I add --binary option)

$ cat /tmp/pp
diff --git a/win32/zlib.def b/win32/zlib.def
index 784b138..b69fa3e 100644
--- a/win32/zlib.def
+++ b/win32/zlib.def
@@ -91,4 +91,3 @@ EXPORTS
     inflateCodesUsed
     inflateResetKeep
     deflateResetKeep
-    gzopen_w
$ file /tmp/pp
/tmp/pp: unified diff output, ASCII text, with CRLF line terminators
$ patch --dry-run -p1 --posix </tmp/pp
(Stripping trailing CRs from patch; use --binary to disable.)
checking file win32/zlib.def
Hunk #1 FAILED at 91 (different line endings).
1 out of 1 hunk FAILED
$ patch --dry-run -p1 --posix --binary </tmp/pp # this works
checking file win32/zlib.def
$ dos2unix /tmp/pp
dos2unix: converting file /tmp/pp to Unix format...
dimpase@penguin:~/sage/upstream/zlib-1.2.11$ patch --dry-run -p1 --posix  </tmp/pp
checking file win32/zlib.def
Hunk #1 FAILED at 91 (different line endings).
1 out of 1 hunk FAILED

[dimpase]

Commit: bfac27c

[dimpase]

Branch: public/packages/zlibpatchfix

[dimpase]

Author: Dima Pasechnik

[dimpase]

New commits:

bfac27c

apply windows patch separately

[dimpase]

comment:15

This didn't work as needed, as cygwin-windef.diff got different line endings as I was checking it into the git repo... Arrgh...

[sagetrac-git]

Changed commit from bfac27c to 4eb6c8f

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

4eb6c8f

endings hopefully OK now

[dimpase]

comment:17

hmm, no, while locally the endings are OK, pulling from the repo mixes them up :-(

[dimpase]

comment:18

I will try to declare that diff file as binary in .gitattributes

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

6373ca4

setting patch type *.diff_bin to binary

[sagetrac-git]

Changed commit from 4eb6c8f to 6373ca4

[sagetrac-git]

Changed commit from 6373ca4 to 77ab81f

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

77ab81f

apply windows patch separately

[dimpase]

comment:21

OK, try this please (only tested on Linux)

[vbraun]

comment:22

When applying patches with --ignore-whitespace this should work automatically, but only if the diff has LF endings (patch is biased towards unix endings)

[dimpase]

comment:23

if this is so them I suppose we can just add this option to the patch call in
build/bin/sage-apply-patches and done with?

[dimpase]

comment:24

Replying to @vbraun:

When applying patches with --ignore-whitespace this should work automatically, but only if the diff has LF endings (patch is biased towards unix endings)

no, it does not work - on Debian 10 with its patch I get different line endings error.
So the current solution is more or less the only way.

I suppose we might not want to apply all our patches with --binary.

But we can also modify build/bin/sage-apply-patches to deal with binary patches (say, files with a particular suffix) by treating them as patches that must be applied with --binary.

[vbraun]

comment:25

Did you convert the patch to LF endings only? because right now its mixed, and --ignore-whitespace only works with pure LF endings:

$ file build/pkgs/zlib/patches/cygwin-gzopen_w.patch
build/pkgs/zlib/patches/cygwin-gzopen_w.patch: unified diff output, ASCII text, with CRLF, LF line terminators

[dimpase]

comment:26

Replying to @vbraun:

Did you convert the patch to LF endings only? because right now its mixed, and --ignore-whitespace only works with pure LF endings:

certainly, I tried it on the converted to the LF-endings patch build/pkgs/zlib/patches/cygwin-windef.diff_bin in the branch.

It's seriosly platform-dependent. As you might know, e.g. on macOS the original patch just works.
I won't be surprised if it works on some Linuxes, too.

[dimpase]

comment:27

Perhaps you are confused by --ignore-whitespace option of git apply, which indeed allows to ignore different EOLs. This is not the case with patch.

[mkoeppe]

comment:28

I guess someone should push this to gitlab or something to trigger the Docker build?

[saraedum]

comment:30

Since our runner has been down for ten days nothing gets built on GitLab anymore currently: https://zulip.sagemath.org/#narrow/stream/117-docker/topic/GitLab.20runners.20are.20gone

Otherwise, it would have been that one already https://gitlab.com/sagemath/dev/trac/-/jobs/728299492

[mkoeppe]

Reviewer: Matthias Koeppe

[mkoeppe]

comment:31

Tested at https://github.com/mkoeppe/sage/actions/runs/250810929 that this change does not break any of our platforms

[vbraun]

Changed branch from public/packages/zlibpatchfix to 77ab81f