#30403 closed defect (fixed)

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

Reported by: saraedum Owned by:
Priority: blocker Milestone: sage-9.2
Component: docker Keywords: gitlab, docker
Cc: embray, gh-tobiasdiez, dimpase, slelievre Merged in:
Authors: Dima Pasechnik Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 77ab81f (Commits, GitHub, GitLab) Commit: 77ab81fcdbf1398c56c72533fcd8a45d464eb058
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (32)

comment:1 Changed 12 months ago by saraedum

  • Keywords gitlab docker added

comment:2 Changed 12 months ago by tmonteil

  • Cc embray added

comment:3 Changed 11 months ago by mkoeppe

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.

comment:4 Changed 11 months ago by saraedum

  • Cc gh-tobiasdiez added

comment:5 Changed 11 months ago by vbraun

  • Priority changed from blocker to critical

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

comment:6 Changed 11 months ago by saraedum

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

comment:7 Changed 11 months ago by saraedum

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

comment:8 Changed 11 months ago by mkoeppe

  • Priority changed from critical to blocker
  • Summary changed from Docker build is broken to Standard package zlib cannot be installed on some platforms, breaking Docker build

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

comment:9 Changed 11 months ago by mkoeppe

  • Cc dimpase added

comment:10 Changed 11 months ago by dimpase

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

comment:11 Changed 11 months ago by dimpase

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

comment:12 Changed 11 months ago by mkoeppe

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

comment:13 Changed 11 months ago by dimpase

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

comment:14 Changed 11 months ago by dimpase

  • Authors set to Dima Pasechnik
  • Branch set to public/packages/zlibpatchfix
  • Commit set to bfac27ca0ca2d00704dd190f7f720828765ee70a

New commits:

bfac27capply windows patch separately

comment:15 Changed 11 months ago by dimpase

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

comment:16 Changed 11 months ago by git

  • Commit changed from bfac27ca0ca2d00704dd190f7f720828765ee70a to 4eb6c8f364f24645c3365ec20e22fd3b50aff0a8

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

4eb6c8fendings hopefully OK now

comment:17 Changed 11 months ago by dimpase

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

comment:18 Changed 11 months ago by dimpase

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

comment:19 Changed 11 months ago by git

  • Commit changed from 4eb6c8f364f24645c3365ec20e22fd3b50aff0a8 to 6373ca46e0d8565407777d202314a9b31dc9cd5c

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

6373ca4setting patch type *.diff_bin to binary

comment:20 Changed 11 months ago by git

  • Commit changed from 6373ca46e0d8565407777d202314a9b31dc9cd5c to 77ab81fcdbf1398c56c72533fcd8a45d464eb058

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

77ab81fapply windows patch separately

comment:21 Changed 11 months ago by dimpase

  • Status changed from new to needs_review

OK, try this please (only tested on Linux)

comment:22 follow-up: Changed 11 months ago by 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)

comment:23 Changed 11 months ago by dimpase

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?

comment:24 in reply to: ↑ 22 Changed 11 months ago by dimpase

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.

comment:25 follow-up: Changed 11 months ago by vbraun

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

comment:26 in reply to: ↑ 25 Changed 11 months ago by dimpase

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.

comment:27 Changed 11 months ago by dimpase

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.

comment:28 Changed 11 months ago by mkoeppe

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

comment:29 Changed 11 months ago by mkoeppe

  • Cc slelievre added

comment:30 Changed 11 months ago by saraedum

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

comment:31 Changed 11 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

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

comment:32 Changed 11 months ago by vbraun

  • Branch changed from public/packages/zlibpatchfix to 77ab81fcdbf1398c56c72533fcd8a45d464eb058
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.