Skip to content

makes dependency on zlib optional#4844

Merged
mattdowle merged 13 commits intomasterfrom
nozlib
Jan 5, 2021
Merged

makes dependency on zlib optional#4844
mattdowle merged 13 commits intomasterfrom
nozlib

Conversation

@jangorecki
Copy link
Copy Markdown
Member

@jangorecki jangorecki commented Dec 12, 2020

Follow-up of #3872
It is NOT meant to solve Solaris issue, but general issue when zlib is not available.


before PR

docker run -it rocker/r-ver:3.5.3 Rscript -e 'install.packages("data.table", repos="https://Rdatatable.gitlab.io/data.table"); data.table::fwrite(iris, "iris.gz")'
*** pkg-config is not installed.
*** Compilation will now be attempted and if it works you can ignore this message. In
*** particular, this should be the case on Mac where zlib is built in.
*** However, if compilation fails, try 'locate zlib.h zconf.h' and ensure the zlib
*** development library is installed :
***   deb: zlib1g-dev (Debian, Ubuntu, ...)
***   rpm: zlib-devel (Fedora, EPEL, ...)
***   There is a zlib in brew for OSX but the built in zlib should work.
*** Note that zlib is required to compile R itself so you may find the advice in the R-admin
*** guide helpful regarding zlib. On Debian/Ubuntu, zlib1g-dev is a dependency of r-base as
*** shown by 'apt-cache showsrc r-base | grep ^Build-Depends | grep zlib', and therefore
*** 'sudo apt-get build-dep r-base' should be sufficient too.
*** To silence this message, please ensure that :
***   1) 'pkg-config --exists zlib' succeeds (i.e. $? -eq 0)
***   2) 'pkg-config --libs zlib' contains -lz
*** Compilation will now be attempted ...
...
gcc -I"/usr/local/lib/R/include" -DNDEBUG   -I/usr/local/include  -fopenmp -fpic  -g -O2 -fstack-protector-strong -Wformat -Werror=f
ormat-security -Wdate-time -D_FORTIFY_SOURCE=2 -g  -c fwrite.c -o fwrite.o
fwrite.c:10:49: fatal error: zlib.h: No such file or directory
 #include <zlib.h>      // for compression to .gz

after PR

docker run -it rocker/r-ver:3.5.3 Rscript -e 'install.packages("remotes", repos="https://cloud.r-project.org"); remotes::install_github("Rdatatable/data.table@nozlib"); data.table::fwrite(iris, "iris.gz")'
*** pkg-config is not installed.
*** Compilation will now be attempted and if it works you can ignore this message. In
*** particular, this should be the case on Mac where zlib is built in or pkg-config
*** is not installed. However, if compilation fails, try 'locate zlib.h zconf.h' and
*** ensure the zlib development library is installed :
***   deb: zlib1g-dev (Debian, Ubuntu, ...)
***   rpm: zlib-devel (Fedora, EPEL, ...)
***   There is a zlib in brew for OSX but the built in zlib should work.
*** Note that zlib is required to compile R itself so you may find the advice in the R-admin
*** guide helpful regarding zlib. On Debian/Ubuntu, zlib1g-dev is a dependency of r-base as
*** shown by 'apt-cache showsrc r-base | grep ^Build-Depends | grep zlib', and therefore
*** 'sudo apt-get build-dep r-base' should be sufficient too.
*** To silence this message, please ensure that :
***   1) 'pkg-config --exists zlib' succeeds (i.e. $? -eq 0)
***   2) 'pkg-config --libs zlib' contains -lz
*** Compilation will now be attempted ...
R CMD SHLIB supports OpenMP without any extra hint
*** Compilation without compression support in fwrite
...
Error in data.table::fwrite(iris, "iris.gz") : 
  Compression in fwrite uses zlib library which was not installed at the time when data.table was compiled. To enable compression support in fwrite one must install zlib and re-install data.table.

Comment thread configure
echo "*** particular, this should be the case on Mac where zlib is built in or pkg-config"
echo "*** is not installed. However, if compilation fails, try 'locate zlib.h zconf.h' and"
echo "*** ensure the zlib development library is installed :"
echo "*** deb: zlib1g-dev (Debian, Ubuntu, ...)"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

doesn't this turn off zlib support on mac by default

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If pkg-config is not available on Mac then yes, it turns off. Needs testing anyway.
There is a related comment above:

if pkg-config is not available then zlib will be disabled for higher chance of compilation success

Copy link
Copy Markdown
Member Author

@jangorecki jangorecki Dec 12, 2020

Choose a reason for hiding this comment

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

Note that this verbose message branch was hit before when pkg-config was not installed, just the message was not mentioning that.

Copy link
Copy Markdown
Member Author

@jangorecki jangorecki Dec 12, 2020

Choose a reason for hiding this comment

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

We could eventually try to check if zlib is available by attempting to compile dummy code, like we do for openmp. Docker image in your report didn't have neither pkg-config and zlib. So then we could still compile with zlib support. For now I don't think it is worth. Messages are clear and points users in the proper direction. Installing pkg-config and/or zlib.

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 12, 2020

Codecov Report

Merging #4844 (08a2584) into master (354cf84) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4844      +/-   ##
==========================================
- Coverage   99.47%   99.47%   -0.01%     
==========================================
  Files          73       73              
  Lines       14560    14559       -1     
==========================================
- Hits        14483    14482       -1     
  Misses         77       77              
Impacted Files Coverage Δ
src/utils.c 98.36% <ø> (ø)
src/fwrite.c 97.69% <100.00%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 354cf84...08a2584. Read the comment docs.

@jangorecki jangorecki added this to the 1.13.7 milestone Dec 12, 2020
@jangorecki
Copy link
Copy Markdown
Member Author

I put it on label High so we can have it merged at the beginning on a development cycle to be well tested before it will land on CRAN.

@mattdowle
Copy link
Copy Markdown
Member

mattdowle commented Jan 5, 2021

Changes I made in 08a2584:

  • zlib runtime is still a requirement for those installing the binary package from CRAN. So reinstated the SystemRequirements zlib line in DESCRIPTION. Not a problem because the zlib runtime is already required by R. Further, test.data.table() will fail, and therefore so will R CMD check, if data.table has not been compiled with zlib headers because tests of fwrite compression will fail. So leaving that line as-is seems more appropriate than removing that line.
  • #define z_stream struct line removed. I'm not sure that would result in valid syntax. I checked all uses of z_stream are inside #ifndef NOZLIB blocks which is stricter.
  • distinction between library and headers made in the messages
  • zlib headers may be installed but not found for some reason. So strictly speaking, using the words 'not found'.
  • the error message now tells the user that reinstalling will output further guidance
  • if (nth>1) verbose=false also removed (unrelated to this PR) as I neglected to do that in Remove tracing of zlib/fwrite for Solaris #4860. The comment on that line was clear that it was only there for the Solaris tracing.

@mattdowle mattdowle merged commit dae9652 into master Jan 5, 2021
@mattdowle mattdowle deleted the nozlib branch January 5, 2021 01:12
@mattdowle mattdowle modified the milestones: 1.13.7, 1.14.0 Feb 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants