Gzip support, take 2 #2509

ChrisJefferson · 2018-06-04T15:36:43Z

This is a cleaned up attempt at adding gzip support to GAP.

I recommend reading this commit by commit. The changes are:

First two commits: PR #2508, to disable a warning in the new code

3rd commit: Add zlib to configure (note, this should / will be improved).

4th/5th commit: This abstracts out most uses of read and write to use a SyRead and SyWrite function. At this point this seems fairly useless, but it will make adding gzip support a much smaller change.

6th commit: Actually add gzip

7th commit: Testing

ChrisJefferson · 2018-06-04T15:44:46Z

The 'DO NOT MERGE' is because I'm not happy with how I find zlib (that is, the user must have it installed, or else GAP won't build).

fingolfin · 2018-06-04T22:54:47Z

Hmm, tests are failing (and issue labels are missing). Will wait with reviewing until this is resolved.

codecov · 2018-06-05T09:03:39Z

Codecov Report

Merging #2509 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2509      +/-   ##
==========================================
- Coverage   74.48%   74.48%   -0.01%     
==========================================
  Files         481      481              
  Lines      242733   242764      +31     
==========================================
+ Hits       180808   180824      +16     
- Misses      61925    61940      +15

Impacted Files	Coverage Δ
src/saveload.c	`65.78% <ø> (+0.34%)`	⬆️
src/sysfiles.c	`39.21% <ø> (-0.19%)`	⬇️
src/streams.c	`77.08% <ø> (-0.2%)`	⬇️
src/system.c	`74.37% <ø> (+0.28%)`	⬆️
lib/streams.gd	`87.5% <ø> (ø)`	⬆️
src/hpc/threadapi.c	`43.59% <0%> (+0.1%)`	⬆️
... and 2 more

fingolfin · 2018-06-05T15:46:41Z

Travis now fails, but AppVeyor still fails, because of missing zlib headers.

This of course would be fixed if zlib was bundled.

ChrisJefferson · 2018-06-08T12:47:24Z

Some changes:

I have forced the built-in zlib to always use the static build. This is because zlib isn't libtool-aware, so while GAP builds and links, when GAP is loaded the extern/build/zlib directory isn't in the library path and GAP either goes off and uses /lib/zlib1.so (which could be bad), or just fails to load.
At the moment tests are failing because some packages (for example cvec) aren't picking up the zlib header location. However, the most recent round of configure changes seems to have fixed this (it works on cvec 2.6.1, released yesterday), so things should look much better in a couple of days.

fingolfin · 2018-06-12T22:34:57Z

configure.ac

+  ])
+  BUILD_ZLIB=yes
+  ZLIB_CPPFLAGS='-I${abs_builddir}/extern/install/zlib/include'
+  ZLIB_LDFLAGS='-L${abs_builddir}/extern/install/zlib/lib -l:libz.a'


-l:libz.a is not portable (in fact, I could find no documentation on it at all). But it's also not needed here: just change this to:

ZLIB_LDFLAGS='${abs_builddir}/extern/install/zlib/lib/libz.a'

https://sourceware.org/binutils/docs/ld/Options.html gives the documentation for -l. I've used it previously as i don't like compiler's habits of "magically" choosing which file they want (dynamic or static), but I don't know what's "standard" linker behaviour, as opposed to GNU behaviour. I'm happy to change it.

fingolfin · 2018-06-12T22:36:28Z

configure.ac

+
+# Use bundled zlib if requested
+AS_IF([test x$BUILD_ZLIB = xyes],[
+  AS_IF([test x$CYGWIN = xyes],[


It seems a bit overkill to move the a huge section of configure.ac just to check for cygwin here, esp. as that only requires matching $host_os against *cygwin*...

How do I do substring matching in configure scripts? Google isn't turning up anything useful. In shell I'd normally do [[ ... ]], but configure doesn't seem to like that.

I eventually managed to track down you are supposed to use AC_CASE of course....

fingolfin · 2018-06-12T22:39:05Z

This still has a "DO NOT MERGE" label, and I am not sure what it's status is, so I'll refrain from a full review.

Just one more comment: GitHub's UI shows that commits ordered by date, which here does not fit their actual order. Perhaps consider doing a git rebase --ignore-date master next time you update it?

ChrisJefferson · 2018-06-14T00:11:55Z

Now updated (hopefully)

fingolfin · 2018-06-14T14:06:17Z

src/sysfiles.c

@@ -84,15 +84,27 @@ typedef void sig_handler_t ( int );
 #include <mach-o/dyld.h>
 #endif

+#include <zlib.h>
+

 /* utility to check return value of 'write'  */
 ssize_t echoandcheck(int fid, const char *buf, size_t count) {


(I am not asking you to actually change this, but: wouldn't this have been one of the few cases were one really could have fixed the formatting of the whole function, given that you touch all of it except the first two and last two lines?)

fingolfin · 2018-06-14T14:06:53Z

src/sysfiles.c

-        syBuf[fid].isTTY = 0;
+       syBuf[fid].type = pipe_socket;
+       syBuf[fid].fp = fileno(syBuf[fid].pipehandle);
+       syBuf[fid].bufno = -1;


Weirdly, the indentation in this case is wrong by one space (but again, I am not really asking you to change this)

fingolfin · 2018-06-14T14:08:13Z

src/sysfiles.c

+      ret = gzwrite(syBuf[fid].gzfp, buf, count);
+      if (ret < 0)
+          ErrorQuit(
+              "Cannot write to compressed file, see 'LastSystemError();'\n",


Language nitpick: If I saw that message, I would think "ah, writing to compressed files is not supported"; but I think what really is meant here is that writing to a compressed file failed. Perhaps "Could not write" or "Failed to write" would be clearer (or maybe that's me as a non-native speaker just not getting it)?

(Again, not necessarily asking you to change this, just wondering)

fingolfin · 2018-06-14T14:08:26Z

src/sysfiles.c

+  else {
+      ret = write(syBuf[fid].echo, buf, count);
+      if (ret < 0)
+          ErrorQuit("Cannot write to file descriptor %d, see "


Same as above (but of course this message is old and predates this PR)

fingolfin · 2018-06-14T14:09:44Z

src/sysfiles.h

-  UInt        ateof;       /* set to 1 by any read operation that hits eof
+    /* gzfp is used if type == gzip_socket
+    ** Stored as a void* to avoid including zlib.h */
+    void* gzfp;


Formatting: space before * (but we can also fix this afterwards -- indeed, with your PR merged, I think we are close to making syBuf private to sysfiles.h, then we could move this out of the header and into the .c file, and take care of formatting in this struct overall)

Yes, we could try moving this into sysfiles.h later. I did at some point run clang-format on this part, but it totally changed the whole struct and I wanted it to be clear what I'd added/changed.

ChrisJefferson · 2018-06-14T14:16:20Z

@fingolfin , while many of reformatting comments are reasonable, I'd prefer to merge this and then make cleanups later, partly because it will be a pain to figure out which changes have to go in which of the existing commits.

fingolfin · 2018-06-15T10:53:37Z

I'd merge this now but there was a build failure, seemingly due to a network problem. I just restarted that job, if it passes, we can merge.

ChrisJefferson force-pushed the gzip branch from 02aaaf8 to 3c4374b Compare June 4, 2018 15:43

ChrisJefferson added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jun 4, 2018

ChrisJefferson force-pushed the gzip branch 2 times, most recently from cdce5fc to bdec59a Compare June 5, 2018 10:08

fingolfin mentioned this pull request Jun 5, 2018

Add zlib to extern (DO NOT MERGE) #2516

Closed

ChrisJefferson force-pushed the gzip branch 4 times, most recently from 1b3d92e to 477db3e Compare June 8, 2018 09:34

olexandr-konovalov added the gapdays2018-spring Issues and PRs that arose at http://www.gapdays.de/gap-jupyter-days2018 label Jun 8, 2018

ChrisJefferson force-pushed the gzip branch 2 times, most recently from 598a741 to 91df31d Compare June 11, 2018 17:25

fingolfin reviewed Jun 12, 2018

View reviewed changes

ChrisJefferson force-pushed the gzip branch from 91df31d to 27fc0e7 Compare June 13, 2018 23:13

ChrisJefferson added topic: kernel and removed do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) labels Jun 13, 2018

ChrisJefferson force-pushed the gzip branch 4 times, most recently from 3318e13 to 9f41940 Compare June 14, 2018 00:11

ChrisJefferson force-pushed the gzip branch from 9f41940 to 43992af Compare June 14, 2018 13:55

extern: import zlib 1.2.11

2035447

fingolfin and others added 5 commits June 14, 2018 14:57

extern: workaround zlib issue on cygwin

b4c5565

Install zlib-devel on appveyor

79ca346

buildsys: check for and/or build zlib

a49b781

Add initial SyRead/SyWrite/SyWriteandcheck

130abf7

Use SyRead/SyWrite

5fd8749

ChrisJefferson force-pushed the gzip branch from 43992af to f8e0b5a Compare June 14, 2018 13:58

fingolfin reviewed Jun 14, 2018

View reviewed changes

ChrisJefferson and others added 3 commits June 14, 2018 15:45

Add gzip support

dd5948d

Update gzip tests to support seeking

df30f87

Update InputTextFile and OutputTextFile documentation for compression

12610b3

ChrisJefferson force-pushed the gzip branch from f8e0b5a to 12610b3 Compare June 14, 2018 14:45

fingolfin approved these changes Jun 15, 2018

View reviewed changes

fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Jun 15, 2018

fingolfin merged commit 809347d into gap-system:master Jun 15, 2018

This was referenced Jun 28, 2018

Add built-in support for reading (and writing?) gzip'ed files #916

Closed

RewindStream fails for compressed files #999

Closed

ChrisJefferson deleted the gzip branch July 9, 2018 14:22

fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Sep 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gzip support, take 2 #2509

Gzip support, take 2 #2509

ChrisJefferson commented Jun 4, 2018

ChrisJefferson commented Jun 4, 2018

fingolfin commented Jun 4, 2018

codecov bot commented Jun 5, 2018 •

edited

Loading

fingolfin commented Jun 5, 2018

ChrisJefferson commented Jun 8, 2018

fingolfin Jun 12, 2018

ChrisJefferson Jun 13, 2018

ChrisJefferson Jun 14, 2018

fingolfin Jun 12, 2018

ChrisJefferson Jun 13, 2018 •

edited

Loading

ChrisJefferson Jun 13, 2018

ChrisJefferson Jun 14, 2018

fingolfin commented Jun 12, 2018

ChrisJefferson commented Jun 14, 2018

fingolfin Jun 14, 2018

fingolfin Jun 14, 2018

fingolfin Jun 14, 2018

fingolfin Jun 14, 2018

fingolfin Jun 14, 2018

ChrisJefferson Jun 14, 2018

ChrisJefferson commented Jun 14, 2018

fingolfin commented Jun 15, 2018

Gzip support, take 2 #2509

Gzip support, take 2 #2509

Conversation

ChrisJefferson commented Jun 4, 2018

ChrisJefferson commented Jun 4, 2018

fingolfin commented Jun 4, 2018

codecov bot commented Jun 5, 2018 • edited Loading

Codecov Report

fingolfin commented Jun 5, 2018

ChrisJefferson commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisJefferson Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Jun 12, 2018

ChrisJefferson commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisJefferson commented Jun 14, 2018

fingolfin commented Jun 15, 2018

codecov bot commented Jun 5, 2018 •

edited

Loading

ChrisJefferson Jun 13, 2018 •

edited

Loading