Integrate baseline object library into the main build. #4446
Conversation
|
This pull request has been linked to Shortcut Story #15115: Integrate baseline object library into the main build. |
…B_COMMON_SOURCES`. They are now imported via baseline, and including them twice causes ODR violations.
…objects as sources. Now that `TILEDB_CORE_OBJECTS` has dependencies on its own, they have to be propagated.
On Linux CMake complains that the targets have no sources, and now it succeeds in both Linux and Windows as well.
teo-tsirpanis
force-pushed
the
baseline-objlib
branch
from
d65cb71 to
5b90604
Compare
| $<TARGET_OBJECTS:TILEDB_CORE_OBJECTS_STATIC> | ||
| $<TARGET_OBJECTS:TILEDB_CORE_OBJECTS_STATIC>) | ||
| target_link_libraries(tiledb_static | ||
| PRIVATE |
There was a problem hiding this comment.
Could it be that you're not getting symbol published because this is PRIVATE and not PUBLIC?
tiledb/CMakeLists.txt
Outdated
| set_property(TARGET TILEDB_CORE_OBJECTS_STATIC PROPERTY POSITION_INDEPENDENT_CODE ON) | ||
| target_link_libraries(TILEDB_CORE_OBJECTS_STATIC | ||
| PUBLIC | ||
| baseline $<TARGET_OBJECTS:baseline> |
There was a problem hiding this comment.
See line 417 above.
tiledb/CMakeLists.txt
Outdated
| PRIVATE | ||
| common | ||
| PUBLIC | ||
| baseline $<TARGET_OBJECTS:baseline> |
There was a problem hiding this comment.
We'll be needing to add a number of object modules to both the dynamic and static libraries. In order to avoid synchronizing two lists, we make another object library that incorporates everything. We can use the object_library environment to define it in this file.
There was a problem hiding this comment.
I am actually planning to merge tiledb_shared and tiledb_static after we stop bundling the shared libraries on GitHub Releases, which should be unblocked soon (whether to build a static or dynamic library will be determined by the BUILD_SHARED_LIBS variable). After that, the object library that incorporates everything could be TILEDB_CORE_OBJECTS.
It wouldn't hurt to keep this duplication for a little while, right? I can add a comment.
There was a problem hiding this comment.
I tried moving the baseline dependency to TILEDB_CORE_OBJECTS_ILIB to remove duplication, but it fails:
diff --git a/tiledb/CMakeLists.txt b/tiledb/CMakeLists.txt
index ef97cb5c5..92b50f1ff 100644
--- a/tiledb/CMakeLists.txt
+++ b/tiledb/CMakeLists.txt
@@ -414,8 +414,6 @@ add_library(TILEDB_CORE_OBJECTS OBJECT
target_link_libraries(TILEDB_CORE_OBJECTS
PRIVATE
common
- PUBLIC
- baseline $<TARGET_OBJECTS:baseline>
)
target_link_libraries(TILEDB_CORE_OBJECTS INTERFACE object_store_definitions)
@@ -470,7 +468,7 @@ if (WIN32 AND TILEDB_STATIC)
target_link_libraries(TILEDB_CORE_OBJECTS_STATIC
PUBLIC
- baseline $<TARGET_OBJECTS:baseline>
+ baseline
)
target_compile_definitions(TILEDB_CORE_OBJECTS_STATIC
@@ -524,6 +522,11 @@ endif()
# so we can use the targets created by the calls to find_package().
add_library(TILEDB_CORE_OBJECTS_ILIB INTERFACE)
+target_link_libraries(TILEDB_CORE_OBJECTS_ILIB
+ INTERFACE
+ $<BUILD_INTERFACE:baseline> $<TARGET_OBJECTS:baseline>
+)
+
# object store definitions
target_link_libraries(TILEDB_CORE_OBJECTS_ILIB INTERFACE object_store_definitions)
@@ -1070,7 +1073,7 @@ endif()
# Link the dependencies specified earlier
target_link_libraries(tiledb_shared
PRIVATE
- $<TARGET_PROPERTY:TILEDB_CORE_OBJECTS_ILIB,INTERFACE_LINK_LIBRARIES>
+ TILEDB_CORE_OBJECTS_ILIB
TILEDB_CORE_OBJECTS
)
@@ -1121,7 +1124,7 @@ if (TILEDB_STATIC)
target_link_libraries(tiledb_static
PRIVATE
- $<TARGET_PROPERTY:TILEDB_CORE_OBJECTS_ILIB,INTERFACE_LINK_LIBRARIES>
+ TILEDB_CORE_OBJECTS_ILIB
)
endif()| add_library(tiledb_static STATIC | ||
| $<TARGET_OBJECTS:TILEDB_CORE_OBJECTS_STATIC> | ||
| $<TARGET_OBJECTS:TILEDB_CORE_OBJECTS_STATIC>) |
There was a problem hiding this comment.
The problem might be here. STATIC libraries expect sources, so add_library might be treating the target objects as sources, not finding sources, and ignoring them. Adding the same sources in target_link_libraries may work.
There was a problem hiding this comment.
This line existed before. And the problem is that it does not add the objects from baseline, not TILEDB_CORE_OBJECTS_STATIC.
I also checked the generated Ninja file; the TILEDB_CORE_OBJECTS_STATIC target has an order-only dependency to baseline, but the objects are not included in the tiledb_static target. This does not happen with TILEDB_CORE_OBJECTS and tiledb_shared.
Fixes the `basline` objects not being included in `tiledb_static`.
|
Please don't merge right now, I'm going to try another idea to deduplicate stuff. |
Can it go in a separate PR? |
|
I pushed it in another branch in 2032a6f, and on Ninja I see the Let me know if you want it to go in this PR or a subsequent one. |
Already merged, so moot. You can put it up as a quick, ready-to-go PR. |
See #4446 (comment). --- TYPE: NO_HISTORY
SC-15115
Supersedes #2916.
TYPE: IMPROVEMENT
DESC: Integrate baseline object library into the main build