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

Add CMake option to bundle all static dependencies #5347

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

davisp
Copy link
Contributor

@davisp davisp commented Oct 11, 2024

This PR adds a new bootstrap/cmake option for *nix operating systems that allows for creating a single unified static library that contains libtiledb and all the vcpkg installed dependencies.


TYPE: IMPROVEMENT
DESC: Add static bundle option for static builds

This PR adds a new bootstrap/cmake option for *nix operating systems
that allows for creating a single unified static library that contains
libtiledb and all the vcpkg installed dependencies.
@davisp
Copy link
Contributor Author

davisp commented Oct 11, 2024

Anyone wanting to use this needs to have armerge available on their path. If you have cargo installed you should be able to just do cargo install armerge.

Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

For some backstory, my understanding has been that applying custom logic to transitive dependencies goes against CMake's methodology and I've been nearly convinced that it is impossible to do precisely in the general case. We used to support installing all static libraries of our dependencies alongside our own static library, which was very fragile and prone to failures when updating dependencies, and its removal in #4505 led to massive simplification of the build system.

A new feature in CMake 3.30 might change the situation, which I tried to use to no avail. If we can iterate on it and make it work I would be more open but as I commented, the proposed implementation is IMHO not general-purpose enough to make me comfortable to include in the Core's build system.

If a downstream project requires to ship a combined static library, I believe it is more appropriate to put that logic there, where the downstream project has more control over its dependencies.

Comment on lines +949 to +952
find_program(ARMERGE armerge)
if (NOT ARMERGE)
message(FATAL_ERROR "Failed to find 'armerge' for building static bundle.")
endif()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
find_program(ARMERGE armerge)
if (NOT ARMERGE)
message(FATAL_ERROR "Failed to find 'armerge' for building static bundle.")
endif()
find_program(ARMERGE armerge REQUIRED)

Alternatively we could use libtool and lib on Windows.

import os.path
import subprocess as sp

VCPKG_DIR = "vcpkg_installed"
Copy link
Member

Choose a reason for hiding this comment

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

Bundling all static libraries in vcpkg_installed is fragile (assumes all dependencies are being obtained via vcpkg, would need extra logic for Windows and debug libraries) and inefficient (we don't necessarily depend on all libraries in that folder).

Comment on lines +944 to +967
# Create a static library that includes libtiledb and all of its dependencies
message(STATUS "CHECKING FOR STATIC BUNDLE")
if (NOT BUILD_SHARED_LIBS AND TILEDB_STATIC_BUNDLE)
# We check for the availability of armerge here rather than in the
# bundle-libs.py script.
find_program(ARMERGE armerge)
if (NOT ARMERGE)
message(FATAL_ERROR "Failed to find 'armerge' for building static bundle.")
endif()

add_custom_target(static_bundle
ALL
COMMENT "Bundle tiledb and all static library dependencies into a single archive."
COMMAND "${CMAKE_SOURCE_DIR}/scripts/bundle-libs.py"
WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
DEPENDS tiledb
BYPRODUCTS libtiledb_bundled.a
)

install(
FILES ${PROJECT_BINARY_DIR}/libtiledb_bundled.a
DESTINATION ${CMAKE_INSTALL_LIBDIR}
)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

I have noticed a recent CMake change that might help us precisely get all transitive static dependencies of a target. Here's my (unsuccessful) experiment, annotated with open questions:

# Create a static library that includes libtiledb and all of its dependencies
if (TILEDB_STATIC_BUNDLE)
    if(POLICY CMP0166)
      # TARGET_PROPERTY evaluates link properties transitively over private dependencies of static libraries.
      # Hmm, it doesn't have any effect? I'm not seeing azure-core-cpp for example in the generated Ninja file.
      cmake_policy(SET CMP0166 NEW)
    else()
      message(FATAL_ERROR "TILEDB_STATIC_BUNDLE requires CMake 3.30 or newer")
    endif()

    # Incrementally build generator expression with the library files of all of our dependencies
    set(static_deps_genex $<TARGET_PROPERTY:LINK_LIBRARIES>)
    # Expand expressions like $<TARGET_PROPERTY:tgt,INTERFACE_LINK_LIBRARIES>
    set(static_deps_genex $<GENEX_EVAL:${static_deps_genex}>)
    # Wrap each dependency target in a $<TARGET_LINKER_FILE:dep>.
    # TODO: What happens on system libraries? We might need to transform dep to
    # $<IF:$<TARGET_EXISTS:dep>,$<TARGET_LINKER_FILE:dep>,dep>. This could be done
    # with a regex replacement which I tried, but still couldn't andwer the question below.
    # The million-dollar question: How to escape the dollar?? Is it even possible?
    set(static_deps_genex $<LIST:TRANSFORM,${static_deps_genex},PREPEND,$<TARGET_LINKER_FILE:>)
    set(static_deps_genex $<LIST:TRANSFORM,${static_deps_genex},APPEND,$<ANGLE-R>>)
    # Evaluate the genex again. We go from $<TARGET_LINKER_FILE:a>;$<TARGET_LINKER_FILE:b>
    # to liba.a;libb.a
    set(static_deps_genex $<GENEX_EVAL:${static_deps_genex}>)

    # We can simplify the developer experience and post-process the existing libtiledb.a.
    # This is optional and should be done if we can resolve the concern below.
    add_custom_command(TARGET tiledb
        POST_BUILD
        # TODO: Use libtool or armerge on non-Windows.
        COMMAND lib /out:$<TARGET_FILE:tiledb> $<TARGET_FILE:tiledb> ${static_deps_genex}
        COMMAND_EXPAND_LISTS
    )

    # TODO: Clear all link_libraries of the tiledb target.
    # This sounds fragile. All our PUBLIC link_libraries are currently link-only and removing them
    # is safe, but if it stops being true, how to pick them out? There is already such target
    # in configuration_definitions, but that's undocumented and should not be exposed either way.
endif()

@ihnorton ihnorton marked this pull request as draft November 14, 2024 14:49
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.

2 participants