Skip to content

fwrite zlib #3951

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.

Sign up for GitHub

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

Merged
mattdowle merged 9 commits into from
Oct 9, 2019
Merged

fwrite zlib #3951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
53f91a4
removed z_const
mattdowle Oct 8, 2019
cc4fa28
added src/configure, and changed "zlib.h" to <zlib.h>
mattdowle Oct 9, 2019
d1896a6
only search c files
MichaelChirico Oct 9, 2019
5c12ce6
const results in 'discards const' warning, so removed
mattdowle Oct 9, 2019
3a3b3b8
Merge branch 'fwrite_zlib' of https://github.com/Rdatatable/data.tabl…
mattdowle Oct 9, 2019
6e1a5be
exit not return for osx
mattdowle Oct 9, 2019
554587c
remove a space
mattdowle Oct 9, 2019
c627c14
search .c and .h files
mattdowle Oct 9, 2019
06f5953
news item
mattdowle Oct 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ environment:

- R_VERSION: release # the single Windows.zip binary (both 32bit/64bit) that users following dev version of installation instructions should click

# - R_VERSION: devel # temporarily off pending R-devel fix of slowdown from 5min to 20-30min (slowing down data.table dev) in July 2019 possibly due to r76734.
- R_VERSION: devel # temporarily off pending R-devel fix of slowdown from 5min to 20-30min (slowing down data.table dev) in July 2019 possibly due to r76734.

before_build:
- cmd: ECHO no Revision metadata added to DESCRIPTION
Expand Down
4 changes: 4 additions & 0 deletions .dev/CRAN_Release.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ grep "Rprintf" ./src/init.c
grep "nearest *=" ./src/*.c # none
grep "class *=" ./src/*.c # quite a few but none global

# ensure no use of z_const from zconf.h; #3939
grep "z_const" ./src/*.[hc] # none other than the comment

# No undefined type punning of the form: *(long long *)&REAL(column)[i]
# Failed clang 3.9.1 -O3 due to this, I think.
grep "&REAL" ./src/*.c
Expand Down Expand Up @@ -149,6 +152,7 @@ system.time(test.data.table(script="*.Rraw")) # apx 8h = froll 3h + nafill 1m +

# Upload to win-builder: release, dev & old-release
# Turn on Travis OSX; it's off in dev until it's added to GLCI (#3326) as it adds 17min after 11min Linux.
# Turn on r-devel in Appveyor; it may be off in dev for similar dev cycle speed reasons


###############################################
Expand Down
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

1. `shift()` on a `nanotime` with the default `fill=NA` now fills a `nanotime` missing value correctly, [#3945](https://github.com/Rdatatable/data.table/issues/3945). Thanks to @mschubmehl for reporting and fixing in PR [#3942](https://github.com/Rdatatable/data.table/pull/3942).

2. Compilation failed on CRAN's MacOS due to an older version of `zlib.h/zconf.h` which did not have `z_const` defined, [#3939](https://github.com/Rdatatable/data.table/issues/3939). Other open-source projects unrelated to R have experienced this problem on MacOS too. We have followed the common practice of removing `z_const` to support the older `zlib` versions, and data.table's release procedures have gained a `grep` to ensure `z_const` isn't used again by accident in future. The library `zlib` is used for `fwrite`'s new feature of multithreaded compression on-the-fly; see item 3 of 1.12.4 below.

## NOTES


Expand Down
32 changes: 32 additions & 0 deletions configure
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#!/bin/sh
# Let's keep this simple. If pkg-config is available, use it. Otherwise print
# the helpful message to aid user if compilation does fail. Note 25 of R-exts:
# "[pkg-config] is available on the machines used to produce the CRAN binary packages"

if ! hash pkg-config 2>/dev/null; then
echo "*** pkg-config is not installed. It is used to check that zlib is installed. Compilation"
echo "*** will now be attempted and if it works you can ignore this message. If it fails and"
echo "*** and you cannot install pkg-config, try: locate zlib.h zconf.h. If they are not found"
echo "*** then try installing zlib1g-dev (Debian/Ubuntu), zlib-devel (Fedora) or zlib (OSX)"
exit 0 # now that the advice has been printed, exit with success and continue
fi

if ! `pkg-config --exists zlib`; then
echo "pkg-config did not detect zlib is installed. Try installing:"
echo "* deb: zlib1g-dev (Debian, Ubuntu, ...)"
echo "* rpm: zlib-devel (Fedora, EPEL, ...)"
echo "* brew: zlib (OSX)"
exit 1 # nothing more to do; zlib is required currently
fi

version=`pkg-config --modversion zlib`
echo "zlib ${version} is available ok"

lib=`pkg-config --libs zlib`
if test $lib != "-lz"; then
echo "zlib is linked using ${lib} not the expected standard -lz. Please report to data.table issue tracker on GitHub."
exit 1
fi

exit 0

6 changes: 3 additions & 3 deletions src/fwrite.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#include "fwriteLookups.h"
#include <errno.h>
#include <unistd.h> // for access()
#include <fcntl.h>
Expand All @@ -7,6 +6,7 @@
#include <math.h> // isfinite, isnan
#include <stdlib.h> // abs
#include <string.h> // strlen, strerror
#include <zlib.h> // for compression to .gz

#ifdef WIN32
#include <sys/types.h>
Expand All @@ -19,8 +19,8 @@
#define CLOSE close
#endif

#include "zlib.h" // for writing gzip file
#include "myomp.h"
#include "fwriteLookups.h"
#include "fwrite.h"

#define NUM_SF 15
Expand Down Expand Up @@ -566,7 +566,7 @@ int compressbuff(z_stream *stream, void* dest, size_t *destLen, const void* sour

stream->next_out = dest;
stream->avail_out = *destLen;
stream->next_in = (z_const Bytef *)source;
stream->next_in = (Bytef *)source; // don't use z_const anywhere; #3939
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico Oct 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was wrong with const exactly?

I see this in my zlib.h:

/usr/include/zconf.h:234:#if defined(ZLIB_CONST) && !defined(z_const)
/usr/include/zconf.h:235:#  define z_const const
/usr/include/zconf.h-236-#else
/usr/include/zconf.h:237:#  define z_const
/usr/include/zconf.h-238-#endif

would it make sense to simply imitate this logic like

#ifndef zconst
#define z_const
#endif

?

Copy link
Copy Markdown
Member Author

@mattdowle mattdowle Oct 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put the error message I got in the comment at the top of this issue.
I don't understand what z_const is for. If we put those 3 lines back in, could you think of a comment to put alongside to explain why if z_const is defined, it should be used? And why const can't be used? master as of now is simple: don't use z_const.

Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico Oct 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed the comment, I had only seen the commit message.

I don't really know any details, mine was more a question of curiosity... perhaps @philippechataignon could comment based on motivation for using z_const in the first place?

stream->avail_in = sourceLen;

err = deflate(stream, Z_FINISH);
Expand Down