Skip to content

build: use CRoaring amalgamation sources - #2556

Open
wxyucs wants to merge 1 commit into
antgroup:mainfrom
wxyucs:codex/croaring-amalgamation-download
Open

build: use CRoaring amalgamation sources#2556
wxyucs wants to merge 1 commit into
antgroup:mainfrom
wxyucs:codex/croaring-amalgamation-download

Conversation

@wxyucs

@wxyucs wxyucs commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Change Type

  • Bug fix
  • New feature
  • Improvement/Refactor
  • Documentation
  • CI/Build/Infra

Linked Issue

Not applicable: this is a kind/improvement dependency-download optimization.

What Changed

  • Download the three official CRoaring v3.0.1 standalone release assets
    (roaring.c, roaring.h, and roaring.hh) instead of the full source
    archive.
  • Verify every cached, mirrored, local, or upstream asset with pinned SHA-256
    hashes and retain deterministic DOWNLOAD_DIR caching.
  • Preserve the established local/URL source-archive override with its pinned
    MD5 while keeping normal builds on the smaller standalone assets.
  • Build an equivalent local roaring target while preserving VSAG's include,
    link, AVX-disable, PIC/static/shared, warning, and install/export behavior.
  • Document the CRoaring directory/base-URL override in the English and Chinese
    offline-build guides.

The observed dependency transfer falls from 100,717,067 bytes for the former
v3.0.1 source archive to 1,232,827 bytes for the three official release assets:
99,484,240 fewer bytes (98.775950%, or 81.696x smaller).

Test Evidence

  • make fmt
  • make lint
  • make test
  • make cov, run tests, and collect coverage
  • Other (describe below)

Test details:

Clean CMake 3.31 configure through a local HTTP mirror: passed
Second configure: all three CRoaring assets reused from the verified cache
Build roaring with GCC 11, C11, PIC, and -Werror: passed
Full ENABLE_TESTS + mock implementation build: passed (671 targets)
unittests "[bitset]": passed (38 assertions, 5 cases)
functests "Test BitsetHandler": passed (4 assertions, 1 case)
functests "[filter_search]~[daily]": passed (43,525 assertions, 5 cases)
Full non-daily unittests: passed (85,542,300 assertions, 636 cases)
Forced AVX/AVX-512-disabled roaring build: passed
ASan/UBSan roaring build: passed
BUILD_SHARED_LIBS=ON roaring build and SONAME/symlink inspection: passed
Install/export inspection: CRoaring is not installed or exported
Fresh CMake 3.22.1 configure and roaring build: passed
Local amalgamation directory with no network access: passed
Local HTTP amalgamation base URL on CMake 3.22.1: passed
Local source archive with unreachable proxies on CMake 3.31 and 3.22.1: passed
Local HTTP source-archive URL on CMake 3.22.1: passed
AVX definitions present on roaring.c and absent from its consumer: passed
Verbose HTTP-success/hash-mismatch diagnostics: passed
git diff --check: passed
clang-format/tidy: not applicable; no repository C/C++ source changed

The clean download cache contains only the three expected assets and no
CRoaring archive. Their exact sizes and SHA-256 hashes match the official
v3.0.1 release metadata.

Compatibility Impact

  • API/ABI compatibility: none; VSAG retains the same roaring target and
    <roaring.hh> API, and the standalone assets contain the complete v3.0.1
    implementation and C++ wrapper.
  • Behavior changes: normal CRoaring downloads use three verified release files
    rather than one large archive; explicit legacy archive overrides remain
    supported.

Performance and Concurrency Impact

  • Performance impact: dependency download volume improves by 98.775950%;
    runtime behavior is unchanged.
  • Concurrency/thread-safety impact: none.

Documentation Impact

  • No docs update needed
  • Updated docs:
    • README.md
    • DEVELOPMENT.md
    • CONTRIBUTING.md
    • Other: English and Chinese offline-build guides

Risk and Rollback

  • Risk level: low. The main risk is platform-specific compilation of the
    upstream amalgamation; Linux GCC, sanitizer, shared/static, AVX-disabled, and
    older-CMake paths were exercised, and existing macOS CI provides the dynamic
    AppleClang check.
  • Rollback plan: revert this commit to restore the full-archive FetchContent
    integration.

Checklist

  • I have linked the relevant issue (not required for kind/improvement)
  • I have added/updated tests for new behavior or bug fixes (not applicable;
    build integration is covered by the clean configure/build evidence above)
  • I have considered API compatibility impact
  • I have updated docs if behavior/workflow changed
  • My commit messages follow project conventions (Conventional Commits,
    optional [skip ci] prefix)

Copilot AI review requested due to automatic review settings July 28, 2026 10:02
@wxyucs wxyucs added kind/improvement Optimizations, UX polish, or minor improvements 性能优化、体验打磨或细节改良 module/thirdparty version/1.1 labels Jul 28, 2026
@vsag-bot

vsag-bot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

/label status/waiting-for-review
/waiting-on reviewer
/request-review @jiaweizone
/request-review @inabao

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@vsag-bot
vsag-bot self-requested a review July 28, 2026 10:03
@vsag-bot

vsag-bot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Automated pull request review completed.

Review effort: medium (208 changed lines across 4 files).

Submitted 2 inline comments.
Review: #2556 (review)

@mergify mergify Bot added the module/docs label Jul 28, 2026
@mergify

mergify Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 Require kind label

  • label~=^kind/

🟢 Require version label

  • label~=^version/

Comment thread extern/roaringbitmap/roaringbitmap.cmake
Copilot AI review requested due to automatic review settings July 28, 2026 10:06
@wxyucs
wxyucs force-pushed the codex/croaring-amalgamation-download branch from 9bfea01 to da90bac Compare July 28, 2026 10:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread extern/roaringbitmap/roaringbitmap.cmake

@LHT129 LHT129 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a well-executed build-system improvement that reduces CRoaring dependency download size by ~98.8% (from ~100 MB to ~1.2 MB) by switching from the full source archive to the three official amalgamation assets.

Summary of changes:

  • extern/roaringbitmap/roaringbitmap.cmake: Replaced FetchContent with manual file(DOWNLOAD) for the three amalgamation files (roaring.c, roaring.h, roaring.hh), with SHA-256 verification and deterministic caching. Manually creates the roaring target with matching upstream properties (C11, SOVERSION, AVX disable, etc.).
  • cmake/VSAGOptions.cmake: Updated DOWNLOAD_DIR docstring to reflect broader usage beyond ExternalProject.
  • docs/: Updated both English and Chinese offline-build guides to document the CRoaring-specific override behavior (directory/base URL instead of single archive).

What I checked:

  • Correctness: The amalgamation files are verified with pinned SHA-256 hashes. The download function handles cached, mirrored, local, and upstream fallback paths correctly. The roaring target properties match upstream behavior.
  • Security: TLS_VERIFY ON is set. Hashes are pinned. No command injection vectors.
  • Compatibility: vsag_roaring_headers now points to the amalgamation root directory (where roaring.h and roaring.hh live), which is correct for the single-file layout. The roaring-headers/roaring-headers-cpp INTERFACE targets are retained for upstream name compatibility.
  • Documentation: Both EN and ZH guides accurately describe the new override behavior.

One minor note (posted as inline comment): The roaring target does not set POSITION_INDEPENDENT_CODE. This is fine for the shared-library path, but static-library consumers embedding VSAG in a shared library may benefit from adding it. Non-blocking.

Overall this PR is in good shape. The existing inline discussion about the INTERFACE targets has been resolved with an explanatory comment.

@vsag-bot vsag-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated inline review completed.

Review effort: medium (190 changed lines across 4 files).
Submitted 2 inline comments.

Reviewed commit da90bac.

Comment thread extern/roaringbitmap/roaringbitmap.cmake Outdated
Comment thread extern/roaringbitmap/roaringbitmap.cmake
@wxyucs
wxyucs force-pushed the codex/croaring-amalgamation-download branch from da90bac to 61341fa Compare July 28, 2026 11:28
Copilot AI review requested due to automatic review settings July 28, 2026 11:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@vsag-bot
vsag-bot self-requested a review July 28, 2026 11:28
Comment thread extern/roaringbitmap/roaringbitmap.cmake
@wxyucs
wxyucs force-pushed the codex/croaring-amalgamation-download branch from 61341fa to 9a9f237 Compare July 28, 2026 11:33
Copilot AI review requested due to automatic review settings July 28, 2026 11:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread extern/roaringbitmap/roaringbitmap.cmake
Comment thread extern/roaringbitmap/roaringbitmap.cmake

@vsag-bot vsag-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated inline review completed.

Review effort: medium (208 changed lines across 4 files).
Submitted 2 inline comments.

Reviewed commit 9a9f237.

Comment thread extern/roaringbitmap/roaringbitmap.cmake
Comment thread docs/docs/en/src/development/offline_build.md Outdated
Signed-off-by: Xiangyu Wang <wxy407827@antgroup.com>
Assisted-by: OpenAI Codex:gpt-5
@wxyucs
wxyucs force-pushed the codex/croaring-amalgamation-download branch from 9a9f237 to c3a9079 Compare July 29, 2026 06:38
Copilot AI review requested due to automatic review settings July 29, 2026 06:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@LHT129 LHT129 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This iteration looks clean. All previously raised issues have been addressed:

  • The roaring-headers/roaring-headers-cpp interface targets are retained with a comment documenting the compatibility purpose.
  • The EXPECTED_HASH fatal-error-on-first-URL issue has been resolved by switching to manual SHA-256 verification with multi-URL fallback.
  • The download_log is now emitted at VERBOSE level for all download attempts.
  • The AVX/AVX512 disable definitions are now PRIVATE on the amalgamation roaring target.
  • Archive-form override compatibility is preserved with the roaringbitmap_archive_override detection branch.
  • Documentation correctly scopes the new directory/base-URL form to main and notes the archive-only form for release lines.

No new issues found in this push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/improvement Optimizations, UX polish, or minor improvements 性能优化、体验打磨或细节改良 module/docs module/thirdparty size/L version/1.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants