Integrate vcpkg to manage dependencies for all platforms #371
Conversation
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
BewareMyPower
requested review from
Demogorgon314,
RobertIndie,
merlimat and
shibd
BewareMyPower
force-pushed
the
bewaremypower/vcpkg-integration
branch
2 times, most recently
from
2824d10 to
a005a6b
Compare
BewareMyPower
force-pushed
the
bewaremypower/vcpkg-integration
branch
from
a005a6b to
29b857b
Compare
…nux, macOS) ### Motivation Currently we manage dependencies by ourselves: - When running unit tests in CI, dependencies are installed from the Debian package management tool `apt` - When verifying macOS build, dependencies are installed from the Homebrew package manager - When verifying Windows build, dependencies are installed via vcpkg but no versions are specified - When building pre-built binaries to release, dependencies are installed by scripts under `pkg/` directories. These scripts download the source code according to `dependencies.yaml` and build It makes the pre-built binaries never tested except for the simple manual tests when voting for a new release. As a result, regression like apache#363 could happen. This patch aims at integrating vcpkg to manage dependencies for all platforms, not only for Windows. ### Modifications Integrate the CMakeLists.txt with vcpkg by: 1. Introduce vcpkg as a submodule of this project 2. Update `vcpkg.json` to specify dependency versions. 3. Set `CMAKE_TOOLCHAIN_FILE` with the `vcpkg.cmake` in the submodule. Then, we can simply use `find_package` to find all dependencies and depend on them via a target like `CURL::libcurl` to have all include directores and link libraries. Update the `unit-tests` workflow to verify building the binaries via vcpkg to test. The README is also updated to show how much simpler to build with vcpkg now. ### TODO There are still some tasks that cannot be done by vcpkg: 1. The static library (e.g. `libpulsarwithdeps.a`) that bundles all 3rd-party dependencies. 2. The pre-built binaries are still built by scripts under `./pkg` directory. Specifically, vcpkg does not work with GCC 4.8 so on CentOS 7 we still need to build dependencies manually. So the previous CMakeLists.txt are retained and will be used if `INTEGRATE_VCPKG` is OFF (by default). They will be removed until the task above can be done by vcpkg in future. We also need to update `dependencies.yaml` to be consistent with `vcpkg.json` if all tasks above cannot be replaced by vcpkg.
BewareMyPower
force-pushed
the
bewaremypower/vcpkg-integration
branch
from
f5223e0 to
5244eed
Compare
Demogorgon314
approved these changes
Dec 13, 2023
Member
Demogorgon314
left a comment
Demogorgon314
left a comment
There was a problem hiding this comment.
I tested it on M1 macOS, it works well. Great work!
Member
|
I will review it today. |
shibd
reviewed
Dec 14, 2023
| - [libcurl](https://curl.se/libcurl/) | ||
| - [openssl](https://github.com/openssl/openssl) | ||
| Since it's integrated with vcpkg, see [vcpkg#README](https://github.com/microsoft/vcpkg#readme) for the requirements. See [LEGACY_BUILD](./LEGACY_BUILD.md) if you want to manage dependencies by yourself or you cannot install vcpkg in your own environment. | ||
|
|
Member
There was a problem hiding this comment.
We need to add a link to vcpkg.json to explain what version of is dependency
RobertIndie
approved these changes
Dec 14, 2023
BewareMyPower
commented
Dec 14, 2023
shibd
approved these changes
Dec 15, 2023
BewareMyPower
added a commit
to streamnative/pulsar-client-cpp
that referenced
this pull request
Jul 4, 2024
### Motivation Currently we manage dependencies by ourselves: - When running unit tests in CI, dependencies are installed from the Debian package management tool `apt` - When verifying macOS build, dependencies are installed from the Homebrew package manager - When verifying Windows build, dependencies are installed via vcpkg but no versions are specified - When building pre-built binaries to release, dependencies are installed by scripts under `pkg/` directories. These scripts download the source code according to `dependencies.yaml` and build It makes the pre-built binaries never tested except for the simple manual tests when voting for a new release. As a result, regression like apache#363 could happen. This patch aims at integrating vcpkg to manage dependencies for all platforms, not only for Windows. ### Modifications Integrate the CMakeLists.txt with vcpkg by: 1. Introduce vcpkg as a submodule of this project 2. Update `vcpkg.json` to specify dependency versions. 3. Set `CMAKE_TOOLCHAIN_FILE` with the `vcpkg.cmake` in the submodule. Then, we can simply use `find_package` to find all dependencies and depend on them via a target like `CURL::libcurl` to have all include directores and link libraries. Update the `unit-tests` workflow to verify building the binaries via vcpkg to test. The README is also updated to show how much simpler to build with vcpkg now. ### TODO There are still some tasks that cannot be done by vcpkg: 1. The static library (e.g. `libpulsarwithdeps.a`) that bundles all 3rd-party dependencies. 2. The pre-built binaries are still built by scripts under `./pkg` directory. Specifically, vcpkg does not work with GCC 4.8 so on CentOS 7 we still need to build dependencies manually. So the previous CMakeLists.txt are retained and will be used if `INTEGRATE_VCPKG` is OFF (by default). They will be removed until the task above can be done by vcpkg in future. We also need to update `dependencies.yaml` to be consistent with `vcpkg.json` if all tasks above cannot be replaced by vcpkg. (cherry picked from commit 7baa312)
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 we manage dependencies by ourselves:
aptpkg/directories. These scripts download the source code according todependencies.yamland buildIt makes the pre-built binaries never tested except for the simple manual tests when voting for a new release. As a result, regression like #363 could happen.
This patch aims at integrating vcpkg to manage dependencies for all platforms, not only for Windows.
Modifications
Integrate the CMakeLists.txt with vcpkg by:
vcpkg.jsonto specify dependency versions.CMAKE_TOOLCHAIN_FILEwith thevcpkg.cmakein the submodule.Then, we can simply use
find_packageto find all dependencies and depend on them via a target likeCURL::libcurlto have all include directores and link libraries.Update the
unit-testsworkflow to verify building the binaries via vcpkg to test. The README is also updated to show how much simpler to build with vcpkg now.TODO
There are still some tasks that cannot be done by vcpkg:
libpulsarwithdeps.a) that bundles all 3rd-party dependencies../pkgdirectory. Specifically, vcpkg does not work with GCC 4.8 so on CentOS 7 we still need to build dependencies manually.So the previous CMakeLists.txt are retained and will be used if
INTEGRATE_VCPKGis OFF (by default). They will be removed until the task above can be done by vcpkg in future.