Skip to content

Conversation

@BewareMyPower
Copy link
Contributor

Motivation

Currently when LINK_STATIC is ON, Python client will still find libpulsar.a, which doesn't include the 3rd party dependencies of C++ client. In this case, we have to find those libraries.

A simple way is to link statically to libpulsarwithdeps.a, which already contains all the dependencies of C++ client.

Modifications

Find libpulsarwithdeps.a when LINK_STATIC is ON. For macOS, the statically linked libcurl depends on the Foundation and SystemConfiguration frameworks, so the related compile options are added.

@BewareMyPower BewareMyPower self-assigned this Oct 12, 2022
@BewareMyPower BewareMyPower marked this pull request as draft October 12, 2022 03:26
@BewareMyPower BewareMyPower force-pushed the bewaremypower/build-macos-intel branch from 6775af7 to 2a797a3 Compare October 12, 2022 04:56
@BewareMyPower BewareMyPower marked this pull request as ready for review October 12, 2022 04:56
### Motivation

Currently when LINK_STATIC is ON, Python client will still find
libpulsar.a, which doesn't include the 3rd party dependencies of C++
client. In this case, we have to find those libraries.

A simple way is to link statically to libpulsarwithdeps.a, which already
contains all the dependencies of C++ client.

### Modifications

Find libpulsarwithdeps.a when LINK_STATIC is ON. For macOS, the
statically linked libcurl depends on the Foundation and
SystemConfiguration frameworks, so the related compile options are
added.

In addition, a `LINK_OPENSSL` option is also added because the old C++
client doesn't link to OpenSSL statically so that `libpulsarwithdeps.a`
doesn't contain OpenSSL. For new C++ client releases, there is no need
to enable this option.
@BewareMyPower BewareMyPower force-pushed the bewaremypower/build-macos-intel branch from 091f45e to bd19c63 Compare October 12, 2022 07:11
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
#
Copy link
Contributor Author

@BewareMyPower BewareMyPower Oct 12, 2022

Choose a reason for hiding this comment

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

This script can be used for both CI verification and local development environment establishment. In future, we can replace this script with vcpkg.

@BewareMyPower BewareMyPower added this to the 3.0.0 milestone Oct 12, 2022
@BewareMyPower
Copy link
Contributor Author

@merlimat Let's continue the discussion here. I don't understand well about the advantage of packaging both libpulsar.so and libboost_python.so (than including libboost_python.a and libpulsarwithdeps.a).

If we use the .a, everything is visible and subject to symbols conflict (eg if you load another Python extension that uses this symbols)

cd $ROOT_DIR

CACHE_DIR=~/.pulsar-python-deps
if [[ $# -le 1 ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if [[ $# -le 1 ]]; then
if [[ $# -ge 1 ]]; then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry I forgot to push latest commits. I found this issue as well.

@BewareMyPower BewareMyPower marked this pull request as draft October 13, 2022 18:33
@BewareMyPower BewareMyPower deleted the bewaremypower/build-macos-intel branch November 2, 2022 04:05
@BewareMyPower BewareMyPower removed this from the 3.0.0 milestone Dec 14, 2022
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