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

Remove tests against hidden libwebp symbols in link_webp_test1.cc #4525

Merged
merged 5 commits into from
Nov 20, 2023

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Nov 19, 2023

link_webp_test1.cc currently tests linking against a few internal-only symbols of the webp library. This sort of works with a static webp, but fails for a shared version.

Here's what objdump shows for these symbols in the latest version of libwebp.

Static:

objdump -t ~/.conan2/p/libwe0ab986b34fa07/p/lib/*.a | grep -P 'WebPGetDemuxVersion|WebPGetMuxVersion|WebPAllocateDecBuffer|WebPFreeDecBuffer|VP8LNew|VP8LClear' | grep -v '*UND*' | sort -u
0000000000000000 g     F .text	0000000000000006 WebPGetMuxVersion
00000000000000f0 g     F .text	0000000000000428 .hidden WebPAllocateDecBuffer
0000000000000570 g     F .text	000000000000003b WebPFreeDecBuffer
0000000000000a40 g     F .text	0000000000000031 .hidden VP8LClearBackwardRefs
0000000000000bb0 g     F .text	0000000000000006 WebPGetDemuxVersion
0000000000002860 g     F .text	000000000000002f .hidden VP8LNew
0000000000002890 g     F .text	0000000000000101 .hidden VP8LClear

Shared:

objdump -T ~/.conan2/p/libwed5d42768c6b93/p/lib/*.so | grep -P 'WebPGetDemuxVersion|WebPGetMuxVersion|WebPAllocateDecBuffer|WebPFreeDecBuffer|VP8LNew|VP8LClear' | LC_COLLATE=C sort -u
0000000000002780 g    DF .text	0000000000000006  Base        WebPGetDemuxVersion
0000000000002da0 g    DF .text	000000000000003b  Base        WebPFreeDecBuffer
0000000000003f10 g    DF .text	000000000000003b  Base        WebPFreeDecBuffer
00000000000064a0 g    DF .text	0000000000000006  Base        WebPGetMuxVersion

For context, this cropped up in a PR for adding a TileDB recipe to the Conan Center Index: conan-io/conan-center-index#19381


TYPE: BUG
DESC: Fix broken linking tests against a shared WebP library

@teo-tsirpanis
Copy link
Member

this cropped up in a PR for adding a TileDB recipe to the Conan Center Index

Curious how it appeared. The Conan PR seems to disable tests and link_webp_test1.cc should not have been compiled.

@valgur
Copy link
Contributor Author

valgur commented Nov 20, 2023

this cropped up in a PR for adding a TileDB recipe to the Conan Center Index

Curious how it appeared. The Conan PR seems to disable tests and link_webp_test1.cc should not have been compiled.

@teo-tsirpanis This test does not appear to be hidden behind any CMake variables and CMakeLists.txt always applies enable_testing(). Probably should make enable_testing() contional on TILEDB_TESTS.

@teo-tsirpanis
Copy link
Member

Hmm, the sources for the test are defined in tiledb/sm/compressors/test but the CMake target is in tiledb/sm/compressors. The add_test_subdirectory command takes care of not including the test subdirectories if tests are not enabled. Will take care of it, thanks for noticing!

Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

LGTM if CI succeeds. Thanks for the contribution!

@KiterLuc KiterLuc merged commit 2f7361c into TileDB-Inc:dev Nov 20, 2023
52 checks passed
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.

3 participants