Page MenuHomePhabricator

[Support][llvm] Add optional libCURL dependency to llvm build configuration
ClosedPublic

Authored by noajshu on Oct 6 2021, 9:23 AM.

Details

Summary

This finds the curl libraries if LLVM_ENABLE_CURL is set. This is needed to implement the debuginfod client library in LLVM.

Diff Detail

Event Timeline

noajshu created this revision.Oct 6 2021, 9:23 AM
noajshu requested review of this revision.Oct 6 2021, 9:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2021, 9:23 AM
lebedev.ri added inline comments.
llvm/CMakeLists.txt
407 ↗(On Diff #377569)

This can't be right, shouldn't you be using CURL_LIBRARIES set by find_package()?

noajshu added inline comments.Oct 6 2021, 10:12 AM
llvm/CMakeLists.txt
407 ↗(On Diff #377569)

Thanks, I agree! L407 is needed to avoid the following error which arises if it is removed:

CMake Error at /usr/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:165 (message):
  Could NOT find CURL (missing: CURL_LIBRARY) (found suitable version
  "7.74.0", minimum required is "7.74.0")
Call Stack (most recent call first):
  /usr/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:458 (_FPHSA_FAILURE_MESSAGE)
  /usr/share/cmake-3.18/Modules/FindCURL.cmake:169 (find_package_handle_standard_args)
  CMakeLists.txt:407 (find_package)


-- Configuring incomplete, errors occurred!

I tried a few workarounds but setting the CURL_LIBRARY was the simplest fix I found. Do you have any idea what might cause this?

lebedev.ri added inline comments.Oct 6 2021, 1:53 PM
llvm/CMakeLists.txt
407 ↗(On Diff #377569)

I don't know why you are seeing that error, but this should be roughly

if (LLVM_ENABLE_DEBUGINFOD_CLIENT)
  set(LLVM_WITH_CURL 1)
  find_package(CURL 7.74.0 REQUIRED)
  add_compile_definitions(
    LLVM_ENABLE_DEBUGINFOD_CLIENT
    LLVM_WITH_CURL)
  # use CURL_LIBRARIES where needed / link to CURL::libcurl imported target.
endif()

Anything that hardcodes -lcurl is wrong.

phosek added a comment.EditedOct 6 2021, 2:26 PM

This logic should go into config-ix.cmake. I'd recommend following the same model we use for zlib here: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/config-ix.cmake#L119. So this should look something like this:

if(LLVM_ENABLE_CURL)
  if(LLVM_ENABLE_CURL STREQUAL FORCE_ON)
    find_package(CURL REQUIRED)
  else()
    find_package(CURL)
  endif()
  if(CURL_FOUND)
    # Check if curl we found is usable; for example, we may have found a 32-bit
    # library on a 64-bit system which would result in a link-time failure.
    cmake_push_check_state()
    list(APPEND CMAKE_REQUIRED_INCLUDES ${CURL_INCLUDE_DIRS})
    list(APPEND CMAKE_REQUIRED_LIBRARIES ${CURL_LIBRARY})
    check_symbol_exists(curl_easy_init curl.h HAVE_CURL)
    cmake_pop_check_state()
    if(LLVM_ENABLE_CURL STREQUAL FORCE_ON AND NOT HAVE_CURL)
      message(FATAL_ERROR "Failed to configure curl")
    endif()
  endif()
  set(LLVM_ENABLE_CURL "${HAVE_CURL}")
endif()

To use it, you'd include CURL::libcurl as a dependency in you target, but only if LLVM_ENABLE_CURL is set. See https://github.com/llvm/llvm-project/blob/fd9a5b911d5e85c5184e9ed6907f53f23b41cd99/llvm/lib/Support/CMakeLists.txt#L25 for a zlib example.

llvm/CMakeLists.txt
405 ↗(On Diff #377569)

Rather than having a LLVM_ENABLE_DEBUGINFOD_CLIENT variable, I'd prefer LLVM_ENABLE_CURL to control whether to search for curl or not. Whether debuginfod client should be built or not could be conditionalized on that.

noajshu updated this revision to Diff 378789.Oct 11 2021, 1:57 PM

rebase against main

noajshu updated this revision to Diff 378792.Oct 11 2021, 2:13 PM

Move CURL dependency logic to config-ix.cmake.
Also update to the same model used for zlib as suggested by phosek.

noajshu updated this revision to Diff 378794.Oct 11 2021, 2:17 PM

Remove extraneous change to llvm/CMakeLists.txt

phosek accepted this revision.Oct 11 2021, 2:53 PM

LGTM

This revision is now accepted and ready to land.Oct 11 2021, 2:53 PM
noajshu updated this revision to Diff 378818.Oct 11 2021, 4:00 PM

changed header file from curl.h to curl/curl.h to pass check_symbol_exists test

noajshu updated this revision to Diff 378828.Oct 11 2021, 4:57 PM

rebase against main

Thanks @phosek. I don’t have commit access, could you land this patch for me? Please use “Noah Shutty <shutty@google.com>” to commit the change.

noajshu edited the summary of this revision. (Show Details)Oct 11 2021, 4:59 PM
noajshu updated this revision to Diff 379081.Oct 12 2021, 9:27 AM

rebase against D111238