Combine the `tiledb_shared` and `tiledb_static` CMake targets. #4528

teo-tsirpanis · 2023-11-21T14:08:15Z

This PR combines the tiledb_shared and tiledb_static CMake targets into a tiledb target whose linkage is determined by the BUILD_SHARED_LIBS variable. This approach has several advantages:

Simplifies the build system and brings it more in line with CMake's conventions.
Avoids excessive building of code. For example the MinGW workflow needs a static library but had to also build a shared library because there is no way to disable that.
Provides a unified TileDB::tiledb CMake target that links to TileDB either statically or dynamically, depending on how TileDB was built.
- The existing TileDB::tiledb_static and TileDB::tiledb_shared targets are retained for compatibility.

The TILEDB_STATIC option was removed and replaced by BUILD_SHARED_LIBS. The bootstrap scripts were updated accordingly.

TYPE: IMPROVEMENT
DESC: Export a TileDB::tiledb CMake target regardless of static or dynamic linkage.
TYPE: BUILD
DESC: Remove the TILEDB_STATIC option and replace it with BUILD_SHARED_LIBS which is disabled by default.
TYPE: BUILD
DESC: The build system supports building only a static or shared library from the same buildtree.

shortcut-integration · 2023-11-21T14:08:20Z

This pull request has been linked to Shortcut Story #36838: Combine the tiledb_shared and tiledb_static CMake targets..

teo-tsirpanis · 2023-11-22T12:51:27Z

We run out of disk or memory when statically linking the Core to the examples. 🥲

…s targets.

Fixes segfaults when running it with statically linking to TileDB.

Prevents out of disk errors on CI.

tiledb/CMakeLists.txt

When we initialize the AWS SDK on Linux, it also initializes s2n, which reads from `/dev/urandom` as part of it. However, the call to `read`, gets resolved to the `read()` function of our example. The example tries to recursively initialize the AWS SDK, but since `std::call_once` is not always reentrant, it deadlocks.

teo-tsirpanis · 2023-12-05T18:56:02Z

The timeout in vfs_c was resolved, this is ready to review.

eric-hughes-tiledb

Mostly good. Some documentation issues and user interface.

A couple of correctness issues:

The default linkage needs to be explicitly set in tiledb/CMakeLists.txt if it's not set on the command line. The bootstrap scripts should still set the option explicitly for good auditability. Nevertheless, if the options on CMake command line aren't right, there needs to be a check and an informative error message.
What looks like an incorrect placement of a script status variable after adding an intervening line.

eric-hughes-tiledb · 2023-12-06T15:14:07Z

bootstrap

+    --enable-static-tiledb          enables building TileDB as a static library (this is the default)
+    --enable-shared-tiledb          enables building TileDB as a shared library


It's a small point, but I would prefer this option as linkage=static or linkage=shared.
What you've got here is has no error checking (can't do both at once like the option structure might imply) and is overly verbose.

eric-hughes-tiledb · 2023-12-06T15:19:21Z

cmake/inputs/Config.cmake.in

 # imported targets are created:
-#   - TileDB::tiledb_shared
-#   - TileDB::tiledb_static
+#   - TileDB::tiledb


I don't know what this target is from this documentation. Same, different, what?

Added documentation that TileDB::tiledb is present always.

eric-hughes-tiledb · 2023-12-06T15:24:29Z

examples/c_api/vfs.c

+// Should be static to avoid collision with the write syscall.
+static void write() {


It would be clearer to rename the functions.

eric-hughes-tiledb · 2023-12-06T15:31:15Z

scripts/run-nix-examples.sh

+  # Remove the executable after running it to prevent disk
+  # space exhaustion when statically linking to tiledb.
+  rm $TestAppDir/$exampleexe
  status=$?


Shouldn't the status be checked before the added rm command?

Oops, reordered. Thanks!

eric-hughes-tiledb · 2023-12-06T15:31:55Z

scripts/run-nix-examples.sh

+  echo $TestAppDir/$exampleexe
+  $TestAppDir/$exampleexe;
+  rm $TestAppDir/$exampleexe
  status=$?


tiledb/CMakeLists.txt

eric-hughes-tiledb · 2023-12-06T15:56:15Z

cmake/Options/BuildOptions.cmake

 option(TILEDB_CMAKE_IDE "(Used for CLion builds). Disables superbuild and sets the EP install dir." OFF)
 option(TILEDB_STATS "Enables internal TileDB statistics gathering." ON)
-option(TILEDB_STATIC "Enables building TileDB as a static library." OFF)
+option(BUILD_SHARED_LIBS "Enables building TileDB as a shared library." OFF)


Just as in the bootstrap scripts, this would be better as a "static" or "shared" rather than boolean.

See https://stackoverflow.com/questions/8709877/cmake-string-options for how to do this.

BUILD_SHARED_LIBS is a standard CMake variable. I don't think it's a good idea to specify linkage in a different way.

BUILD_SHARED_LIBS is a standard CMake variable.

OK.

eric-hughes-tiledb

LGTM

Restores default behavior to prior to #4528. Supersedes #4560. Fixes TileDB-Inc/TileDB-MariaDB#294 Fixes TileDB-Inc/TileDB-CSharp#339 Fixes TileDB-Inc/TileDB-CSharp#340 Fixes TileDB-Inc/TileDB-CSharp#341 --- TYPE: BUILD DESC: `BUILD_SHARED_LIBS` is enabled by default.

#4528 introduced a single `TileDB::tiledb`exported CMake target for TileDB with either static or dynamic linkage. For compatibility with previous versions, the targets `TileDB::tiledb_shared` or `TileDB::tiledb_static` were also defined depending on the linkage, as `ALIAS`es to `TileDB::tiledb`. As it turns out however, we cannot use `ALIAS` targets, because they are always declared in the global scope prior to CMake 3.18 and if `find_package(TileDB)` is not called in the top-level `CMakeLists.txt` file, it will fail with `add_library cannot create ALIAS target "TileDB::tiledb_shared" because target "TileDB::tiledb" is imported but not globally visible.`. Nor can we switch to using `IMPORTED INTERFACE` targets and linking them to `TileDB::tiledb`, because it would bring a minor breaking change[^1]. Because `TileDB::tiledb_shared` would become an `INTERFACE` library, it does not have an `IMPORTED_LOCATION` anymore, which would cause [calls to `install_target_libs(TileDB::tiledb_shared)`](https://github.com/TileDB-Inc/TileDB-VCF/blob/5bcc79b07935ac540c56bf6ed9ee0f5d60bf247e/libtiledbvcf/cmake/Modules/FindTileDB_EP.cmake#L121) to fail. Thankfully there is another solution. We set the [`EXPORT_NAME`](https://cmake.org/cmake/help/latest/prop_tgt/EXPORT_NAME.html) of the `tiledb` target to either `tiledb_shared` or `tiledb_static` depending on the linkage, and define `TileDB::tiledb` as an `IMPORTED INTERFACE` target[^2] to the linkage-specific target. This maintains full compatibility. [^1]: Something similar is the "breaking build system change" I talked about in #4408 (comment). After removing the `install_target_libs` calls from this repository, the change in Curl did not afffect us and we could update much more easily. [^2]: In this opposite case the unified target _must_ be an `IMPORTED INTERFACE`. We cannot get the `IMPORTED_LOCATION` of `TileDB::tiledb`, but since the target is new this is not a breaking change. --- TYPE: BUILD DESC: Fix importing TileDB in CMake versions prior to 3.18.

#4528 introduced a single `TileDB::tiledb`exported CMake target for TileDB with either static or dynamic linkage. For compatibility with previous versions, the targets `TileDB::tiledb_shared` or `TileDB::tiledb_static` were also defined depending on the linkage, as `ALIAS`es to `TileDB::tiledb`. As it turns out however, we cannot use `ALIAS` targets, because they are always declared in the global scope prior to CMake 3.18 and if `find_package(TileDB)` is not called in the top-level `CMakeLists.txt` file, it will fail with `add_library cannot create ALIAS target "TileDB::tiledb_shared" because target "TileDB::tiledb" is imported but not globally visible.`. Nor can we switch to using `IMPORTED INTERFACE` targets and linking them to `TileDB::tiledb`, because it would bring a minor breaking change[^1]. Because `TileDB::tiledb_shared` would become an `INTERFACE` library, it does not have an `IMPORTED_LOCATION` anymore, which would cause [calls to `install_target_libs(TileDB::tiledb_shared)`](https://github.com/TileDB-Inc/TileDB-VCF/blob/5bcc79b07935ac540c56bf6ed9ee0f5d60bf247e/libtiledbvcf/cmake/Modules/FindTileDB_EP.cmake#L121) to fail. Thankfully there is another solution. We set the [`EXPORT_NAME`](https://cmake.org/cmake/help/latest/prop_tgt/EXPORT_NAME.html) of the `tiledb` target to either `tiledb_shared` or `tiledb_static` depending on the linkage, and define `TileDB::tiledb` as an `IMPORTED INTERFACE` target[^2] to the linkage-specific target. This maintains full compatibility. [^1]: Something similar is the "breaking build system change" I talked about in #4408 (comment). After removing the `install_target_libs` calls from this repository, the change in Curl did not afffect us and we could update much more easily. [^2]: In this opposite case the unified target _must_ be an `IMPORTED INTERFACE`. We cannot get the `IMPORTED_LOCATION` of `TileDB::tiledb`, but since the target is new this is not a breaking change. --- TYPE: BUILD DESC: Fix importing TileDB in CMake versions prior to 3.18. (cherry picked from commit ad0371f)

teo-tsirpanis changed the title ~~Unify linkage~~ Combine the tiledb_shared and tiledb_static CMake targets. Nov 21, 2023

teo-tsirpanis force-pushed the unify-linkage branch from 116afef to 8b60957 Compare November 22, 2023 18:00

teo-tsirpanis added 8 commits November 23, 2023 18:00

Remove TILEDB_STATIC and unify tiledb_static and tiledb_shared.

fb8139e

Add bootstrap script options to set BUILD_SHARED_LIBS.

35732e8

Update the release script to build shared libraries.

e04fd2d

Add TileDB::tiledb_static and TileDB::tiledb_shared exported alia…

7b90b53

…s targets.

Compile the Win32 resources only when we build a shared library.

841ae32

Update CI workflows.

05fe01c

Update comments.

ea075ab

Rename mentions of TILEDB_CORE_OBJECTS_EXPORTS to tiledb_EXPORTS.

b332b87

teo-tsirpanis force-pushed the unify-linkage branch from 047ad9b to b332b87 Compare November 23, 2023 16:01

teo-tsirpanis added 4 commits November 24, 2023 14:15

Don't create a context in global scope in quickstart_dense.cc.

371f41c

Fixes segfaults when running it with statically linking to TileDB.

Don't define the alias targets if they are already defined.

908f4b2

List the files in build directory on failure.

4c91a24

Build, run and delete each example individually.

4a0ea34

Prevents out of disk errors on CI.

teo-tsirpanis force-pushed the unify-linkage branch from be2c71a to 4a0ea34 Compare November 27, 2023 11:29

teo-tsirpanis added 4 commits November 27, 2023 14:10

Do not source run-nix-examples.sh.

1519711

Fix linker errors when building the benchmarks.

63ca096

Fix building the examples.

1bbd6cc

Delete seemingly useless code.

6b4fcdd

teo-tsirpanis commented Nov 28, 2023

View reviewed changes

tiledb/CMakeLists.txt Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

teo-tsirpanis marked this pull request as ready for review December 5, 2023 18:56

teo-tsirpanis requested a review from eric-hughes-tiledb December 5, 2023 18:56

eric-hughes-tiledb suggested changes Dec 6, 2023

View reviewed changes

Merge branch 'dev' into oriskany

a7eb4b7

teo-tsirpanis added 2 commits December 6, 2023 20:45

Address most PR feedback.

782a955

Fix outdated bootstrap script documentation.

94112e5

teo-tsirpanis requested a review from eric-hughes-tiledb December 6, 2023 18:53

eric-hughes-tiledb approved these changes Dec 6, 2023

View reviewed changes

KiterLuc merged commit 79266ed into TileDB-Inc:dev Dec 8, 2023

teo-tsirpanis deleted the unify-linkage branch December 11, 2023 14:24

teo-tsirpanis mentioned this pull request Dec 11, 2023

Build shared libraries by default. #4569

Merged

teo-tsirpanis mentioned this pull request Jan 26, 2024

Fix importing TileDB in CMake versions prior to 3.18. #4671

Merged

teo-tsirpanis mentioned this pull request Feb 20, 2024

update to TileDB-2.20.0 TileDB-Inc/TileDB-Java#340

Merged

		--enable-static-tiledb enables building TileDB as a static library (this is the default)
		--enable-shared-tiledb enables building TileDB as a shared library

		// Should be static to avoid collision with the write syscall.
		static void write() {

Combine the tiledb_shared and tiledb_static CMake targets. #4528

Combine the tiledb_shared and tiledb_static CMake targets. #4528

Uh oh!

Conversation

teo-tsirpanis commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shortcut-integration bot commented Nov 21, 2023

Uh oh!

teo-tsirpanis commented Nov 22, 2023

Uh oh!

Uh oh!

This comment was marked as resolved.

teo-tsirpanis commented Dec 5, 2023

Uh oh!

eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-hughes-tiledb Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Combine the `tiledb_shared` and `tiledb_static` CMake targets. #4528

Combine the `tiledb_shared` and `tiledb_static` CMake targets. #4528

teo-tsirpanis commented Nov 21, 2023 •

edited

Loading

eric-hughes-tiledb Dec 6, 2023 •

edited

Loading