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

PIP-209: Build C++ client in standalone repo #2

Merged
merged 4 commits into from
Sep 29, 2022
Merged

PIP-209: Build C++ client in standalone repo #2

merged 4 commits into from
Sep 29, 2022

Conversation

merlimat
Copy link
Contributor

Changs to build C++ client in the new standalone repo

@Demogorgon314
Copy link
Member

Demogorgon314 commented Sep 29, 2022 •
edited
Loading

Look like the build still failed. The ExternalTest.proto file is missing.

Could not make proto path relative: /Users/wangkai/Developer/github/pulsar-client-cpp/tests/proto/ExternalTest.proto: No such file or directory
gmake[2]: *** [tests/CMakeFiles/main.dir/build.make:74: generated/tests/Test.pb.cc] Error 1
gmake[2]: *** Waiting for unfinished jobs....
gmake[1]: *** [CMakeFiles/Makefile2:565: tests/CMakeFiles/main.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....
[ 67%] Building CXX object python/CMakeFiles/_pulsar.dir/src/message.cc.o
[ 68%] Building CXX object python/CMakeFiles/_pulsar.dir/src/authentication.cc.o
[ 68%] Building CXX object python/CMakeFiles/_pulsar.dir/src/reader.cc.o
[ 69%] Building CXX object python/CMakeFiles/_pulsar.dir/src/schema.cc.o
[ 69%] Building CXX object python/CMakeFiles/_pulsar.dir/src/cryptoKeyReader.cc.o
[ 70%] Building CXX object python/CMakeFiles/_pulsar.dir/src/exceptions.cc.o
[ 70%] Building CXX object python/CMakeFiles/_pulsar.dir/src/utils.cc.o
[ 71%] Linking CXX shared library _pulsar.so
ld: warning: -undefined dynamic_lookup may not work with chained fixups
[ 71%] Built target _pulsar
gmake: *** [Makefile:136: all] Error 2

@merlimat
Copy link
Contributor Author

Look like the build still failed. The ExternalTest.proto file is missing.

Yes, the build should be fixed now

@Demogorgon314
Copy link
Member

In my local env, the Version.h is not generated into include dir, so its builds failed.

[ 69%] Building CXX object tests/CMakeFiles/main.dir/VersionTest.cc.o
/Users/wangkai/Developer/github/pulsar-client-cpp/tests/VersionTest.cc:24:5: error: expected expression
    ASSERT_GE(PULSAR_VERSION, 2000000);
    ^
/usr/local/include/gtest/gtest.h:2096:32: note: expanded from macro 'ASSERT_GE'
# define ASSERT_GE(val1, val2) GTEST_ASSERT_GE(val1, val2)
                               ^
/usr/local/include/gtest/gtest.h:2072:3: note: expanded from macro 'GTEST_ASSERT_GE'
  ASSERT_PRED_FORMAT2(::testing::internal::CmpHelperGE, val1, val2)

@merlimat
Copy link
Contributor Author

In my local env, the Version.h is not generated into include dir, so its builds failed.

I had one more change that I missed to include in the commit. fixed now.

@BewareMyPower
Copy link
Contributor

It still doesn't work.

-- Pulsar Client version: 3.0.0-pre
Traceback (most recent call last):
  File "/Users/xuyunze/github.com/BewareMyPower/pulsar-client-cpp/build-support/gen-pulsar-version-macro.py", line 23, in <module>
    version = open("version.txt").read()
FileNotFoundError: [Errno 2] No such file or directory: 'version.txt'
-- Pulsar Client version macro:

BewareMyPower
BewareMyPower requested changes Sep 29, 2022
View reviewed changes
CMakeLists.txt Show resolved Hide resolved
Demogorgon314
Demogorgon314 approved these changes Sep 29, 2022
View reviewed changes
Copy link
Member

@Demogorgon314 Demogorgon314 left a comment

Choose a reason for hiding this comment

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

It can be build successfully now. 🎉

@BewareMyPower BewareMyPower merged commit 12c69f9 into apache:main Sep 29, 2022
@merlimat merlimat deleted the build-standalone-client branch September 29, 2022 05:58
BewareMyPower added a commit that referenced this pull request Sep 13, 2023
@BewareMyPower
[fix] Add the curl wrapper to avoid inconsistent curl options (#313)
787bfd0 
### Motivation

When libcurl is used in `AuthOauth2`, the `CURLOPT_NOSIGNAL` option is
not set, i.e. it will be the default value so that the
`Curl_resolv_timeout` function might crash in multi-threading
environment.

```
#2 0xf630 in _L_unlock_13 from /lib64/libpthread.so.0 (0x34)
#3 0x2e6c7f in Curl_failf from /usr/local/bin/***/libpulsar.so (0x6f)
#4 0x30a285 in Curl_resolv_timeout from /usr/local/bin/***/libpulsar.so (0x95)
```

Since there are many duplicated code when calling curl C APIs, it's hard to
notice that `CURLOPT_NOSIGNAL` is not configured in `AuthOauth2`.

### Modifications

Introduce a `CurlWrapper` class that sets the same options to reduce the
duplicated code and adapting consistent behaviors unless a few options.
BewareMyPower added a commit to streamnative/pulsar-client-cpp that referenced this pull request Sep 14, 2023
@BewareMyPower
[fix] Add the curl wrapper to avoid inconsistent curl options (apache…
57f848f 
…#313)

### Motivation

When libcurl is used in `AuthOauth2`, the `CURLOPT_NOSIGNAL` option is
not set, i.e. it will be the default value so that the
`Curl_resolv_timeout` function might crash in multi-threading
environment.

```
apache#2 0xf630 in _L_unlock_13 from /lib64/libpthread.so.0 (0x34)
apache#3 0x2e6c7f in Curl_failf from /usr/local/bin/***/libpulsar.so (0x6f)
apache#4 0x30a285 in Curl_resolv_timeout from /usr/local/bin/***/libpulsar.so (0x95)
```

Since there are many duplicated code when calling curl C APIs, it's hard to
notice that `CURLOPT_NOSIGNAL` is not configured in `AuthOauth2`.

### Modifications

Introduce a `CurlWrapper` class that sets the same options to reduce the
duplicated code and adapting consistent behaviors unless a few options.

(cherry picked from commit 787bfd0)
@jato-c8y jato-c8y mentioned this pull request Nov 30, 2023
Open
2 tasks
@erobot erobot mentioned this pull request Feb 1, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BewareMyPower BewareMyPower BewareMyPower approved these changes

@Demogorgon314 Demogorgon314 Demogorgon314 approved these changes

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@RobertIndie RobertIndie Awaiting requested review from RobertIndie

Assignees

@merlimat merlimat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@merlimat @Demogorgon314 @BewareMyPower