- 
                Notifications
    You must be signed in to change notification settings 
- Fork 182
Move yyjson dependency to PAX storage directory #1383
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
base: main
Are you sure you want to change the base?
Conversation
Move yyjson from top-level dependency directory to `contrib/pax_storage/src/cpp/contrib/yyjson` since it's only used by the PAX storage component. Update all references in CMake build files, .gitmodules, and documentation to reflect the new location. This change improves the project structure by keeping PAX-specific dependencies together and removing the unnecessary top-level dependency directory. The move includes: - Moving yyjson directory to contrib/pax_storage/src/cpp/contrib/yyjson - Updating CMakeLists.txt and pax.cmake to reference the new location - Updating .gitmodules to reflect the new submodule path - Updating README.md documentation to show the new location
| Hi @tuhaihe ! I reviewed your PR and wanted to share an alternative implementation that eliminates the need for git submodules entirely. This approach uses CMake's  Alternative: CMake FetchContentI've implemented this on branch  Key change: Replace the git submodule with automatic dependency fetching during CMake configuration. Implementation
 if(USE_MANIFEST_API AND NOT USE_PAX_CATALOG)
    include(FetchContent)
    # Try system package first
    find_package(yyjson QUIET)
    if(NOT yyjson_FOUND)
        message(STATUS "yyjson not found in system, fetching from GitHub...")
        FetchContent_Declare(
            yyjson
            GIT_REPOSITORY https://github.com/ibireme/yyjson.git
            GIT_TAG 0.12.0
            GIT_SHALLOW TRUE
        )
        set(SAVED_BUILD_SHARED_LIBS ${BUILD_SHARED_LIBS})
        set(BUILD_SHARED_LIBS ON)
        FetchContent_MakeAvailable(yyjson)
        set(BUILD_SHARED_LIBS ${SAVED_BUILD_SHARED_LIBS})
    else()
        message(STATUS "Using system yyjson package")
    endif()
endif()
 # Update include path to use FetchContent variable
set(pax_target_include ${pax_target_include} ${yyjson_SOURCE_DIR}/src)Remove from  Benefits
 Important NoteDuring my review, I noticed that yyjson is only used when both  
 TestingTo verify this works when yyjson IS needed: cd contrib/pax_storage/build
cmake .. -DUSE_MANIFEST_API=ON -DUSE_PAX_CATALOG=OFF
# Should see: "yyjson not found in system, fetching from GitHub..."
makeRecommendationYour PR's organizational improvement is valuable regardless. This FetchContent approach is simply an alternative that: 
 Both approaches work - this is just offered as food for thought. Happy to discuss! Branch for reference:  | 
| Hi @edespino, thanks for your great idea. There are other submodules needed for PAX building, such as tabulate. I prefer to keep the same behavior across yyjson and the other submodules so that users can download all required submodules with  | 
| More: I searched and found that  | 
| Would like to have more voices from the core PAX developers on this. cc @gfphoenix78 @jiaqizho @gongxun0928 At least, it's not good to place the  | 
| Subject: Release Engineering View – Using EPEL vs. Building from SourceWhen building on Rocky Linux 9, EPEL can be convenient for missing dependencies, but it introduces several trade-offs worth noting: Key Issues
 Recommended Approach
 Bottom LineEPEL is fine for development convenience, but from a release-engineering and ASF-compliance standpoint, source builds give stronger reproducibility, auditability, and long-term stability. | 
| 
 One thing I'm curious about is why the selected submodule commit shas were used. It is not important for the relocation work. But it wasn't obvious at all why the yyjson submodule commit sha was used. | 
| 
 The reason why  @gfphoenix78 do you still have any context about this module? | 
| 
 Thanks @jiaqizho for your reply. I remember our private conversation on this topic. ❤️ Now, from the open-source side, I believe we can move the  | 
Move yyjson from top-level dependency directory to
contrib/pax_storage/src/cpp/contrib/yyjsonsince it's only used by the PAX storage component. Update all references in CMake build files, .gitmodules, and documentation to reflect the new location. This change improves the project structure by keeping PAX-specific dependencies together and removing the unnecessary top-level dependency directory.The move includes:
Fixes #ISSUE_Number
What does this PR do?
Type of Change
Breaking Changes
Test Plan
make installcheckmake -C src/test installcheck-cbdb-parallelImpact
Performance:
User-facing changes:
Dependencies:
Checklist
Additional Context
CI Skip Instructions