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

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

Open
mathomp4 opened this issue Jan 25, 2024 · 2 comments
Open

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

mathomp4 opened this issue Jan 25, 2024 · 2 comments
Assignees
Labels
help wanted Extra attention is needed

Comments

@mathomp4
Copy link
Member

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.

@gmao-rreichle
Copy link
Contributor

@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.

@mathomp4
Copy link
Member Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants