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

cmake(libfmt): use find_package to find libfmt #3786

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chenrui333
Copy link

@chenrui333 chenrui333 commented Jan 23, 2025

Description

TODO: fill description here

This patch improves libfmt detection by calling find_package(fmt QUIET) first when WITH_LIBFMT is set to system or auto. If a system libfmt is located successfully, we skip the custom source-compile snippet. Otherwise, we proceed with the old detection logic (compile test) and finally fall back to the bundled library if needed. This change makes it easier for package managers (e.g., Homebrew) or system environments that already have a modern fmt installed to integrate with MariaDB, without having to override CMake variables manually.

Before this patch, the logic always set CMAKE_REQUIRED_INCLUDES to the bundled copy’s include directory and ran a snippet test, failing if the user had removed or disabled the bundled directory. Now, if a system installation is discoverable via CMake config files, MariaDB can be built cleanly against that library.

Release Notes

  • Enhancement: MariaDB can now detect and use system-provided libfmt more reliably when WITH_LIBFMT is system or auto. If find_package(fmt) succeeds, the bundled fallback is skipped, ensuring smoother packaging in environments providing up-to-date versions of libfmt.

No system variables or status variables were changed. No existing behavior changed aside from this improvement in build-time detection of libfmt.

How can this PR be tested?

homebrew PRs

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

relates to:

We now call find_package(fmt QUIET) to detect a system-installed libfmt when WITH_LIBFMT is system or auto. If that succeeds, we skip the old snippet-based test. Otherwise, we fall back to the existing compile test and bundling logic. This enables standard CMake config discovery for system libfmt (e.g., from Homebrew) without forcibly setting CMAKE_REQUIRED_INCLUDES to the bundled path.
@grooverdan
Copy link
Member

Currently because libfmt changed the API by removing the thousands separator (fmtlib/fmt#4284) and we haven't written a compatibility for the newer libfmt yet.

ref: https://jira.mariadb.org/browse/MDEV-35746

@cvicentiu cvicentiu added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.
Development

Successfully merging this pull request may close these issues.

3 participants