Skip to content

build: support system fmt dependency#2169

Open
wxyucs wants to merge 1 commit into
mainfrom
build/use-system-fmt
Open

build: support system fmt dependency#2169
wxyucs wants to merge 1 commit into
mainfrom
build/use-system-fmt

Conversation

@wxyucs

@wxyucs wxyucs commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add VSAG_USE_SYSTEM_FMT as a per-dependency override for VSAG_USE_SYSTEM_DEPS.
  • Reuse pre-existing or system fmt::fmt targets and fall back to bundled fmt when AUTO cannot find a valid system header.
  • Add target header validation so incomplete fmt packages fail early under ON instead of breaking during compilation.

Verification

  • make debug VSAG_USE_SYSTEM_DEPS=OFF EXTRA_DEFINED=\"-DVSAG_USE_SYSTEM_FMT=AUTO\"
  • External superbuild at /home/xiangyu/vsag-system-fmt-superbuild with parent-provided fmt::fmt and an executable linking both vsag and fmt::fmt.

Copilot AI review requested due to automatic review settings June 8, 2026 12:12
@wxyucs wxyucs added kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) version/0.18 labels Jun 8, 2026
@mergify

mergify Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 Require kind label

Wonderful, this rule succeeded.
  • label~=^kind/

🟢 Require version label

Wonderful, this rule succeeded.
  • label~=^version/

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces support for using a system-installed fmt library instead of always downloading it via FetchContent. It adds a helper function vsag_target_has_header to verify if a target contains the required header, exposes a VSAG_USE_SYSTEM_FMT option, and updates the fmt configuration to search for the system package. Feedback suggests removing the redundant and cache-polluting find_path call in favor of relying solely on find_package, and adding a safety check in vsag_target_has_header to prevent checking the root directory when an include directory path is empty.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread extern/fmt/fmt.cmake Outdated
Comment thread cmake/VSAGHelpers.cmake Outdated

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.

Pull request overview

This PR extends VSAG’s CMake third-party dependency resolution to support using a system-provided fmt independently of the global VSAG_USE_SYSTEM_DEPS setting, including reuse of a parent project’s pre-existing fmt::fmt target and early validation of required headers.

Changes:

  • Add VSAG_USE_SYSTEM_FMT cache option as a per-dependency override.
  • Update extern/fmt/fmt.cmake to prefer pre-existing/system fmt::fmt and fall back to bundled fmt when unavailable.
  • Introduce a CMake helper to validate that an imported/parent-provided target’s include dirs contain a required header.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
extern/fmt/fmt.cmake Implements system/pre-existing fmt::fmt discovery with header validation and bundled fallback.
cmake/VSAGOptions.cmake Adds VSAG_USE_SYSTEM_FMT option to control system fmt usage policy.
cmake/VSAGHelpers.cmake Adds vsag_target_has_header() helper used for validating dependency targets.

Comment thread extern/fmt/fmt.cmake
@wxyucs wxyucs assigned wxyucs and unassigned wxyucs Jun 8, 2026
Reuse parent or system fmt targets while keeping bundled fallback behavior safe for incomplete installations.

Signed-off-by: Xiangyu Wang <wxy407827@antgroup.com>
Assisted-by: OpenCode:gpt-5.5
@wxyucs wxyucs force-pushed the build/use-system-fmt branch from 6de59ac to 2eb9bc2 Compare June 8, 2026 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) module/thirdparty size/M version/1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants