Skip to content

Conversation

@cwaldren-ld
Copy link
Contributor

@cwaldren-ld cwaldren-ld commented Oct 9, 2023

This adds documentation of the various options used to control the SDK build!

It also cleans up the options by prefixing them with LD_ and making use of cmake_dependent_option to expose only relevant options.

The default, hands-off configuration is:

  • Build the SDK and its unit tests (with sanitizer support)
  • The SDK artifact is a static library
  • Build example apps
  • Static link OpenSSL

Users can then tweak away from the default:

  • Disable unit test build (or disable sanitizers)
  • Enable contract test build
  • Disable example app build
  • Make the SDK artifact a shared library
  • Link OpenSSL dynamically

Finally, to simply disable all testing stuff and just produce the SDK, the common BUILD_TESTING flag can be disabled. That'll force any of the testing related flags off - use case is build scripts / package maintainers.

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #218523: Allow OpenSSL to be dynamically linked.

@cwaldren-ld cwaldren-ld force-pushed the cw/sc-218523/openssl-dynamic-link branch 2 times, most recently from 444a6a7 to 64c6978 Compare October 10, 2023 22:41
@cwaldren-ld cwaldren-ld force-pushed the cw/sc-218523/openssl-dynamic-link branch from 64c6978 to 0d29f36 Compare October 10, 2023 22:53
@cwaldren-ld cwaldren-ld changed the title build: clean up LD CMake variables & allow for OpenSSL dynamic link feat: clean up LD CMake variables & allow for OpenSSL dynamic link Oct 11, 2023
@cwaldren-ld cwaldren-ld changed the title feat: clean up LD CMake variables & allow for OpenSSL dynamic link build: clean up LD CMake variables & allow for OpenSSL dynamic link Oct 11, 2023
@cwaldren-ld cwaldren-ld changed the title build: clean up LD CMake variables & allow for OpenSSL dynamic link feat: clean up LD CMake variables & allow for OpenSSL dynamic link Oct 11, 2023

option(LD_BUILD_EXAMPLES "Build hello-world examples." ON)


Copy link
Contributor Author

Choose a reason for hiding this comment

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

Throwing a warning when someone tries to build shared lib with unit tests enabled. This won't work since we hide symbols by default.

@cwaldren-ld cwaldren-ld marked this pull request as ready for review October 12, 2023 20:53
@cwaldren-ld cwaldren-ld requested a review from a team October 12, 2023 20:53
if (NOT LD_BUILD_STATIC_LIBS)
# When building a shared library we hide all symbols
# aside from this we have specifically exported for the C-API.
set(CMAKE_CXX_VISIBILITY_PRESET hidden)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe not right now, but we probably need an option for this. For those that build themselves and want to build a C++ dll.

Copy link
Member

Choose a reason for hiding this comment

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

(If they build all their own stuff, then the ABI issue isn't really an SDK issue.)


if (MSVC OR (NOT BUILD_SHARED_LIBS))
if (MSVC OR LD_BUILD_STATIC_LIBS)
target_link_libraries(${LIBNAME}
Copy link
Member

Choose a reason for hiding this comment

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

We may want to note this MSVC condition in the readme.

Unless we want to figure out how to make it actually work with dynamically linked boost on windows (world of pain).

@cwaldren-ld cwaldren-ld merged commit ed23c9a into main Oct 13, 2023
@cwaldren-ld cwaldren-ld deleted the cw/sc-218523/openssl-dynamic-link branch October 13, 2023 20:16
@github-actions github-actions bot mentioned this pull request Oct 13, 2023
cwaldren-ld pushed a commit that referenced this pull request Oct 16, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>launchdarkly-cpp-client: 3.1.0</summary>

##
[3.1.0](launchdarkly-cpp-client-v3.0.10...launchdarkly-cpp-client-v3.1.0)
(2023-10-13)


### Features

* clean up LD CMake variables & allow for OpenSSL dynamic link
([#255](#255))
([ed23c9a](ed23c9a))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * launchdarkly-cpp-internal bumped from 0.1.11 to 0.2.0
    * launchdarkly-cpp-common bumped from 0.3.7 to 0.4.0
    * launchdarkly-cpp-sse-client bumped from 0.1.3 to 0.2.0
</details>

<details><summary>launchdarkly-cpp-common: 0.4.0</summary>

##
[0.4.0](launchdarkly-cpp-common-v0.3.7...launchdarkly-cpp-common-v0.4.0)
(2023-10-13)


### Features

* clean up LD CMake variables & allow for OpenSSL dynamic link
([#255](#255))
([ed23c9a](ed23c9a))
</details>

<details><summary>launchdarkly-cpp-internal: 0.2.0</summary>

##
[0.2.0](launchdarkly-cpp-internal-v0.1.11...launchdarkly-cpp-internal-v0.2.0)
(2023-10-13)


### Features

* clean up LD CMake variables & allow for OpenSSL dynamic link
([#255](#255))
([ed23c9a](ed23c9a))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * launchdarkly-cpp-common bumped from 0.3.7 to 0.4.0
</details>

<details><summary>launchdarkly-cpp-sse-client: 0.2.0</summary>

##
[0.2.0](launchdarkly-cpp-sse-client-v0.1.3...launchdarkly-cpp-sse-client-v0.2.0)
(2023-10-13)


### Features

* clean up LD CMake variables & allow for OpenSSL dynamic link
([#255](#255))
([ed23c9a](ed23c9a))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Oct 23, 2023
@github-actions github-actions bot mentioned this pull request Oct 24, 2023
@github-actions github-actions bot mentioned this pull request May 13, 2024
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.

3 participants