Skip to content

Add fetch content for c-ares. - #400

Open
fe-dagostino wants to merge 5 commits into
an-tao:masterfrom
fe-dagostino:master
Open

Add fetch content for c-ares.#400
fe-dagostino wants to merge 5 commits into
an-tao:masterfrom
fe-dagostino:master

Conversation

@fe-dagostino

Copy link
Copy Markdown

Propose to adopt fetch_content and to simply all cmake files.

@fe-dagostino

Copy link
Copy Markdown
Author

it seems that I'm fighting all the time with linters!

@fe-dagostino

Copy link
Copy Markdown
Author

to be honest my version was more readable :D

Copilot AI left a comment

Copy link
Copy Markdown

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 introduces FetchContent-based provisioning of c-ares for trantor's optional asynchronous DNS resolver. A new cmake/c-ares.cmake module declares a pinned c-ares source build (v1.34.6, static, PIC), and the top-level CMakeLists.txt includes it before calling find_package(c-ares) (with OVERRIDE_FIND_PACKAGE). The link target is renamed from c-ares_lib to the namespaced c-ares::c-ares. The existing Findc-ares.cmake is tidied to use lowercase variable names matching the package name, but it still produces only c-ares_lib.

Changes:

  • Add cmake/c-ares.cmake that pulls and statically builds c-ares via FetchContent, with OVERRIDE_FIND_PACKAGE and a c-ares::c-ares alias.
  • Wire the new module into the top-level build (include(FetchContent), prepend cmake/ to CMAKE_MODULE_PATH, include(c-ares) before find_package, and link c-ares::c-ares).
  • Lowercase the variable names inside cmake_modules/Findc-ares.cmake for consistency with the package name.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
CMakeLists.txt Adds module-path setup, FetchContent include, include(c-ares), and switches link target to c-ares::c-ares.
cmake/c-ares.cmake New FetchContent module for c-ares with static build options and namespaced alias.
cmake_modules/Findc-ares.cmake Renames C-ARES_* cache/output variables to lowercase c-ares_*.
Comments suppressed due to low confidence (1)

CMakeLists.txt:266

  • include(c-ares) is invoked unconditionally inside if(BUILD_C-ARES), which means a c-ares source tree is always cloned and built from GitHub even when the user already has c-ares installed system-wide. Previously, find_package(c-ares) would simply succeed against the system library at no extra cost. Consider gating include(c-ares) on find_package(c-ares QUIET) failing first, or expose a separate option (e.g. TRANTOR_FETCH_C_ARES) so that users/packagers/CI can opt out of the network fetch.
if(BUILD_C-ARES)
  include(c-ares)
  find_package(c-ares)
  if(c-ares_FOUND)
    message(STATUS "c-ares found!")
    set(HAVE_C-ARES TRUE)
  endif()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmake/c-ares.cmake
Comment on lines +1 to +5
FetchContent_Declare(
c-ares
GIT_REPOSITORY https://github.com/c-ares/c-ares.git
GIT_TAG v1.34.6 OVERRIDE_FIND_PACKAGE
)
Comment thread CMakeLists.txt
target_compile_definitions(${PROJECT_NAME} PRIVATE CARES_STATICLIB)
endif()
target_link_libraries(${PROJECT_NAME} PRIVATE c-ares_lib)
target_link_libraries(${PROJECT_NAME} PRIVATE c-ares::c-ares)
Comment thread CMakeLists.txt
Comment thread cmake/c-ares.cmake

FetchContent_MakeAvailable(c-ares)

add_library(c-ares::c-ares ALIAS c-ares)
Comment thread cmake/c-ares.cmake
Comment on lines +20 to +23
set(c-ares_INCLUDE_DIRS "${FETCHCONTENT_BASE_DIR}/c-ares-build/" "${FETCHCONTENT_BASE_DIR}/c-ares-src/include/")
set(c-ares_LIBRARIES "${FETCHCONTENT_BASE_DIR}/c-ares-build/src/lib/")

mark_as_advanced(c-ares_INCLUDE_DIRS c-ares_LIBRARIES)
@an-tao

an-tao commented May 29, 2026

Copy link
Copy Markdown
Owner

@fe-dagostino Thanks for the PR, this is a good improvement overall.

I think we may want to clarify two things in the CMake logic:

  1. Prefer find_package(c-ares) first, and only fallback to FetchContent if it is not found. This would better support offline builds and system/package-manager based workflows.

  2. In the system-c-ares case, we still need to keep find_dependency(c-ares) in TrantorConfig.cmake, because Trantor::Trantor will link against c-ares::c-ares transitively, and consumers would otherwise fail at configure time.

So essentially:

  • system c-ares → must find_dependency(c-ares)
  • fetch/internal c-ares → no need to propagate dependency

This keeps both modes working correctly.

@fe-dagostino

Copy link
Copy Markdown
Author

Hi @an-tao you are welcome! thanks to you for the library :D
unfortunately I don't have visibility about all the projects using the trantor library, so I based the PR mostly on the needs I have, don't taking in account the cmake versions.
coming back to the PR, yes the approach you are proposing is consistent and allow backward compatibility. I personally don't like to prioritize the packages installed in the system but in this case I guess is the best solution.
I believe that current cmake can be cleaned a bit if you decide to move to a newer version.
Please let me know if there is something I can do to help with current PR.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants