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.

Sign up for GitHub

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

Jump to bottom

Providing access to the TileDB-R shared libraries in downstream packages #780

Open
LTLA opened this issue Nov 8, 2024 · 6 comments · Fixed by #782 · May be fixed by #784
Open

Providing access to the TileDB-R shared libraries in downstream packages #780

LTLA opened this issue Nov 8, 2024 · 6 comments · Fixed by #782 · May be fixed by #784
Assignees
@ihnorton @mojaveazure

Comments

@LTLA
Copy link
Contributor

LTLA commented Nov 8, 2024 •
edited
Loading

I have an R package (beachmat.tiledb) that vendors a C++ library that links to the TileDB C++ library. I would like my package to be installable even if TileDB is not installed in the system, so I was going to download the TileDB release binaries myself. However, I noticed that tiledb-R's installation directory already contains a copy of the binaries in its tiledb/ directory. I would like to be able to use these binaries to avoid having to duplicate tiledb-R's configure.ac in my own package. (This also avoids the creation of redundant copies of the TileDB binaries scattered throughout the user's R installation directory.)

Would you consider adding a little function that sets up the Makevars flags for downstream packages to link to these binaries? Based on the configure.ac, it would look something like this (for Linux/MacOS):

pkgconfig <- function(opt = c("PKG_CXX_FLAGS", "PKG_CXX_LIBS")) {
    opt <- match.arg(opt)
    pkgdir <- system.file(package='tiledb')
    vendored <- system.file('tiledb', package='tiledb')

    if (vendored != "") {
        if (opt == "PKG_CXX_FLAGS") {
            return(sprintf("-I%s/include -I%s/include", pkgdir, vendored))
        } else if (opt == "PKG_CXX_LIBS") {
            return(sprintf("-ltiledb -L%s/lib -L%s/lib", pkgdir, vendored))
        }
    }

    pc <- Sys.which("pkg-config")
    if (pc != "") {
        if (system2("pkg-config", c("--exists", "tiledb")) == 0) {
            if (opt == "PKG_CXX_FLAGS"){
                return(system2("pkg-config", c("--cflags", "tiledb"), stdout=TRUE))
            } else if (opt == "PKG_CXX_LIBS") {
                return(system2("pkg-config", c("--libs", "tiledb"), stdout=TRUE))
            }
        }
    }

    if (opt == "PKG_CXX_FLAGS"){
        return("")
    } else if (opt == "PKG_CXX_LIBS") {
        return("-ltiledb")
    }
}

Downstream packages could then create a little Makevars as shown below, allowing their C++ code to compile against tiledb-R's copy of the binaries (or the system libraries, if available).

CXX_STD = CXX17
PKG_LIBS=$(shell "${R_HOME}/bin${R_ARCH_BIN}/Rscript" -e 'tiledb::pkgconfig("PKG_CXX_LIBS")') 
PKG_CPPFLAGS=$(shell "${R_HOME}/bin${R_ARCH_BIN}/Rscript" -e 'tiledb::pkgconfig("PKG_CXX_FLAGS")')

This could be (easily?) extended to Windows, if the newly built static library was deposited somewhere in inst/ so that pkgconfig() could reference it later in their Makevars.win.

FWIW this proposal is based on the strategy used by the Rhdf5lib and Rhtslib packages.
mojaveazure added a commit that referenced this issue Nov 11, 2024
@mojaveazure
Provide access to TileDB shared libraries in downstream packages
3eb236f 
New helper function to provide access to `libtiledb` used by tiledb-r
for downstream packages. This builds off the prototype code @LTLA
provided

resolves #780
@mojaveazure mojaveazure mentioned this issue Nov 11, 2024
Merged
@mojaveazure
Copy link
Member

Hi @LTLA; this functionality is now available in the development version of TileDB-R; you can install the development version of TileDB-R from GitHub with remotes::install_github() or from R-universe (once built in ~2 hours) with

install.packages("tiledb", repos = c("https://tiledb-inc.r-universe.dev", getOption("repos")))

Please let us know if you need more for this functionality to work with beachmat.tiledb

@LTLA
Copy link
Contributor Author

LTLA commented Nov 18, 2024

Nice, thanks - I'll check it out tonight/tomorrow.

@johnkerl
Copy link
Collaborator

BTW/FYI we anticipate a 0.31.0 release of this package in the next couple weeks

@LTLA
Copy link
Contributor Author

LTLA commented Nov 19, 2024 •
edited
Loading

Works like a charm on Linux and MacOS. Will check on the Mac at work tomorrow.

Unfortunately, on Windows, there are a few issues. The first is at:

TileDB-R/R/Version.R

Line 179 in ffabb43

sprintf("%s/lib/%s", shQuote(x, type = "cmd"), arch),

The quotes introduced by shQuote become part of the path and confuses the subsequent dir.exists(), causing Filter to remove all paths. An easy fix is to move the shQuote() call into the paste0 for -L.

After fixing the -L flags, I get a barrage of linking errors. Most of these can be resolved by copying all of the -l flags in the Makevars.win into the output of .pkg_config("PKG_CXX_LIBS"). I managed to whittle it down to the following:

$ /c/Program\ Files/R/R-4.4.1/bin/R CMD INSTALL beachmat.tiledb/
* installing to library 'C:/Users/aaron/AppData/Local/R/win-library/4.4'
* installing *source* package 'beachmat.tiledb' ...
** using staged installation
** libs
using C++ compiler: 'G__~1.EXE (GCC) 13.2.0'
using C++17
g++ -shared -s -static-libgcc -o beachmat.tiledb.dll tmp.def RcppExports.o initialize.o load_dense.o load_sparse.o -LC:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/lib/x64 -LC:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64 -LC:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64-ucrt -ltiledbstatic -ltiledb -lbz2 -lzstd -llz4 -lz -lspdlog -lfmt -laws-cpp-sdk-identity-management -laws-cpp-sdk-cognito-identity -laws-cpp-sdk-sts -laws-cpp-sdk-s3 -laws-cpp-sdk-core -llibmagic -lwebp -lpcre2-posix -lpcre2-8 -laws-crt-cpp -laws-c-mqtt -laws-c-event-stream -laws-c-s3 -laws-c-auth -laws-c-http -laws-c-io -lSecur32 -lCrypt32 -laws-c-compression -laws-c-cal -lNCrypt -laws-c-sdkutils -laws-checksums -laws-c-common -lBCrypt -lKernel32 -lRpcrt4 -lWininet -lWinhttp -lWs2_32 -lShlwapi -lUserenv -lversion -lws2_32 -lsharpyuv -LC:/rtools44/x86_64-w64-mingw32.static.posix/lib/x64 -LC:/rtools44/x86_64-w64-mingw32.static.posix/lib -LC:/PROGRA~1/R/R-44~1.1/bin/x64 -lR
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: initialize.o:initialize.cpp:(.text$_ZNK6tiledb5Query10field_typeERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE[_ZNK6tiledb5Query10field_typeERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE]+0xa0): undefined reference to `tiledb_query_get_field'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: initialize.o:initialize.cpp:(.text$_ZNK6tiledb5Query10field_typeERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE[_ZNK6tiledb5Query10field_typeERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE]+0xf7): undefined reference to `tiledb_field_datatype'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: initialize.o:initialize.cpp:(.text$_ZNK6tiledb5Query10field_typeERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE[_ZNK6tiledb5Query10field_typeERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE]+0x14f): undefined reference to `tiledb_query_field_free'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: initialize.o:initialize.cpp:(.text$_ZNK6tiledb5Query10field_typeERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE[_ZNK6tiledb5Query10field_typeERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE]+0x1e4): undefined reference to `tiledb_query_field_free'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: initialize.o:initialize.cpp:(.text$_ZNK6tiledb5Query15field_var_sizedERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE[_ZNK6tiledb5Query15field_var_sizedERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE]+0xa0): undefined reference to `tiledb_query_get_field'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: initialize.o:initialize.cpp:(.text$_ZNK6tiledb5Query15field_var_sizedERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE[_ZNK6tiledb5Query15field_var_sizedERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE]+0xf7): undefined reference to `tiledb_field_cell_val_num'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: initialize.o:initialize.cpp:(.text$_ZNK6tiledb5Query15field_var_sizedERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE[_ZNK6tiledb5Query15field_var_sizedERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE]+0x14f): undefined reference to `tiledb_query_field_free'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: initialize.o:initialize.cpp:(.text$_ZNK6tiledb5Query15field_var_sizedERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE[_ZNK6tiledb5Query15field_var_sizedERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE]+0x1e4): undefined reference to `tiledb_query_field_free'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libtiledbstatic.a(array_directory.cc.obj):(.text+0xae57): undefined reference to `std::istream::seekg(std::fpos<int>)'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libtiledbstatic.a(array_schema.cc.obj):(.text+0x3693): undefined reference to `__imp___iob_func'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libtiledbstatic.a(attribute.cc.obj):(.text+0x1bb4): undefined reference to `__imp___iob_func'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libtiledbstatic.a(dimension.cc.obj):(.text+0x1882): undefined reference to `__imp___iob_func'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libtiledbstatic.a(dimension_label.cc.obj):(.text+0x477): undefined reference to `__imp___iob_func'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libtiledbstatic.a(domain.cc.obj):(.text+0xec7): undefined reference to `__imp___iob_func'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libtiledbstatic.a(enumeration.cc.obj):(.text+0xcb6): more undefined references to `__imp___iob_func' follow
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libaws-cpp-sdk-core.a(EventStreamBuf.cpp.obj):(.text+0x68): undefined reference to `std::istream::seekg(std::fpos<int>)'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libaws-cpp-sdk-core.a(EventStreamBuf.cpp.obj):(.text+0x335): undefined reference to `std::istream::seekg(std::fpos<int>)'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libaws-cpp-sdk-core.a(ub_core.cpp.obj):(.text+0xcd23): undefined reference to `__imp___iob_func'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libaws-cpp-sdk-core.a(ub_core.cpp.obj):(.text+0xe025): undefined reference to `std::istream::seekg(std::fpos<int>)'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libaws-cpp-sdk-core.a(ub_core.cpp.obj):(.text+0x1624c): undefined reference to `std::istream::seekg(std::fpos<int>)'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libaws-cpp-sdk-core.a(ub_core.cpp.obj):(.text+0x1795c): undefined reference to `std::istream::seekg(std::fpos<int>)'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libaws-cpp-sdk-core.a(ub_core.cpp.obj):(.text+0x257e5): undefined reference to `std::istream::seekg(std::fpos<int>)'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libaws-cpp-sdk-core.a(ub_core.cpp.obj):(.text+0x2e1f3): undefined reference to `std::istream::seekg(std::fpos<int>)'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libaws-cpp-sdk-core.a(ub_core.cpp.obj):(.rdata$_ZTVN3Aws5Utils6Crypto22SymmetricCryptoBufSinkE[_ZTVN3Aws5Utils6Crypto22SymmetricCryptoBufSinkE]+0x38): undefined reference to `std::basic_streambuf<char, std::char_traits<char> >::seekpos(std::fpos<int>, std::_Ios_Openmode)'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/liblibmagic.a(apprentice.c.obj):(.text+0x5bb): undefined reference to `__imp___iob_func'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/liblibmagic.a(apprentice.c.obj):(.text+0x628): undefined reference to `__imp___iob_func'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/liblibmagic.a(apprentice.c.obj):(.text+0x406d): undefined reference to `__imp___iob_func'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/liblibmagic.a(apprentice.c.obj):(.text+0x44e6): undefined reference to `__imp___iob_func'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/liblibmagic.a(funcs.c.obj):(.text+0x7fd): undefined reference to `__imp___iob_func'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/liblibmagic.a(funcs.c.obj):(.text+0xc42): more undefined references to `__imp___iob_func' follow
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libaws-c-common.a(log_formatter.c.obj):(.text+0x246): undefined reference to `vsnprintf_s'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libaws-c-common.a(log_writer.c.obj):(.text+0x8f): undefined reference to `__imp___iob_func'
C:\rtools44\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/Users/aaron/AppData/Local/R/win-library/4.4/tiledb/tiledb/lib/x64/libaws-c-common.a(log_writer.c.obj):(.text+0x10f): undefined reference to `__imp___iob_func'
collect2.exe: error: ld returned 1 exit status
no DLL was created
ERROR: compilation failed for package 'beachmat.tiledb'
* removing 'C:/Users/aaron/AppData/Local/R/win-library/4.4/beachmat.tiledb'

Dunno what's going on here, because the same -l flags work fine for the tiledb R package itself. The only difference is that I see tiledb's -L refers to a non-existent ../inst/tiledb/lib-13.2.0/x64 path, while beachmat.tiledb refers to the correct **/tiledb/lib/x64 path. Maybe this is consequential if linking to x64-ucrt is intended.

FWIW I'm using Rtools 4.4 on R 4.4.1, and I installed tiledb from GitHub (ffabb43).

@LTLA
Copy link
Contributor Author

LTLA commented Dec 2, 2024

Update: I finally got it to work on Windows with the following code:

      PKG_CXX_FLAGS = switch(
        EXPR = .Platform$OS.type,
        # Adapted from Makevars.win, which includes libdir/include/tiledb in
        # addition to libdir/include and pkgdir/include
        windows = sprintf(
          "-I%s/include -I%s/include -I%s/include/tiledb -DTILEDB_STATIC_DEFINE -DTILEDB_SILENT_BUILD",
          shQuote(pkgdir, type = "cmd"),
          shQuote(lib, type = "cmd"),
          shQuote(lib, type = "cmd")
        ),
        sprintf("-I%s/include -I%s/include", pkgdir, lib)
      ),
      PKG_CXX_LIBS = switch(
        EXPR = .Platform$OS.type,
        # rwinlib-tiledb is structured slightly differently than libtiledb for
        # Unix-alikes; R 4.2 and higher require ucrt
        windows = {
          arch <- .Platform$r_arch
          libs <- as.vector(vapply(
            c(pkgdir, lib),
            FUN = \(x) c(
              ifelse(
                test = getRversion() > '4.2.0',
                yes = sprintf("%s/lib/%s-ucrt", x, arch),
                no = sprintf("%s/lib/%s", x, arch)
              )
            ),
            FUN.VALUE = character(1L),
            USE.NAMES = FALSE
          ))
          paste(paste(paste0('-L', shQuote(Filter(dir.exists, libs), type="sh")), collapse = ' '), "-ltiledbstatic -ltiledb -lbz2 -lzstd -llz4 -lz -lspdlog -lfmt -laws-cpp-sdk-identity-management -laws-cpp-sdk-cognito-identity -laws-cpp-sdk-sts -laws-cpp-sdk-s3 -laws-cpp-sdk-core -llibmagic -lwebp -lpcre2-posix -lpcre2-8 -laws-crt-cpp -laws-c-mqtt -laws-c-event-stream -laws-c-s3 -laws-c-auth -laws-c-http -laws-c-io  -lSecur32 -lCrypt32 -laws-c-compression -laws-c-cal -lNCrypt -laws-c-sdkutils -laws-checksums -laws-c-common -lBCrypt -lKernel32 -lRpcrt4 -lWininet -lWinhttp -lWs2_32 -lShlwapi -lUserenv -lversion -lws2_32 -lsharpyuv")
        },
        sprintf("-ltiledb -L%s/lib -L%s/lib", pkgdir, lib)
      )

replacing L158-192 of Version.R. Basically this adds all of the -l linker flags that are used by the TileDB R package itself, and it links exclusively to the UCRT binaries - the linker warnings in my previous message were due to the other libraries in lib/x64 being used instead. I also added some more compiler flags for consistency with TileDB's own build.

@johnkerl johnkerl reopened this Dec 2, 2024
@johnkerl johnkerl changed the title Providing access to the TileDB shared libraries in downstream packages Providing access to the TileDB-R shared libraries in downstream packages Dec 2, 2024
@johnkerl
Copy link
Collaborator

johnkerl commented Dec 2, 2024

[sc-60076]

mojaveazure added a commit that referenced this issue Dec 10, 2024
@mojaveazure
Fix package config on Windows
bb93142 
Fix issues with `.pkg_config()` on Windows; include new include and
linking flags as suggested by @LTLA

[SC-60076](https://app.shortcut.com/tiledb-inc/story/60076)

resolves #780
@mojaveazure mojaveazure linked a pull request Dec 10, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ihnorton ihnorton

@mojaveazure mojaveazure

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@ihnorton @johnkerl @LTLA @mojaveazure