Skip to content

Conversation

@autoantwort
Copy link

When compiled as static lib the generated config contains the following

set_target_properties(avif PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "\$<LINK_ONLY:m>;\$<LINK_ONLY:Threads::Threads>"
)

but the target Threads::Threads is not known because the following is missing:

include(CMakeFindDependencyMacro)
set(CMAKE_THREAD_PREFER_PTHREADS ON)
set(THREADS_PREFER_PTHREAD_FLAG ON)
find_dependency(Threads)

like in the CMakeLists.txt

@autoantwort autoantwort force-pushed the fix-dependency-in-config branch from e7bc8ce to 01bae06 Compare April 13, 2023 11:38
@wantehchang wantehchang requested review from jzern and y-guyon April 13, 2023 21:58
Copy link
Contributor

@jzern jzern left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. I made a similar fix in libwebp a little while ago.

I extended our tests to include an executable to make sure the dependencies worked. Maybe .github/workflows/ci-unix-static.yml can be extended similarly.

    cat > CMakeLists.txt <<EOF
cmake_minimum_required(VERSION 2.8.12)

project(libwebp C)

find_package(WebP)
if(NOT WebP_FOUND)
  message(FATAL_ERROR "WebP package not found")
endif()
message("WebP_FOUND: \${WebP_FOUND}")
message("WebP_INCLUDE_DIRS: \${WebP_INCLUDE_DIRS}")
message("WebP_LIBRARIES: \${WebP_LIBRARIES}")
message("WEBP_INCLUDE_DIRS: \${WEBP_INCLUDE_DIRS}")
message("WEBP_LIBRARIES: \${WEBP_LIBRARIES}")

if(POLICY CMP0028)
  cmake_policy(SET CMP0028 NEW)
endif()

add_executable(main "main.c")
target_link_libraries(main WebP::webp)
target_include_directories(main PRIVATE ${WEBP_INCLUDE_DIRS})
EOF
cat > main.c << EOF
#include "webp/encode.h"

int main() { return WebPGetEncoderVersion(); }
EOF

)

file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}-config.cmake.in "@PACKAGE_INIT@\n")
if(UNIX AND NOT BUILD_SHARED_LIBS)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose in the shared object case, the library's dependency is enough.

if(UNIX AND NOT BUILD_SHARED_LIBS)
file(APPEND ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}-config.cmake.in "
include(CMakeFindDependencyMacro)
set(CMAKE_THREAD_PREFER_PTHREADS ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

CMAKE_THREAD_PREFER_PTHREAD?

It looks like this flag went away somewhere between 3.2.x and 3.26.x (or it's left out from the documentation)

https://cmake.org/cmake/help/v3.2/module/FindThreads.html
https://cmake.org/cmake/help/latest/module/FindThreads.html

@jzern
Copy link
Contributor

jzern commented Apr 14, 2023

Thanks for the patch. I made a similar fix in libwebp a little while ago.

I extended our tests to include an executable to make sure the dependencies worked. Maybe .github/workflows/ci-unix-static.yml can be extended similarly.

    cat > CMakeLists.txt <<EOF
cmake_minimum_required(VERSION 2.8.12)

project(libwebp C)

find_package(WebP)
if(NOT WebP_FOUND)
  message(FATAL_ERROR "WebP package not found")
endif()
message("WebP_FOUND: \${WebP_FOUND}")
message("WebP_INCLUDE_DIRS: \${WebP_INCLUDE_DIRS}")
message("WebP_LIBRARIES: \${WebP_LIBRARIES}")
message("WEBP_INCLUDE_DIRS: \${WEBP_INCLUDE_DIRS}")
message("WEBP_LIBRARIES: \${WEBP_LIBRARIES}")

if(POLICY CMP0028)
  cmake_policy(SET CMP0028 NEW)
endif()

add_executable(main "main.c")
target_link_libraries(main WebP::webp)
target_include_directories(main PRIVATE ${WEBP_INCLUDE_DIRS})
EOF
cat > main.c << EOF
#include "webp/encode.h"

int main() { return WebPGetEncoderVersion(); }
EOF

I forgot to include the invocation:

cmake . -DCMAKE_PREFIX_PATH="${BUILD_DIR}/webp-install/usr/local"
make

@jzern
Copy link
Contributor

jzern commented Jul 22, 2024

I'm sorry, I lost track of this. There have been changes to the cmake files and this will need to be rebased.

cc: @fdintino

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.

2 participants