-
Notifications
You must be signed in to change notification settings - Fork 188
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
build: static compile local dependencies; combine library archives when building static libavif #1579
Conversation
4783dcb
to
3292b32
Compare
I'd also be in favor of forcing static linkage to simplify CMake (which you already did). |
dd71c2e
to
21b9384
Compare
Any updates on this? |
Still in favor: If we delete a folder in ext by inadvertence, the binaries still use the right versions of the deps (the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
@@ -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}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yannis: Could you review this pull request? It reverts some of the changes you made in #1169.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fdintino, just to be sure: you were building libavif as static, with dynamic deps, installing it and using pkgconfig to use it? (not CMake)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
21b9384
to
a7d0823
Compare
@@ -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}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
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. |
@vrabaud okay, I've gone ahead and added that commit to this pull request. Thanks! |
@@ -0,0 +1,65 @@ | |||
function(merge_static_libs target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
# 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, there is not official way of doing this in CMake, looking for extensions is the current right way of doing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, I believe you answered all of our requests and your CMake solutions are the right ones. Thank you!
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fdintino Why were legacy_stdio_definitions.lib, advapi32.lib, and msvcrt.lib added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had missing symbols errors without them. I got this list from the windows rav1e.pc Libs.private
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 toldd
(to show the missing links) andavifenc --help
(job output)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.