Support linking static dependencies when building with MSVC#73
Merged
merlimat merged 7 commits intoapache:mainfrom Oct 29, 2022
Merged
Conversation
Contributor
|
Do we need to support LINK_STATIC when releasing windows artifacts? |
Member
I think need it. |
Contributor
Author
|
@yaalsn Yes. I think we'd better provide a package whose structure is like: |
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-cpp
that referenced
this pull request
Oct 26, 2022
* Support linking static dependencies when building with MSVC See apache#73 * Add Windows packaging
Contributor
Author
|
FYI, I'm also working on the Windows packaging CI based on this PR. https://github.com/BewareMyPower/pulsar-client-cpp/actions/runs/3328649258 |
Member
Would you add this to this PR? |
Contributor
Author
|
@shibd I'm debugging the packaging workflow. There is still a problem. I think both including it in this PR or opening another PR are okay. |
### Motivation Currently it's impossible to build Pulsar C++ client on Windows with `LINK_STATIC=ON`. It means users have to package all 3rd-party DLLs as well as `pulsar.dll`, which harms the experience. ### Modifications Enable `LINK_STATIC` when the Vcpkg triplet is `xxx-static`. In this case, find the 3rd party libraries with correct names on Windows. And replace `Threads::Threads` with `CMAKE_THREAD_LIB_INIT`. The most important change is replacing the `/MD` compile option with `/MT`. It should have been done by setting the [`MSVC_RUNTIME_LIBRARY`](https://cmake.org/cmake/help/latest/prop_tgt/MSVC_RUNTIME_LIBRARY.html) property, but it seems not work. So this PR just modifies the `CMAKE_CXX_FLAGS_<CONFIG>` variables. For `pulsarWithAllDeps.lib`, add the actual library (`*.lib`) to `COMMON_LIBS` instead of the target name (`dlfcn-win32::dl`). Some warnings on Windows caused by incorrect compile options are fixed as well. A workflow is added to verify the static build for x64 and x86 Windows. And a simple example is added as `win-examples` to show the `pulsarWithAllDeps.lib` can be linked without any other dependency to run an executable. Change the existing release workflow to release two `*.zip` files: - pulsar-client-cpp-x64-windows-static.zip - pulsar-client-cpp-x86-windows-static.zip Each zip file consists of: ``` bin/pulsar.dll - The dynamic library that links statically to dependencies include/pulsar/ - Headers lib/ pulsar.lib - The import library of pulsar.dll pulsar-static.lib - The static library pulsarWithDeps.lib - The static library with all dependnecies included dependencies.txt - The vcpkg outputs, which contains the dependency versions ```
a170f9a to
39b4f5d
Compare
Member
|
@BewareMyPower @merlimat Can this PR go into 3.0? The packaging of the Windows Node js client depends on this. |
Contributor
Author
|
Mark it as draft for adding support for debug build. Waiting for #75 being merged first. |
Contributor
Author
|
Hi @merlimat @yaalsn @Demogorgon314 @RobertIndie , I made some changes just now and this PR is ready to build debug version libraries. So I think #75 can be closed by this PR. Could you take a look at this PR? In addition, this PR also reverts changes of #72, please also take a look. |
Contributor
Author
Contributor
Author
|
There is something wrong with the binary upload, I will fix it soon. |
shibd
approved these changes
Oct 28, 2022
Contributor
Author
|
The example WIndows artifacts built by the new workflow can be found here: https://github.com/BewareMyPower/pulsar-client-cpp/actions/runs/3342437635 |
4 tasks
Contributor
Author
merlimat
approved these changes
Oct 29, 2022
2 tasks
BewareMyPower
added a commit
that referenced
this pull request
Nov 1, 2022
* Support linking static dependencies when building with MSVC ### Motivation Currently it's impossible to build Pulsar C++ client on Windows with `LINK_STATIC=ON`. It means users have to package all 3rd-party DLLs as well as `pulsar.dll`, which harms the experience. ### Modifications Enable `LINK_STATIC` when the Vcpkg triplet is `xxx-static`. In this case, find the 3rd party libraries with correct names on Windows. And replace `Threads::Threads` with `CMAKE_THREAD_LIB_INIT`. The most important change is replacing the `/MD` compile option with `/MT`. It should have been done by setting the [`MSVC_RUNTIME_LIBRARY`](https://cmake.org/cmake/help/latest/prop_tgt/MSVC_RUNTIME_LIBRARY.html) property, but it seems not work. So this PR just modifies the `CMAKE_CXX_FLAGS_<CONFIG>` variables. For `pulsarWithAllDeps.lib`, add the actual library (`*.lib`) to `COMMON_LIBS` instead of the target name (`dlfcn-win32::dl`). Some warnings on Windows caused by incorrect compile options are fixed as well. A workflow is added to verify the static build for x64 and x86 Windows. And a simple example is added as `win-examples` to show the `pulsarWithAllDeps.lib` can be linked without any other dependency to run an executable. Change the existing release workflow to release two `*.zip` files: - pulsar-client-cpp-x64-windows-static.zip - pulsar-client-cpp-x86-windows-static.zip Each zip file consists of: ``` bin/pulsar.dll - The dynamic library that links statically to dependencies include/pulsar/ - Headers lib/ pulsar.lib - The import library of pulsar.dll pulsar-static.lib - The static library pulsarWithDeps.lib - The static library with all dependnecies included dependencies.txt - The vcpkg outputs, which contains the dependency versions ``` * Support Debug build and upload debug binaries * Fix $ is missing * Fix apt-get install failure * Remove default LINK_STATIC build * Use upload-artifact for Linux packages (cherry picked from commit 93a44c8)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Currently it's impossible to build Pulsar C++ client on Windows with
LINK_STATIC=ON. It means users have to package all 3rd-party DLLs as well aspulsar.dll, which harms the experience.Modifications
Enable
LINK_STATICwhen the Vcpkg triplet isxxx-static. In this case, find the 3rd party libraries with correct names on Windows. And replaceThreads::ThreadswithCMAKE_THREAD_LIB_INIT.The most important change is replacing the
/MDcompile option with/MT. It should have been done by setting theMSVC_RUNTIME_LIBRARYproperty, but it seems not work. So this PR just modifies theCMAKE_CXX_FLAGS_<CONFIG>variables.For
pulsarWithAllDeps.lib, add the actual library (*.lib) toCOMMON_LIBSinstead of the target name (dlfcn-win32::dl).Some warnings on Windows caused by incorrect compile options are fixed as well.
A workflow is added to verify the static build for x64 and x86 Windows. And a simple example is added as
win-examplesto show thepulsarWithAllDeps.libcan be linked without any other dependency to run an executable.Change the existing release workflow to release two
*.zipfiles:Each zip file consists of: