Skip to content
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.

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

fwrite zlib #3951

Merged
merged 9 commits into from
Oct 9, 2019
Merged

fwrite zlib #3951

merged 9 commits into from
Oct 9, 2019

Conversation

mattdowle
Copy link
Member

@mattdowle mattdowle commented Oct 9, 2019

Closes #3939
Closes #3872

The configure scripts in other R packages seem to be copies of the Autoconf program, at about 5,000 lines long. Plus an associated short configure.ac I just can't wrap my head around. It's just so complicated with those layers.
So let's just try a short simple script. The comment at the top starts "let's keep this simple". If this works then to pass a flag through to the code could be a next step. But honesty, zlib is so standard, that I don't think we should invest effort and complexity in supporting no-zlib. If and when users start to complain they don't have zlib for some reason, then we can reconsider at that point.

Also removed z_const so it should work on MacOS now. I kept it as const rather than removing it because it seems safer to protect that input with const. If any of the C++ compilation problems arise that the zlib developers referred to then we can reconsider at that point. Keeping it as const resulting in this warning so I removed the const just as @philippechataignon thought in the first place :

gcc -std=gnu99 -I"/usr/share/R/include" -DNDEBUG    -fopenmp -fpic  -O0 -g -c fwrite.c -o fwrite.o
fwrite.c: In function ‘compressbuff’:
fwrite.c:569:19: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   stream->next_in = (const Bytef *)source; // don't use z_const anywhere; #3939

There was a discussion about gcc version in #3291. If that's true and we do need a certain version or later, then it could be checked in this configure script, with helpful advice output before the impending compile error.

@mattdowle mattdowle added this to the 1.12.5 milestone Oct 9, 2019
@codecov
Copy link

codecov bot commented Oct 9, 2019

Codecov Report

Merging #3951 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3951   +/-   ##
======================================
  Coverage    99.4%   99.4%           
======================================
  Files          72      72           
  Lines       13642   13642           
======================================
  Hits        13561   13561           
  Misses         81      81
Impacted Files Coverage Δ
src/fwrite.c 97.83% <100%> (ø) ⬆️

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 520a635...06f5953. Read the comment docs.

@mattdowle mattdowle changed the title Fwrite zlib fwrite zlib Oct 9, 2019
@mattdowle mattdowle merged commit 4b5c3d3 into master Oct 9, 2019
@mattdowle mattdowle deleted the fwrite_zlib branch October 9, 2019 05:46
@@ -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
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.

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
Member Author

@mattdowle mattdowle Oct 9, 2019

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
Member

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?

@jangorecki
Copy link
Member

jangorecki commented Oct 9, 2019

Nice PR.
Yes I agree we could lets first see if it actually hits users before we will try to add much more complexity to handle that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.12.4 failed CRAN MacOS in fwrite.c zlib Handling of zlib fwrite dependency
3 participants