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

Export "selected" CMake target for zstd #3859

Closed
anthonyalayo opened this issue Jan 11, 2024 · 7 comments · Fixed by #3965
Closed

Export "selected" CMake target for zstd #3859

anthonyalayo opened this issue Jan 11, 2024 · 7 comments · Fixed by #3965

Comments

@anthonyalayo
Copy link

Can we have a consistent CMake library target exported?

For example with the curl project, if the standard CMake global variable BUILD_SHARED_LIBS is set, LIB_SELECTED will match accordingly: https://github.com/curl/curl/blob/master/CMakeLists.txt#L155-L159

# lib flavour selected for example and test programs.
if(BUILD_SHARED_LIBS)
  set(LIB_SELECTED ${LIB_SHARED})
else()
  set(LIB_SELECTED ${LIB_STATIC})
endif()

This is then used as an exported target (in addition to the fixed static/shared targets):

add_library(${LIB_NAME} ALIAS ${LIB_SELECTED})
add_library(${PROJECT_NAME}::${LIB_NAME} ALIAS ${LIB_SELECTED})

This makes pulling in curl quite clean. In comparison, I have to do the following when using fetchcontent with zstd:

set(ZSTD_LIBRARY libzstd_static)
if(BUILD_SHARED_LIBS)
  set(ZSTD_LIBRARY libzstd_shared)
endif()
@Cyan4973
Copy link
Contributor

Cyan4973 commented Mar 4, 2024

Does the recently merged #3811 by @teo-tsirpanis answer this issue ?

@teo-tsirpanis
Copy link
Contributor

Curl exposes a unified target even if both static and shared libraries are built.

@Cyan4973 should we do the same here (and in lz4)? And if both are specified, should we prefer static or shared?

@anthonyalayo
Copy link
Author

If both are specified, we should rely on whatever BUILD_SHARED_LIBS is set to. This is a global CMake variable than can only be ON or OFF, and configures all subprojects.

@teo-tsirpanis
Copy link
Contributor

The problem is that zstd does not use BUILD_SHARED_LIBS at all. 🤔

@anthonyalayo
Copy link
Author

Even if it doesn't, that's what CMake uses, so it should work out well?

terrelln added a commit to terrelln/zstd that referenced this issue Mar 14, 2024
If both `ZSTD_BUILD_SHARED` and `ZSTD_BUILD_STATIC` are set, then cmake exports the libraries `libzstd_shared` and `libzstd_static` only.
It does not export `libzstd`, which is only exported when exactly one of `ZSTD_BUILD_SHARED` and `ZSTD_BUILD_STATIC` is set.
This PR exports `libzstd` in that case, based on the value of the standard CMake variable [`BUILD_SHARED_LIBS`](https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html).
This ensures that `libzstd` can always be used to refer to the exported zstd library, since the build errors if neither `ZSTD_BUILD_SHARED` nor `ZSTD_BUILD_STATIC` are set.

I tested all the possible combinations of `ZSTD_BUILD_SHARED`, `ZSTD_BUILD_STATIC`, and `BUILD_SHARED_LIBS` and they always worked as expected:
* If only exactly one of `ZSTD_BUILD_SHARED` and `ZSTD_BUILD_STATIC` is set, that is used as `libzstd`.
* Otherwise, libzstd is set based on `BUILD_SHARED_LIBS`.

Fixes facebook#3859.
@terrelln
Copy link
Contributor

@anthonyalayo, @teo-tsirpanis please let me know if PR #3965 fixes your issues.

terrelln added a commit that referenced this issue Mar 14, 2024
If both `ZSTD_BUILD_SHARED` and `ZSTD_BUILD_STATIC` are set, then cmake exports the libraries `libzstd_shared` and `libzstd_static` only.
It does not export `libzstd`, which is only exported when exactly one of `ZSTD_BUILD_SHARED` and `ZSTD_BUILD_STATIC` is set.
This PR exports `libzstd` in that case, based on the value of the standard CMake variable [`BUILD_SHARED_LIBS`](https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html).
This ensures that `libzstd` can always be used to refer to the exported zstd library, since the build errors if neither `ZSTD_BUILD_SHARED` nor `ZSTD_BUILD_STATIC` are set.

I tested all the possible combinations of `ZSTD_BUILD_SHARED`, `ZSTD_BUILD_STATIC`, and `BUILD_SHARED_LIBS` and they always worked as expected:
* If only exactly one of `ZSTD_BUILD_SHARED` and `ZSTD_BUILD_STATIC` is set, that is used as `libzstd`.
* Otherwise, libzstd is set based on `BUILD_SHARED_LIBS`.

Fixes #3859.
@anthonyalayo
Copy link
Author

Works, thanks!

hswong3i pushed a commit to alvistack/facebook-zstd that referenced this issue Mar 27, 2024
If both `ZSTD_BUILD_SHARED` and `ZSTD_BUILD_STATIC` are set, then cmake exports the libraries `libzstd_shared` and `libzstd_static` only.
It does not export `libzstd`, which is only exported when exactly one of `ZSTD_BUILD_SHARED` and `ZSTD_BUILD_STATIC` is set.
This PR exports `libzstd` in that case, based on the value of the standard CMake variable [`BUILD_SHARED_LIBS`](https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html).
This ensures that `libzstd` can always be used to refer to the exported zstd library, since the build errors if neither `ZSTD_BUILD_SHARED` nor `ZSTD_BUILD_STATIC` are set.

I tested all the possible combinations of `ZSTD_BUILD_SHARED`, `ZSTD_BUILD_STATIC`, and `BUILD_SHARED_LIBS` and they always worked as expected:
* If only exactly one of `ZSTD_BUILD_SHARED` and `ZSTD_BUILD_STATIC` is set, that is used as `libzstd`.
* Otherwise, libzstd is set based on `BUILD_SHARED_LIBS`.

Fixes facebook#3859.
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 a pull request may close this issue.

4 participants