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

[libpng] Remove -DPNG_PREFIX=a option when apng feature enabled #40280

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

ADKaster
Copy link
Contributor

@ADKaster ADKaster commented Aug 5, 2024

This option has the effect of manipulating the ELF version and symbol scripts applied to the shared library to define symbols like "apng_read_end@@PNG_16_0". When using a fully shared setup and linking other system libraries (for example, Qt6) in an application, this causes system libraries that were linked against the system libpng.so to fail to find libpng symbols. Because they have a prefix of "a".

This should have been removed in the merge from two separate ports into one port with a feature in
ff6a725

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

An example of the failure when using VCPKG_LIBRARY_LINKAGE of 'dynamic' is like so:

/home/andrew/ladybird-org/ladybird-browser/Build/ladybird/libexec/RequestServer: symbol lookup error: /lib/x86_64-linux-gnu/libcairo.so.2: undefined symbol: png_read_end, version PNG16_0

Because the lib as created by vcpkg does not export that symbol (png_read_end@@PNG16_0) but instead a host of versioned symbols with the name "apng_*"

This option has the effect of manipulating the ELF version and
symbol scripts applied to the shared library to define symbols like
"apng_read_end@@PNG_16_0". When using a fully shared setup and
linking other system libraries (for example, Qt6) in an application,
this causes system libraries that were linked against the system
libpng.so to fail to find libpng symbols. Because they have a
prefix of "a".

This should have been removed in the merge from two separate ports
into one port with a feature in
ff6a725
@dg0yt
Copy link
Contributor

dg0yt commented Aug 5, 2024

/home/andrew/ladybird-org/ladybird-browser/Build/ladybird/libexec/RequestServer: symbol lookup error: /lib/x86_64-linux-gnu/libcairo.so.2: undefined symbol: png_read_end, version PNG16_0

Sidenote: satisfying a system lib's dependencies with vcpkg libs could be error-prone. A cairo port exists.

@ADKaster
Copy link
Contributor Author

ADKaster commented Aug 5, 2024

Sidenote: satisfying a system lib's dependencies with vcpkg libs could be error-prone. A cairo port exists.

Fair enough, but adding qt6 libraries and its dependencies to our vcpkg config at this point doesn't make much sense for our project. we don't need any customization to qt, and building it from source without some kind of shared binary cache for contributors to use sounds like a massive hassle and waste of compute.

For an actual distribution build, we will likely ship vcpkg dependencies as static libraries instead of dylibs. And distros have shown already that they have no interest in integrating our vcpkg config into their setup, and just want us to keep the option for satisfying all libs with system dependencies in our CMake.

that is to say, if it works for a development build on a developer's machine and doesn't explode, 🤷

@Cheney-W Cheney-W added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Aug 5, 2024
@Cheney-W
Copy link
Contributor

Cheney-W commented Aug 5, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Aug 7, 2024
@data-queue data-queue merged commit f5398d9 into microsoft:master Aug 8, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants