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: Take care of zstd static lib on Win32 #2307

Merged
merged 5 commits into from
May 6, 2019

Conversation

myd7349
Copy link
Contributor

@myd7349 myd7349 commented May 1, 2019

When built as a static library on Win32, zstd has output names like zstd_static.lib (Release) and zstd_staticd.lib (Debug):
https://github.com/facebook/zstd/blob/69baaee3e42f90dedea2c946bc19bfeac4e782ee/build/cmake/lib/CMakeLists.txt#L141
and find_library(ZSTD zstd) will not work in such cases.

When built as a static library on Win32, zstd has output names
like zstd_static.lib (Release) and zstd_staticd.lib (Debug):
https://github.com/facebook/zstd/blob/69baaee3e42f90dedea2c946bc19bfeac4e782ee/build/cmake/lib/CMakeLists.txt#L141
and find_library(ZSTD zstd) will not work in such cases.
@@ -0,0 +1,28 @@
# https://github.com/facebook/folly/blob/master/CMake/FindZstd.cmake
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to include the folly license in this directory as LICENSE.FindZstd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi! @edenhill Thanks for your review! I have added a license file for FindZstd.cmake: https://github.com/myd7349/librdkafka/blob/win32-zstd-static-lib/LICENSE.FindZstd
BTW, I cut these lines off: https://github.com/facebook/folly/blob/master/LICENSE#L180-L200

Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

Please move LICENSE.FindZstd to the packaging/cmake/... directory where FindZstd resides.
Otherwise the license will be picked up by the license generator and included in releases, which is not necessary since we're not actually shipping any code of FindZstd

@myd7349 myd7349 force-pushed the win32-zstd-static-lib branch from e4732bf to acdc8c1 Compare May 3, 2019 14:16
@myd7349 myd7349 force-pushed the win32-zstd-static-lib branch from acdc8c1 to 83d7a3e Compare May 4, 2019 01:43
@edenhill edenhill merged commit 1f3203c into confluentinc:master May 6, 2019
@edenhill
Copy link
Contributor

edenhill commented May 6, 2019

thank you!

@myd7349 myd7349 deleted the win32-zstd-static-lib branch May 6, 2019 23:35
@myd7349
Copy link
Contributor Author

myd7349 commented May 7, 2019

Hi! @edenhill Thanks!
BTW, the PR id in the commit message seems to be wrong.

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.

2 participants