wrf-model / WRF

The official repository for the Weather Research and Forecasting (WRF) model
Other
1.24k stars 677 forks source link

Update g2lib png encode/decode to use correct png_voidp define #1977

Closed islas closed 8 months ago

islas commented 9 months ago

TYPE: bug fix

KEYWORDS: png

SOURCE: internal

DESCRIPTION OF CHANGES: Problem:

Incorrect define type used for png void type when compared to libpng functions and NCEP g2lib source code.

While voidp does exist, compile may fail when enabling GRIB2 I/O and enabling libpng compression in the makefiles. Additionally no version of libpng (back to 0.89) ever had those functions explicitly compatibly with voidp - likewise the ncep g2 lib version of these files (back to v2.5.0) never has voidp in the modified code.

This will NOT affect out-of-the-box native builds of WRF, grib2 enabled or not. One must enable the source code manually to exercise PNG compression and then this issue may appear.

Solution: Switch to the appropriate define png_voidp, as can be seen here : https://github.com/weathersource/g2clib/blob/24bcccafb8088f4b5918a878e6b8daf7acc86275/src/dec_png.c#L91 https://github.com/weathersource/g2clib/blob/24bcccafb8088f4b5918a878e6b8daf7acc86275/src/enc_png.c#L91

or here: https://github.com/NOAA-EMC/NCEPLIBS-g2/blob/1dcdfb0d8db0caf2f31b5631abcf1ca04402a0aa/dec_png.c#L104 https://github.com/NOAA-EMC/NCEPLIBS-g2/blob/1dcdfb0d8db0caf2f31b5631abcf1ca04402a0aa/enc_png.c#L103

Note that NCEP's github version does not goes as far back as version 1.0.7 which WRF uses

LIST OF MODIFIED FILES: M external/io_grib2/g2lib/dec_png.c M external/io_grib2/g2lib/enc_png.c

RELEASE NOTE: Update g2lib png encode/decode to use correct png_voidp define

weiwangncar commented 9 months ago

The regression test results:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None
mgduda commented 8 months ago

I'm curious as to how you encountered the issue with the wrong typecast?

Regarding the remark in your commit message, I would guess that png_voidp and voidp are both typdefs of void *. For what it's worth, if I manually compile the enc_png.c file with the gcc 12.2.0 compilers, even with -Wall, there are no warnings generated with the original code. But that probably depends on the libpng and zlib library versions that I'm using.

Anyway, I do agree that we should fix the typecast to agree with the argument type; it's just a question of whether we should add more detail in the PR description (especially if the enc_png.c and dec_png.c files aren't even compiled under the current default WRF build).

islas commented 8 months ago

I found out this file was wrong just because I was trying to compile everything in the cmake build to just cover as much ground as possible. I searched libpng source and symbols in libraries, and while voidp does exist, it failed to compile for me (can't remember all the compilers I was trying at the time but this was on cheyenne) and no version of libpng (back to 0.89) ever had those functions explicitly compatibly with voidp - likewise the ncep g2 lib version of these files (back to v2.5.0) never has voidp in there as well - hence my utter confusion.

It could be that I was not using certain flags like automatic implicit type casting