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.

Sign up for GitHub

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

Jump to bottom

[pulsar-client-cpp] New port #35579

Merged
merged 13 commits into from
Dec 29, 2023
Merged

[pulsar-client-cpp] New port #35579

merged 13 commits into from
Dec 29, 2023

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Dec 9, 2023 •
edited
Loading

  • Changes comply with the maintainer guide
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

Regarding the patch

  1. The upstream uses lots of raw find_path/find_libraries calls to add a path of a library to the COMMON_LIBS list, this patch replaces all of them with the find_package calls and adds the target to the COMMON_LIBS list.
  2. The upstream allows user passing a PROTOC_PATH or find it from the system path, then runs a shell command to generate the protobuf source files. This patch uses the protobuf_generate function instead.

@BewareMyPower BewareMyPower marked this pull request as draft December 9, 2023 13:19
@BewareMyPower
Copy link
Contributor Author

@microsoft-github-policy-service agree

@BewareMyPower
Copy link
Contributor Author

Is there anyway I can check the error logs in CI? e.g. the error logs on x64_linux is /mnt/vcpkg-ci/buildtrees/pulsar-client-cpp/install-x64-linux-dbg-out.log but I cannot find a way to download it.

@dg0yt
Copy link
Contributor

dg0yt commented Dec 9, 2023

Is there anyway I can check the error logs in CI? e.g. the error logs on x64_linux is /mnt/vcpkg-ci/buildtrees/pulsar-client-cpp/install-x64-linux-dbg-out.log but I cannot find a way to download it.

#31357 (comment)

@dg0yt
Copy link
Contributor

dg0yt commented Dec 9, 2023 •
edited
Loading

If the cmake config is not provided/merged by upstream, it has to be prefixed with unofficial.
https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#add-cmake-exports-in-an-unofficial--namespace

Learn to use find_package() / find_dependency(). For static linkage, you need all libs transitively, on non-MSVC plaforms also in the right order. It is not maintainable with this find_library() approach. With packages, you should only have to care about direct dependencies.

@BewareMyPower
Copy link
Contributor Author

I see. I will fix it soon.

@jimwang118 jimwang118 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Dec 11, 2023
@BewareMyPower BewareMyPower force-pushed the add-pulsar-port-complete branch 2 times, most recently from 9735a3a to 0d049cd Compare December 11, 2023 03:16
@BewareMyPower
Copy link
Contributor Author

I renamed the .cmake file and updated the target name to mark them as unofficial. For 3rd party dependencies, I used find_package now.

Regarding the Windows and Linux build failures, they seem related to the official way to find protoc. I'm trying to add a patch for it.

@BewareMyPower BewareMyPower force-pushed the add-pulsar-port-complete branch 5 times, most recently from 942a93b to 9c45e32 Compare December 11, 2023 17:25
@BewareMyPower BewareMyPower marked this pull request as ready for review December 11, 2023 17:50
@BewareMyPower BewareMyPower marked this pull request as draft December 11, 2023 17:54
@BewareMyPower BewareMyPower marked this pull request as ready for review December 11, 2023 18:13
@BewareMyPower
Copy link
Contributor Author

@jimwang118 Now all tests passed, could you take a look?

@jimwang118
Copy link
Contributor

Usage test pass with following triplets:

x64-windows
x64-windows-static

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Dec 13, 2023
vicroms
vicroms requested changes Dec 14, 2023
View reviewed changes
ports/pulsar-client-cpp/unofficial-pulsar-config.cmake Outdated Show resolved Hide resolved
ports/pulsar-client-cpp/portfile.cmake Outdated Show resolved Hide resolved
@vicroms vicroms marked this pull request as draft December 14, 2023 07:54
@BewareMyPower
Address review comments
57717c9 
- Use find_dependency instead of find_package
- Specify BUILD_DYNAMIC_LIB explicitly
@jimwang118 jimwang118 removed the info:reviewed Pull Request changes follow basic guidelines label Dec 14, 2023
@BewareMyPower BewareMyPower marked this pull request as ready for review December 14, 2023 15:48
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

jimwang118
jimwang118 requested changes Dec 19, 2023
View reviewed changes
versions/p-/pulsar-client-cpp.json Outdated Show resolved Hide resolved
@jimwang118
Copy link
Contributor

Note: I will be converting your PR to draft status. The above suggested changes are only recommendations. If you are willing to adopt them, please click "ready for review" after making the modifications. If you do not wish to make any changes, please click "ready for review" directly. That way, I can be aware that you've responded since you can't modify the tags.

@jimwang118 jimwang118 marked this pull request as draft December 19, 2023 01:56
@BewareMyPower BewareMyPower marked this pull request as ready for review December 19, 2023 02:47
jimwang118
jimwang118 previously approved these changes Dec 19, 2023
View reviewed changes
@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Dec 19, 2023
dg0yt
dg0yt reviewed Dec 19, 2023
View reviewed changes
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

I still think it would be better to export the cmake targets. This immediately solves the problems for multi-config generators.

ports/pulsar-client-cpp/unofficial-pulsar-config.cmake Outdated Show resolved Hide resolved
ports/pulsar-client-cpp/unofficial-pulsar-config.cmake Outdated Show resolved Hide resolved
ports/pulsar-client-cpp/unofficial-pulsar-config.cmake Outdated Show resolved Hide resolved
ports/pulsar-client-cpp/unofficial-pulsar-config.cmake Outdated Show resolved Hide resolved
ports/pulsar-client-cpp/unofficial-pulsar-config.cmake Outdated Show resolved Hide resolved
ports/pulsar-client-cpp/unofficial-pulsar-config.cmake Outdated Show resolved Hide resolved
ports/pulsar-client-cpp/unofficial-pulsar-config.cmake Outdated Show resolved Hide resolved
ports/pulsar-client-cpp/unofficial-pulsar-config.cmake Outdated Show resolved Hide resolved
ports/pulsar-client-cpp/unofficial-pulsar-config.cmake Outdated Show resolved Hide resolved
@BewareMyPower BewareMyPower marked this pull request as draft December 19, 2023 06:35
@jimwang118 jimwang118 removed the info:reviewed Pull Request changes follow basic guidelines label Dec 19, 2023
@BewareMyPower BewareMyPower marked this pull request as ready for review December 19, 2023 13:28
@BewareMyPower BewareMyPower marked this pull request as draft December 19, 2023 13:42
@BewareMyPower BewareMyPower marked this pull request as ready for review December 19, 2023 14:02
@BewareMyPower
Copy link
Contributor Author

Hi @dg0yt I have addressed your comments and tested with x64-windows and x64-windows-static triplets for both debug and release builds, PTAL again.

BTW, I found the result of find_library varies for the CMAKE_BUILD_TYPE variable. For example, when it's Debug, the result will be debug/lib/pulsar.lib or debug/lib/pulsar-static.lib. When it's Release, the result becomes lib/pulsar.lib or lib/pulsar-static.lib. So I used CMAKE_IGNORE_PATH to ignore the debug/ path. Not sure if there is a better solution.

@BewareMyPower
Copy link
Contributor Author

ping @dg0yt @jimwang118 @vicroms

@BewareMyPower
Copy link
Contributor Author

It has been pending for over a week. Could anyone take a look?

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Dec 28, 2023
@vicroms vicroms merged commit dc19823 into microsoft:master Dec 29, 2023
15 checks passed
@BewareMyPower BewareMyPower deleted the add-pulsar-port-complete branch December 29, 2023 11:44
Osyotr pushed a commit to Osyotr/vcpkg that referenced this pull request Jan 23, 2024
@BewareMyPower @Osyotr
[pulsar-client-cpp] New port (microsoft#35579)
5c88272 
* [pulsar-client-cpp] New port

* Remove unnecessary link options

* Address review comments

- Use find_dependency instead of find_package
- Specify BUILD_DYNAMIC_LIB explicitly

* Fix the Windows build and linkage

* Fix dynamic library not built when VCPKG_LIBRARY_LINKAGE is dynamic on Windows

* Fix Linux and OSX failures

* Remove PULSAR_FORCE_DYNAMIC_LIBRARY and upgrade version to 3.4.2

* Reduce the changes to the upstream CMakeLists.txt

* Remove unused version

* Optimize finding and linking dependency and patch the header for static library

* Support multi-config generators

* Fix path for release libraries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vicroms vicroms vicroms approved these changes

@jimwang118 jimwang118 jimwang118 left review comments

@dg0yt dg0yt Awaiting requested review from dg0yt

Assignees

@jimwang118 jimwang118

Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@BewareMyPower @dg0yt @jimwang118 @BillyONeal @vicroms