Replace yajl with vendored yyjson for JSON parsing - #134
Open
mattjala wants to merge 5 commits into
Open
Conversation
The REST VOL depended on the unmaintained lloyd/yajl library to parse HSDS's JSON responses. Its ancient CMakeLists (cmake_minimum_required 2.6) fails under CMake 4.x, which broke the Windows CI job that builds yajl from source. This PR entirely replaces lloyd/yajl with the actively-maintained yyjson library. More specifically: - Add JSON helpers in rest_vol.c/.h using yyjson: RV_json_parse, RV_json_get, RV_json_obj_key_at/RV_json_obj_val_at, and RV_json_get_*/RV_json_is_* accessors. - Port all yajl usage to the new routines. Build changes: - CMake: drop find_package(YAJL) and the yajl link, add yyjson.c to sources, remove the unused CMake/FindYAJL.cmake, its install rule, and the HDF5_VOL_REST_RESOURCES_MOD_DIR variable. - Autotools: add yyjson.c to src/Makefile.am, remove -lyajl, the YAJL install-dir flags, the libyajl check and --with-yajl from configure.ac.
Collaborator
There was a problem hiding this comment.
Just as a first comment, we should avoid vendoring any dependencies where we can since it will just result in maintenance burden. yyjson appears to ship a CMake .config file, so a simple find_package() call should be enough to setup what we need.
Collaborator
Author
There was a problem hiding this comment.
Sure. Are we still concerned with supporting autotools builds for the REST VOL, or can we drop it since HDF5 has?
The connector supported both CMake and Autotools builds, but the Autotools path was Linux-only and effectively unused. Drop it in favor of CMake as the single build system.
Replace the vendored copy of yyjson with CMake-based acquisition. Build uses an installed yyjson if one is found, otherwise FetchContent downloads and builds a pinned version (0.12.0).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The REST VOL depended on the unmaintained lloyd/yajl library to parse HSDS's JSON responses. Its ancient CMakeLists (cmake_minimum_required 2.6) fails under CMake 4.x, which broke the Windows CI job that builds yajl from source.
This PR entirely replaces lloyd/yajl with the actively-maintained yyjson library. More specifically:
Some associated build changes: