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

CMake: link curl to its dependencies with PRIVATE #9125

Closed
wants to merge 1 commit into from

Conversation

rouault
Copy link
Contributor

@rouault rouault commented Jul 8, 2022

The current PUBLIC visibility causes issues for downstream users.
Cf OSGeo/PROJ#3172 (comment)

lib/CMakeLists.txt Outdated Show resolved Hide resolved
The current PUBLIC visibility causes issues for downstream users.
Cf OSGeo/PROJ#3172 (comment)
@rouault rouault force-pushed the cmake_private_link_libraries branch from a98f552 to dd7460c Compare Jul 10, 2022
@rouault rouault changed the title CMake: for shared library builds, link curl to its dependencies with PRIVATE CMake: link curl to its dependencies with PRIVATE Jul 10, 2022
@dfandrich
Copy link
Collaborator

@dfandrich dfandrich commented Jul 10, 2022

@dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Jul 10, 2022

Is this the equivalent of the autoconf option --enable-symbol-hiding ...

No. It is not on this level. It is more like pkg-config's Libs.private (for link libraries) or Requires.private (for targets).

Copy link
Contributor

@jzakrzewski jzakrzewski left a comment

Seems correct.
Before the change we actually got unnecessary overlinking when using with FetchContent. Since libcurl does not use any symbols from 3-party libraries in its interface this is the way to go.

Note this may break some projects that assumed that libcurl will pull those dependencies in, but they should have never assume it in the first place and link, what they use.

@bagder bagder closed this in 0525614 Jul 12, 2022
@bagder
Copy link
Member

@bagder bagder commented Jul 12, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants