GEOS-ESM/GEOSgcm_GridComp

Fix needed in zip code in make_bcs: How to test?

mathomp4 opened this issue · 2 comments

Tests of GEOS built using Spack instead of Baselibs found an build issue in make_bcs. Namely, compilation of the zip.c file failed saying, essentially, "you need to include stdlib.h".

The solution for that was simple, enable the STDC ifdef:

#include <stdio.h>
#include "zlib.h"
#ifdef STDC
# include <string.h>
# include <stdlib.h>
#endif

and it built.

So, I can propose a fix, but what I need to know is: how would one test this? I am quite unfamiliar with the make_bcs code. From my reading it looks like if you enable a flag, you can get out compressed rasters?


Note that one possible proposed fix I have is to just enable this for non-Baselibs builds in CMake:

# MAT NOTE This should use find_package(ZLIB) but Baselibs currently
#     confuses find_package(). This is a hack until Baselibs is
#     reorganized.
if (Baselibs_FOUND)
  set (INC_ZLIB ${BASEDIR}/include/zlib)
  target_include_directories(${this} PRIVATE ${INC_ZLIB})
else ()
  find_package(ZLIB)
  target_link_libraries(${this} PRIVATE ZLIB::zlib)
endif ()

So in that else I'd add:

target_compile_definitions(${this} PRIVATE STDC)

Since no one but me (and, well, @climbfuji) are trying out non-Baselibs builds of GEOS, this would affect only us. But my fear is eventually even Baselibs build will hit this issue.

@mathomp4, Thanks for the report. I don't have a clue why make_bcs needs zip.c, but that doesn't necessarily mean it's useless. (It just says that I'm useless...) In any case, @biljanaorescanin or @weiyuan-jiang could look into the need for zip.c. Perhaps we don't need it, after all, or there's a workaround using a zip utility that's present on the system (and doesn't need to be built with GEOS-ESM).

how would one test this?

@biljanaorescanin should be able to run make_bcs from a Spack build (with your fix) and verify that nothing in make_bcs broke.

But perhaps we should first establish that we need to build zip.c in the first place.

For now, I'll push up a PR with the safe fix for the "non-Baselibs" build. That trivially doesn't affect any current make_bcs users as no one but myself are building that way!