build: static compile local dependencies; combine library archives when building static libavif #1579

fdintino · 2023-09-12T12:35:16Z

When building libavif as a shared library, it makes more sense for local dependencies to be statically linked. Otherwise, the shared library is not portable: it can be run from the build directory but not anywhere else. I've modified the ci-unix-shared-local workflow on a separate branch to demonstrate the issue, adding a ninja install and then a call to ldd (to show the missing links) and avifenc --help (job output)

Run ldd /usr/local/lib/libavif.so
	linux-vdso.so.1 (0x00007ffefa387000)
	libdav1d.so.6 => not found
	libaom.so.3 => /lib/x86_64-linux-gnu/libaom.so.3 (0x00007f3421ed6000)
	libyuv.so => not found
	libsharpyuv.so.0 => not found
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f3421def000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f3421bc5000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f3422485000)

avifenc: error while loading shared libraries: libavif.so: cannot open shared object file: No such file or directory
Error: Process completed with exit code 127.

I encountered this issue in my own GHA workflow when I tried to build against the latest version of libavif, since I don't have the sed commands that were added to ci-unix-shared-local, and because the CMakeLists.txt was modified to only allow shared local dependencies when building a shared library.

vrabaud · 2023-09-12T13:25:27Z

I'd also be in favor of forcing static linkage to simplify CMake (which you already did).
I do not see any use case for keeping shared deps. Any opinion @wantehchang ?

fdintino · 2023-09-22T18:07:44Z

Any updates on this?

vrabaud · 2023-09-25T11:33:55Z

Still in favor: If we delete a folder in ext by inadvertence, the binaries still use the right versions of the deps (the .a).

wantehchang

Vincent: I'm sorry I missed your previous question for me on Sep 12 (because I just returned to work from a vacation). I think it is reasonable to link with the kind of library built by the scripts in the ext directory (which is static libraries). I've asked Yannis to review this pull request because this is closely related to his #1169. (I will also review this pull request.)

.github/workflows/ci-disable-gtest.yml

ext/libyuv.cmd

wantehchang · 2023-09-25T14:16:26Z

CMakeLists.txt

@@ -162,7 +160,7 @@ if(AVIF_LOCAL_LIBYUV)
    if(DEFINED ANDROID_ABI)
        set(AVIF_LIBYUV_BUILD_DIR "${AVIF_LIBYUV_BUILD_DIR}/${ANDROID_ABI}")
    endif()
-    set(LIB_FILENAME "${AVIF_LIBYUV_BUILD_DIR}/${AVIF_LIBRARY_PREFIX}yuv${AVIF_LIBRARY_SUFFIX}")
+    set(LIB_FILENAME "${AVIF_LIBYUV_BUILD_DIR}/${AVIF_LIBRARY_PREFIX}yuv${CMAKE_STATIC_LIBRARY_SUFFIX}")


Yannis: Could you review this pull request? It reverts some of the changes you made in #1169.

My assumption was that building a shared libavif made little sense with static dependencies, so either everything is static or everything is shared. Most CI tests exercise "everything static". Reusing scripts in ext and editing them with sed is just a convenient way of testing "everything shared" in other CI tests.

If shared libavif with static dependencies is useful, I guess "everything shared" can be replaced by "everything static but libavif" in the CI and in the CMakeLists.txt. I assume this explains AVIF_LIBRARY_SUFFIX being replaced by CMAKE_STATIC_LIBRARY_SUFFIX in all ext/ dependencies in this PR.

So this change looks good to me overall (except for the already mentioned issues). The downside is that "everything shared" is no longer tested because some installed dependencies are not used in our GitHub workflows (such as GoogleTest). This is fine with me.

It's useful whenever you want to distribute a library that provides bindings to libavif with a bundled build of libavif. For python and node, this is accomplished by shipping your extension with a shared library that has a modified RPATH, to get picked up at runtime.

I could be doing something wrong, but I have had difficulty linking against a static build of libavif. It seems that I need to combine the other static libraries using LIB.EXE or ar, or else add them to the link flags when I build the extension. And the libavif.pc file doesn't include Libs.private or Requires.private, so I can't use pkg-config to figure out what those link flags should be.

Yannis: We can still test "everything shared" in the CI. It's just that the dependent libraries such as libaom and dav1d cannot be locally built (in the ext directory) and need to be installed as packages.

Frankie: I am not familiar with pkg-config, but I think libavif.pc should imitate how libpng's .pc file handles the zlib dependency (assuming libpng does it correctly):
https://github.com/glennrp/libpng/blob/libpng16/libpng.pc.in

@fdintino, just to be sure: you were building libavif as static, with dynamic deps, installing it and using pkgconfig to use it? (not CMake)

I was building libavif with local dependencies, which are static. When you do this, avifenc and avifdec work fine because they have the codecs statically linked against them. But the libavif.a file cannot be linked against on its own because it's missing the symbols for the codec libraries. cmake doesn't provide anything built-in to combine static libraries, but various open source libraries have created some cmake functions to support cross-platform archive bundling. I'm looking into that, but I think that would be a separate pull request.

Closing the loop on this conversation: as I noted in a comment on the PR itself, I've opted to include the commit that fixes the static library bundling as an additional commit here.

ext/libyuv.cmd

wantehchang · 2023-09-25T23:31:45Z

CMakeLists.txt

@@ -162,7 +160,7 @@ if(AVIF_LOCAL_LIBYUV)
    if(DEFINED ANDROID_ABI)
        set(AVIF_LIBYUV_BUILD_DIR "${AVIF_LIBYUV_BUILD_DIR}/${ANDROID_ABI}")
    endif()
-    set(LIB_FILENAME "${AVIF_LIBYUV_BUILD_DIR}/${AVIF_LIBRARY_PREFIX}yuv${AVIF_LIBRARY_SUFFIX}")
+    set(LIB_FILENAME "${AVIF_LIBYUV_BUILD_DIR}/${AVIF_LIBRARY_PREFIX}yuv${CMAKE_STATIC_LIBRARY_SUFFIX}")


Yannis: We can still test "everything shared" in the CI. It's just that the dependent libraries such as libaom and dav1d cannot be locally built (in the ext directory) and need to be installed as packages.

Frankie: I am not familiar with pkg-config, but I think libavif.pc should imitate how libpng's .pc file handles the zlib dependency (assuming libpng does it correctly):
https://github.com/glennrp/libpng/blob/libpng16/libpng.pc.in

.github/workflows/ci-unix-shared-local.yml

ext/libyuv.cmd

.github/workflows/ci-unix-shared-local.yml

ext/aom.cmd

fdintino · 2023-09-29T01:45:25Z

Following up on the discussion about statically linking against libavif: I've created a branch adding a post build command in cmake that merges any platform and codec static libraries into libavif.a / avif.lib. I've also added a step in the CI static build workflows to test that an executable can be built without having to link against any local static dependencies. And I've pushed out a separate branch which includes those tests but doesn't include my cmake changes to demonstrate the issue I'm trying to fix; here are the failing CI Unix Static and CI Windows runs.

Depending on how folks feel about those changes, I'd be happy to open them in a separate pull request. Or I could add that as a commit to this PR.

vrabaud · 2023-09-29T12:06:33Z

Thx a lot @fdintino for all the work: I looked at your other PR and it does look right. It is weird that CMake does not bundle static libraries by default.

fdintino · 2023-09-29T12:47:31Z

@vrabaud okay, I've gone ahead and added that commit to this pull request. Thanks!

vrabaud · 2023-10-02T14:22:18Z

cmake/Modules/merge_static_libs.cmake

@@ -0,0 +1,65 @@
+function(merge_static_libs target)


I checked online and indeed, there is no CMake way of doing it. This macro is a very complete way of doing it. https://stackoverflow.com/questions/37924383/combining-several-static-libraries-into-one-using-cmake

vrabaud · 2023-10-02T14:23:04Z

CMakeLists.txt

+# A macro that wraps find_package, preferring static libraries if this is a static build
+# Should only be used for libraries linked against avif, not for those only used
+# by avif_apps or tests
+macro(find_package_libavif)


Similarly, there is not official way of doing this in CMake, looking for extensions is the current right way of doing it.

vrabaud

Thx, I believe you answered all of our requests and your CMake solutions are the right ones. Thank you!

wantehchang · 2023-10-03T22:34:17Z

CMakeLists.txt

            set(AVIF_CODEC_SYSTEM_INCLUDES ${AVIF_CODEC_SYSTEM_INCLUDES} ${RAV1E_INCLUDE_DIR})
        endif()
        set(AVIF_CODEC_LIBRARIES ${AVIF_CODEC_LIBRARIES} ${RAV1E_LIBRARIES})
    endif()

    # Unfortunately, rav1e requires a few more libraries
    if(WIN32)
-        set(AVIF_PLATFORM_LIBRARIES ${AVIF_PLATFORM_LIBRARIES} ws2_32.lib bcrypt.lib userenv.lib ntdll.lib)
+        set(AVIF_PLATFORM_LIBRARIES ${AVIF_PLATFORM_LIBRARIES} legacy_stdio_definitions.lib ntdll.lib advapi32.lib userenv.lib ws2_32.lib bcrypt.lib msvcrt.lib)


@fdintino Why were legacy_stdio_definitions.lib, advapi32.lib, and msvcrt.lib added?

I had missing symbols errors without them. I got this list from the windows rav1e.pc Libs.private

fdintino force-pushed the ci/shared-local-static branch from 4783dcb to 3292b32 Compare September 12, 2023 12:36

fdintino force-pushed the ci/shared-local-static branch 2 times, most recently from dd71c2e to 21b9384 Compare September 22, 2023 16:33

vrabaud requested a review from wantehchang September 25, 2023 11:33

wantehchang requested a review from y-guyon September 25, 2023 14:18

wantehchang reviewed Sep 25, 2023

View reviewed changes

ext/libyuv.cmd Outdated Show resolved Hide resolved

fdintino added 2 commits September 25, 2023 10:52

ci: link shared library build against static local dependencies

2be33e2

Undo ext cache key changes in GHA workflows

a7d0823

fdintino force-pushed the ci/shared-local-static branch from 21b9384 to a7d0823 Compare September 25, 2023 14:58

fix: -DCMAKE_POSITION_INDEPENDENT_CODE=ON for all cmake static builds

ff25eab

wantehchang reviewed Sep 25, 2023

View reviewed changes

fdintino added 2 commits September 26, 2023 09:17

Remove -DBUILD_SHARED_LIBS=OFF From libyuv build scripts

a213343

Remove PIC cmake flag for libxml2 and libsharpyuv

311cd3b

wantehchang reviewed Sep 26, 2023

View reviewed changes

.github/workflows/ci-unix-shared-local.yml Show resolved Hide resolved

ext/aom.cmd Outdated Show resolved Hide resolved

fdintino added 2 commits September 26, 2023 17:39

s/CMAKE_POSITION_INDEPENDENT_CODE/CONFIG_PIC/ in aom+avm cmd

dacb3d5

ci: merge static dependencies when building static libavif

f9e5dd7

fdintino changed the title ~~build: link against static local dependencies when building a shared library~~ build: static compile local dependencies; combine library archives when building static libavif Sep 29, 2023

vrabaud reviewed Oct 2, 2023

View reviewed changes

vrabaud approved these changes Oct 2, 2023

View reviewed changes

vrabaud merged commit 12e0666 into AOMediaCodec:main Oct 2, 2023
16 checks passed

fdintino mentioned this pull request Oct 2, 2023

Add AVIF plugin (decoder + encoder using libavif) python-pillow/Pillow#5201

Open

wantehchang mentioned this pull request Oct 3, 2023

mingw-w64: legacy_stdio_definitions.lib does not exist #1647

Closed

wantehchang reviewed Oct 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: static compile local dependencies; combine library archives when building static libavif #1579

build: static compile local dependencies; combine library archives when building static libavif #1579

fdintino commented Sep 12, 2023

vrabaud commented Sep 12, 2023

fdintino commented Sep 22, 2023

vrabaud commented Sep 25, 2023

wantehchang left a comment

wantehchang Sep 25, 2023

y-guyon Sep 25, 2023

fdintino Sep 25, 2023

wantehchang Sep 25, 2023

vrabaud Sep 26, 2023

fdintino Sep 26, 2023

fdintino Sep 29, 2023

wantehchang Sep 25, 2023

fdintino commented Sep 29, 2023 •

edited

Loading

vrabaud commented Sep 29, 2023

fdintino commented Sep 29, 2023

vrabaud Oct 2, 2023

vrabaud Oct 2, 2023

vrabaud left a comment

wantehchang Oct 3, 2023

fdintino Oct 3, 2023

build: static compile local dependencies; combine library archives when building static libavif #1579

build: static compile local dependencies; combine library archives when building static libavif #1579

Conversation

fdintino commented Sep 12, 2023

vrabaud commented Sep 12, 2023

fdintino commented Sep 22, 2023

vrabaud commented Sep 25, 2023

wantehchang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdintino commented Sep 29, 2023 • edited Loading

vrabaud commented Sep 29, 2023

fdintino commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrabaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdintino commented Sep 29, 2023 •

edited

Loading