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.

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

Enable calls to find_dependency in the exported config only if we are building a static library. #4526

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

teo-tsirpanis
Copy link
Member

#4505 replaced bundling of our dependencies' static libraries with calls to CMake's find_dependency command. We definitely need these calls when we have installed a static library, and we definitely don't need them in the release artifacts where everything is contained in a shared library.

Out of abundance of caution for potential cases where we might need these options in future scenarios, I controlled emitting these calls with TILEDB_INSTALL_FIND_DEPENDENCIES, an on-by-default option that was always enabled when building a static library. This caused disruptions in downstream projects such as SOMA because of this change in defaults.

My initial thought was to make this option off-by-default if we are not building a static library, but as it turns out we never need it enabled in such cases, because a shared library does not require linking to its dependencies when we link to it, and these dependencies are implementation details and not exposed through our public C API. Contrast this with cases like azure-storage-blobs-cpp depending on azure-storage-common-cpp at install-time, because the former's public headers depend on the latter's.

So this PR removes the TILEDB_INSTALL_FIND_DEPENDENCIES option and replaces it with TILEDB_STATIC.


TYPE: NO_HISTORY

…re building a static library.

Matches the previous behavior where the targets for the dependencies were defined only in that case.
Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Thumbs up -- a build from the branch underlying this PR and its one commit does the right thing for me with SOMA.

@KiterLuc KiterLuc merged commit f1c8d30 into TileDB-Inc:dev Nov 23, 2023
53 checks passed
@teo-tsirpanis teo-tsirpanis deleted the find-deps-static branch November 23, 2023 11:53
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.

The nightly build job failed on Thursday (2023-11-16)
3 participants