Make lib and cmake target names the same as repository name, do NOT support cmake install multiple configurations simultaneously #812
Conversation
This file contains 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
…nt from the cmake find_package name and library names. It becomes too confusing to the users and it is NOT recommended by package managers such as conan and vcpkg. Also, supporting different targets for ssl and static was not a good idea and source of confusion. The user configures it anyway when using cmake or package managers, hence, deleted those options. Eliminated the generation of different targets for static and ssl and it will be only controlled by cmake options. Only a single type is supported per installation. The library and target names are also changed to the same name as the project `hazelcast-cpp-client`.
|
Linux test FAILed. |
|
Windows test FAILed. |
sancar
previously approved these changes
Feb 16, 2021
…s is the standard recommended option name https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html Removed all the `BUILD_STATIC_LIB` flags and related documentation. Updated the scripts to work with the latest changes. Minor fixes for the copy ellision warning of std::move statements.
|
Windows test FAILed. |
|
Linux test FAILed. |
|
Windows test FAILed. |
|
Linux test PASSed. |
ihsandemir
added a commit
to ihsandemir/vcpkg
that referenced
this pull request
Feb 17, 2021
yemreinci
suggested changes
Feb 18, 2021
CMakeLists.txt
Outdated
| @@ -103,66 +93,66 @@ if (WITH_OPENSSL) | |||
| endif () | |||
|
|
|||
|
|
|||
| function(add_hazelcast_library name type) | |||
| function(add_hazelcast_library type) | |||
There was a problem hiding this comment.
I guess now that we work with a single type of library during the build, we can remove this function and put its contents in the global scope.
README.md
Outdated
| ``` | ||
| Build both the shared and static library without SSL support: | ||
| Build both the shared library without SSL support: |
There was a problem hiding this comment.
I think we can delete this example, because it does the same with the default cmake .. (SSL is off by default).
The above example alone is explanatory enough.
You may also consider removing the section header "1.1.5.2.1" completely, and put the above example under "1.1.5.2".
There was a problem hiding this comment.
Deleted the line. Removed the section "1.1.5.2.1".
README.md
Outdated
|
|
||
| The package name depends on the specific library type you want to use. | ||
| Options are `hazelcastcxx`, `hazelcastcxx_ssl`, `hazelcastcxx_static`, and `hazelcastcxx_ssl_static`. | ||
| target_link_libraries(mytarget PRIVATE hazelcast-cpp-client) |
There was a problem hiding this comment.
namespace is missing.
Suggested change
| target_link_libraries(mytarget PRIVATE hazelcast-cpp-client) | |
| target_link_libraries(mytarget PRIVATE hazelcast-cpp-client::hazelcast-cpp-client) |
README.md
Outdated
| @@ -252,18 +252,17 @@ You can provide additional configuration options using the `-DVARIABLE=VALUE` sy | |||
| Here are all the options that are supported: | |||
| * `WITH_OPENSSL` : Set to `ON` to build the library with SSL support. | |||
| This will require [OpenSSL](https://www.openssl.org) to be installed on your system. The default is `OFF`. | |||
| * `BUILD_STATIC_LIB` : Set to `ON` or `OFF` depending on whether you want the static library. The default is `OFF`. | |||
| * `BUILD_SHARED_LIB` : Set to `ON` or `OFF` depending on whether you want the shared library. The default is `ON`. | |||
| * `BUILD_SHARED_LIBS` : Set to `ON` or `OFF` depending on whether you want the shared(ON) or static(OFF) library. The default is `ON`. | |||
| * `DISABLE_LOGGING` : Setting this option to `ON` disables logging. The default is `OFF`. | |||
|
|
|||
| ##### 1.1.5.2.1 Example Configuration Commands | |||
| Build only the static library with SSL support: | |||
There was a problem hiding this comment.
Since building both is not supported anymore, we may drop "only".
Suggested change
| Build only the static library with SSL support: | |
| Build the static library with SSL support: |
cmake/config.cmake.in
Outdated
| @@ -8,10 +8,10 @@ if (@WITH_OPENSSL@) | |||
| find_dependency(OpenSSL) | |||
| endif() | |||
|
|
|||
| include(${CMAKE_CURRENT_LIST_DIR}/@name@-targets.cmake) | |||
| include(${CMAKE_CURRENT_LIST_DIR}/hazelcast-cpp-client-targets.cmake) | |||
There was a problem hiding this comment.
@PROJECT_NAME@ can be used in this file.
…indows test so that gtest.dll can be found during running the test executable.
Co-authored-by: yemreinci <18687880+yemreinci@users.noreply.github.com>
|
Linux test PASSed. |
|
Windows test PASSed. |
|
Linux test PASSed. |
1 similar comment
|
Linux test PASSed. |
|
Windows test PASSed. |
|
Windows test PASSed. |
yemreinci
approved these changes
Feb 18, 2021
ras0219-msft
added a commit
to microsoft/vcpkg
that referenced
this pull request
Feb 24, 2021
* Added Hazelcast C++ client (https://github.com/hazelcast/hazelcast-cpp-client) port for Hazelcast in-memory database. * Fixed the patch (The generated cmake files destination is corrected.) * Updated the patch. see hazelcast/hazelcast-cpp-client#812 * Disable uwp support. * Added the missing version file for hazelcast-cpp-client. * [hazelcast-cpp-client] Avoid checking in large patch file * Format vcpkg.json * Updated the version for the latest commit using the command `./vcpkg x-add-version --overwrite-version hazelcast-cpp-client`. Co-authored-by: Robert Schumacher <roschuma@microsoft.com> Co-authored-by: NancyLi1013 <lirui09@beyondsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
It is a problem for users when out library and github name is different from the cmake find_package name and library names. It becomes too confusing to the users and it is NOT recommended by package managers such as conan and vcpkg. Also, supporting different targets for ssl and static was not a good idea and source of confusion. The user configures it anyway when using cmake or package managers, hence, deleted those options.
Eliminated the generation of different targets for static and ssl and it will be only controlled by cmake options. Only a single type is supported per installation. The library and target names are also changed to the same name as the project
hazelcast-cpp-client.