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

are_images_equal and many tests are linked with both libavif and libavif_internal #1816

Closed
wantehchang opened this issue Nov 28, 2023 · 2 comments · Fixed by #1819
Closed

Comments

@wantehchang
Copy link
Collaborator

I found that are_images_equal and many tests are linked with both libavif and libavif_internal. This is more noticeable when BUILD_SHARED_LIBS is ON (the default), because when BUILD_SHARED_LIBS is OFF, libavif_internal is an alias of libavif.

if(BUILD_SHARED_LIBS)
    add_library(avif_internal STATIC ${AVIF_SRCS})
    ...
else()
    add_library(avif_internal ALIAS avif)
    ...
endif()

Steps to reproduce:

$ cmake -G Ninja .. -DAVIF_CODEC_AOM=SYSTEM -DAVIF_BUILD_EXAMPLES=ON -DAVIF_BUILD_APPS=ON -DAVIF_BUILD_TESTS=ON -DAVIF_LOCAL_GTEST=ON
$ ninja -v

Here are two representative linker command lines on Linux:

/usr/bin/c++ tests/CMakeFiles/aviftest_helpers.dir/gtest/aviftest_helpers.cc.o tests/CMakeFiles/are_images_equal.dir/gtest/are_images_equal.cc.o -o tests/are_images_equal -Wl,-rpath,/home/wtc/libavif.1/libavif/build libavif_apps.a libavif.so.16.0.2 /usr/lib/x86_64-linux-gnu/libpng.so /usr/lib/x86_64-linux-gnu/libz.so /usr/lib/x86_64-linux-gnu/libjpeg.so libavif_internal.a /usr/lib/x86_64-linux-gnu/libaom.so -lm

/usr/bin/c++ tests/CMakeFiles/aviftest_helpers.dir/gtest/aviftest_helpers.cc.o tests/CMakeFiles/aviftilingtest.dir/gtest/aviftilingtest.cc.o -o tests/aviftilingtest -Wl,-rpath,/home/wtc/libavif.1/libavif/build /home/wtc/libavif.1/libavif/ext/googletest/build/lib/libgtest.a /home/wtc/libavif.1/libavif/ext/googletest/build/lib/libgtest_main.a libavif_apps.a libavif.so.16.0.2 /usr/lib/x86_64-linux-gnu/libpng.so /usr/lib/x86_64-linux-gnu/libz.so /usr/lib/x86_64-linux-gnu/libjpeg.so libavif_internal.a /usr/lib/x86_64-linux-gnu/libaom.so -lm

Note that we link the executable programs with both libavif.so.16.0.2 and libavif_internal.a.

I saw the same problem when I built libavif on Windows and forgot to set BUILD_SHARED_LIBS to OFF.

@wantehchang
Copy link
Collaborator Author

@vrabaud @y-guyon

The following patch seems to fix the problem. I don't think it is the right fix. The patch is intended to pinpoint the sources of the problem (avif_apps and the avifincrtest_helpers, which depend on avif, are used with aviftest_helpers, which depends on avif_internal).

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 18629ae..40123f1 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -777,7 +777,7 @@ if(AVIF_BUILD_APPS OR (AVIF_BUILD_TESTS AND (AVIF_ENABLE_FUZZTEST OR AVIF_ENABLE
         avif_apps STATIC apps/shared/avifexif.c apps/shared/avifjpeg.c apps/shared/avifpng.c apps/shared/avifutil.c
                          apps/shared/iccmaker.c apps/shared/y4m.c third_party/iccjpeg/iccjpeg.c
     )
-    target_link_libraries(avif_apps avif ${AVIF_PLATFORM_LIBRARIES} ${PNG_LIBRARY} ${ZLIB_LIBRARY} ${JPEG_LIBRARY})
+    target_link_libraries(avif_apps ${AVIF_PLATFORM_LIBRARIES} ${PNG_LIBRARY} ${ZLIB_LIBRARY} ${JPEG_LIBRARY})
     # In GitHub CI's macos-latest os image, /usr/local/include has not only the headers of libpng
     # and libjpeg but also the headers of an older version of libavif. Put the avif include
     # directory before ${PNG_PNG_INCLUDE_DIR} ${JPEG_INCLUDE_DIR} to prevent picking up old libavif
@@ -803,12 +803,12 @@ if(AVIF_BUILD_APPS)
     if(AVIF_USE_CXX)
         set_target_properties(avifenc PROPERTIES LINKER_LANGUAGE "CXX")
     endif()
-    target_link_libraries(avifenc avif_apps)
+    target_link_libraries(avifenc avif_apps avif)
     add_executable(avifdec apps/avifdec.c)
     if(AVIF_USE_CXX)
         set_target_properties(avifdec PROPERTIES LINKER_LANGUAGE "CXX")
     endif()
-    target_link_libraries(avifdec avif_apps)
+    target_link_libraries(avifdec avif_apps avif)
 
     if(NOT SKIP_INSTALL_APPS AND NOT SKIP_INSTALL_ALL)
         install(
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index d174e05..8268b3d 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -97,7 +97,8 @@ if(AVIF_ENABLE_GTEST OR AVIF_ENABLE_FUZZTEST)
     endif()
 
     add_library(avifincrtest_helpers OBJECT gtest/avifincrtest_helpers.cc)
-    target_link_libraries(avifincrtest_helpers avif ${AVIF_PLATFORM_LIBRARIES} ${GTEST_LIBRARIES})
+    target_link_libraries(avifincrtest_helpers ${AVIF_PLATFORM_LIBRARIES} ${GTEST_LIBRARIES})
+    target_include_directories(avifincrtest_helpers PRIVATE $<TARGET_PROPERTY:avif,INTERFACE_INCLUDE_DIRECTORIES>)
     target_include_directories(avifincrtest_helpers PUBLIC ${GTEST_INCLUDE_DIRS})
 endif()
 

@wantehchang
Copy link
Collaborator Author

Please see the patch.txt file in patch.zip. I removed the dependency of aviftest_helpers.cc on avif_internal. This allows the tests that only call the public functions to be linked with avif rather than avif_internal.

This patch is unlikely to be the right fix. It is intended to show which tests can be linked with avif and which tests must be linked with avif_internal.

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 a pull request may close this issue.

1 participant