[Issue 2461][pulsar-client-cpp] Modified CMake files and source to enable compilation on Windows #4071

heronr · 2019-04-18T05:35:37Z

Motivation

My motivation was to be able to use the pulsar cpp client on Windows systems.

Modifications

There are a number of modifications I needed to make to enable Windows compilation

Extensive rework of multiple CMakeLists.txt files
The creation of a PULSAR_PUBLIC define to define symbol visibility in lieue of #pragma GCC visibility push(default). This is because Windows requires specifying __declspec(dllexport) and __declspec(import) explicitly and does not have a #pragma GCC visibility push(default) analogue.
all calls to usleep or sleep have been replaced with calls to boost::this_thread::sleep()

Verifying this change

This change is a trivial rework / code cleanup without any test coverage. However, it does introduce a new platform to test on. It is likely that CI checks will have to be created.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): yes
- It adds a dependency to the dlfcn-win32 library for Windows builds.
The public API: no
The schema: no
The default values of configurations: no
The wire protocol: no
The rest endpoints: no
The admin cli options: no
Anything that affects deployment: no

Documentation

Does this pull request introduce a new feature?: No
The README.md file in pulsar-client-cpp has been updated with instructions for Windows compilation

This introduces a dependency on dlfcn-win32 (https://github.com/dlfcn-win32/dlfcn-win32). This change involved some reworking of the CMake file and creating 2 new defines, PULSAR_PUBLIC and PULSAR_STATIC, to enable building static or shared libraries on Windows.

find_package call to get the include dir and library and expects the user to have gotten the library separately.

merlimat

Very nice work!

Just left few minor comments

merlimat · 2019-04-18T22:45:14Z

pulsar-client-cpp/lib/Backoff.cc

-      mandatoryStop_(mandatoryStop),
-      randomSeed_(time(NULL)) {}
+      mandatoryStop_(mandatoryStop)
+#ifndef _MSC_VER


Maybe we could use boost::random to get a portable version of this.

That's a great idea. I went ahead modified Backoff to use a boost::random::mt19937 object for random number generation.

merlimat · 2019-04-18T23:45:46Z

pulsar-client-cpp/tests/BackoffTest.cc

@@ -51,7 +53,7 @@ TEST(BackoffTest, firstBackoffTimerTest) {
    Backoff backoff(milliseconds(100), seconds(60), milliseconds(1900));
    ASSERT_EQ(backoff.next().total_milliseconds(), 100);
    boost::posix_time::ptime firstBackOffTime = PulsarFriend::getFirstBackoffTime(backoff);
-    usleep(300 * 1000);
+    boost::this_thread::sleep(boost::posix_time::milliseconds(300));


We could also std::this_thread::sleep_for() instead of boost

That's true and it does mean one less boost lib to link. If that is preferable I can go ahead and make the change.

I did go ahead and switch everything to std::this_thread::sleep_for as you suggested

Nice. Thanks!

merlimat · 2019-04-18T23:53:49Z

pulsar-client-cpp/lib/ExecutorService.cc

+    // Ensure this service has not already been closed. This is
+    // because worker_.join() is not re-entrant on Windows
+    if (work_)
+    {


Even if Windows related, can you submit this fix as a separate PR so that it gets more visible?

Also, the { on new line will fail on the make check-format. You should be able to do make format and automatically fix all the formatting (that uses clang-format underneath)

Ok, I took this change out of this PR and into a new one, #4084

In addition, I realized that I didn't notice the check-format failure because the Windows version of the CMake files were omitting those targets. I re-added them and was able to correct a host of formatting mistakes.

merlimat · 2019-04-19T01:34:51Z

Once this is merged, we can start looking in how to enable C++ build validation for Windows. Apache's Jenkins has several Windows worker nodes.

and including format corrected files

better portability

and removed the dependency on boost::thread

separate PR

merlimat · 2019-04-19T19:47:01Z

@heronr There seems to be a linking issue on the test tools (protobuf missing) : https://builds.apache.org/job/pulsar_precommit_cpp/7133/console

and fixed other linker errors

heronr · 2019-04-26T01:09:27Z

@merlimat Finally got all the tests passing. Should be good to go.

### Motivation I don't think the following `--exclude-libs,ALL` are reflected in linker. [https://github.com/apache/pulsar/blob/f8080f4eb690d751f1b07ca54c23e1cd7774473f/pulsar-client-cpp/CMakeLists.txt#L214](https://github.com/apache/pulsar/blob/f8080f4eb690d751f1b07ca54c23e1cd7774473f/pulsar-client-cpp/CMakeLists.txt#L214) I think `--exclude-libs,ALL` needs to be set in `add_link_options`. [add_link_options](https://cmake.org/cmake/help/latest/command/add_link_options.html) `add_link_options` can be used from v3.13. Therefore, fixed(reverted) `add_compile_options` to `set`.([#4071](#4071))

### Motivation I don't think the following `--exclude-libs,ALL` are reflected in linker. [https://github.com/apache/pulsar/blob/f8080f4eb690d751f1b07ca54c23e1cd7774473f/pulsar-client-cpp/CMakeLists.txt#L214](https://github.com/apache/pulsar/blob/f8080f4eb690d751f1b07ca54c23e1cd7774473f/pulsar-client-cpp/CMakeLists.txt#L214) I think `--exclude-libs,ALL` needs to be set in `add_link_options`. [add_link_options](https://cmake.org/cmake/help/latest/command/add_link_options.html) `add_link_options` can be used from v3.13. Therefore, fixed(reverted) `add_compile_options` to `set`.([apache#4071](apache#4071))

### Motivation I don't think the following `--exclude-libs,ALL` are reflected in linker. [https://github.com/apache/pulsar/blob/f8080f4eb690d751f1b07ca54c23e1cd7774473f/pulsar-client-cpp/CMakeLists.txt#L214](https://github.com/apache/pulsar/blob/f8080f4eb690d751f1b07ca54c23e1cd7774473f/pulsar-client-cpp/CMakeLists.txt#L214) I think `--exclude-libs,ALL` needs to be set in `add_link_options`. [add_link_options](https://cmake.org/cmake/help/latest/command/add_link_options.html) `add_link_options` can be used from v3.13. Therefore, fixed(reverted) `add_compile_options` to `set`.([#4071](apache/pulsar#4071))

heronr added 2 commits April 18, 2019 00:31

Updated README.md to include Windows build instructions

edbdcc8

heronr force-pushed the CppClientWinCompile branch from b0e39b5 to edbdcc8 Compare April 18, 2019 07:31

Removed dlfcn-win32 from the pulsar repo. The CMake file now uses a

d494267

find_package call to get the include dir and library and expects the user to have gotten the library separately.

merlimat approved these changes Apr 18, 2019

View reviewed changes

merlimat added this to the 2.4.0 milestone Apr 19, 2019

merlimat added component/c++ type/feature The PR added a new feature or issue requested a new feature labels Apr 19, 2019

heronr added 5 commits April 18, 2019 21:35

Ensured clang-tidy and clang-format are still invoked on Windows builds

201c74c

and including format corrected files

Replaced raw calls to rand() or rand_s() with boost::random::mt19937 for

63cbba7

better portability

Replaced usage of boost::this_thread::sleep with std::thread::sleep_for

0e11322

and removed the dependency on boost::thread

Removing Windows specific change to the ExecutorService to put into a

a75af66

separate PR

Fixed formatting using clang-format-5

c7c2504

heronr and others added 5 commits April 19, 2019 22:59

Fixed issue finding the protobuf-lite libraries on the build container

70ffef0

and fixed other linker errors

Fixed test for gmock and gtest

e1aa3e3

Merge branch 'master' into CppClientWinCompile

cd6d292

Ensure the random number generator returns values in the correct range

ff76d83

Formatting fixes

81b5029

sijie approved these changes Apr 26, 2019

View reviewed changes

sijie merged commit b3596c4 into apache:master Apr 26, 2019

sijie assigned heronr Apr 26, 2019

heronr deleted the CppClientWinCompile branch April 26, 2019 04:18

merlimat mentioned this pull request Nov 7, 2019

Fixed static linking on C++ lib on MacOS #5581

Merged

shustsud mentioned this pull request Nov 13, 2020

[C++] Fixed the setting method of linker option. #8554

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue 2461][pulsar-client-cpp] Modified CMake files and source to enable compilation on Windows #4071

[Issue 2461][pulsar-client-cpp] Modified CMake files and source to enable compilation on Windows #4071

heronr commented Apr 18, 2019 •

edited

Loading

merlimat left a comment

merlimat Apr 18, 2019

heronr Apr 19, 2019

merlimat Apr 18, 2019

heronr Apr 19, 2019

heronr Apr 19, 2019

merlimat Apr 19, 2019

merlimat Apr 18, 2019

heronr Apr 19, 2019

merlimat commented Apr 19, 2019

merlimat commented Apr 19, 2019

heronr commented Apr 26, 2019

[Issue 2461][pulsar-client-cpp] Modified CMake files and source to enable compilation on Windows #4071

[Issue 2461][pulsar-client-cpp] Modified CMake files and source to enable compilation on Windows #4071

Conversation

heronr commented Apr 18, 2019 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

merlimat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat commented Apr 19, 2019

merlimat commented Apr 19, 2019

heronr commented Apr 26, 2019

heronr commented Apr 18, 2019 •

edited

Loading