mesonbuild / wrapdb

New wrap requests
https://mesonbuild.com/Adding-new-projects-to-wrapdb.html
MIT License
69 stars 174 forks source link

nanoarrow: Add new package #1536

Closed WillAyd closed 1 month ago

WillAyd commented 1 month ago

@paleolimbot

WillAyd commented 1 month ago

Hmm we must be missing something for visual studio. Unfortunately not a platform I have access to, but judging by other PRs I think the issue will be related to what has been noticed in:

https://github.com/mesonbuild/wrapdb/pull/1387 https://github.com/mesonbuild/wrapdb/pull/1345

paleolimbot commented 1 month ago

I have a Windows machine and can check on this. If I'm reading the other issues correctly, it seems as though we might just not be exporting any symbols when compiling as a shared library on Windows. Using nanoarrow as a shared library is not really the intended use case (it's designed to be namespaced + statically linked or vendored); however, it should still work and we can fix that upstream.

WillAyd commented 1 month ago

That's my take too. Maybe related to some of the discussion/work in https://github.com/mesonbuild/meson/pull/10199 as well?

WillAyd commented 1 month ago

Tried to add building the tests as part of CI but they require the arrow library, so we'd have a ways to go to get that to work with meson

eli-schwartz commented 1 month ago

You can install arrow via apt / brew / pacman (doubtful it will work for choco on Windows) without building arrow itself as a wrap, which is definitely better than nothing, and at least gets CI coverage on Linux and macOS.

Regarding the library on Windows, one possible solution here is:

if is_windows
    libtype = 'static_library'
else
    libtype = 'library'
endif

build_target(
    ....,
    target_type: libtype,
)

So that you force nanoarrow to always build statically on Windows, but allow users to build however they like elsewhere.

Using vs_module_defs: 'nanoarrow.def' or explicitly adding dllexport / GNU __attribute__ ((visibility ("default"))) is a prettier solution, yes, but it's not the "only" way.

WillAyd commented 1 month ago

Looks like Arrow is distributed via apt outside of the debian repository

https://arrow.apache.org/install/

paleolimbot commented 1 month ago

Tried to add building the tests as part of CI but they require the arrow library

Will and I have chatted about this, but the Arrow C++ dependency in the tests is slated to be properly fenced in the next release if it's acceptable here to get the tests building/running on just one platform.

Using vs_module_defs: 'nanoarrow.def' or explicitly adding dllexport / GNU __attribute__ ((visibility ("default"))) is a prettier solution, yes, but it's not the "only" way.

We can add proper exporting in the next release if there's a way to get a workaround implemented in the meantime.

eli-schwartz commented 1 month ago

I think that for msys2 you can "just" say the package is called "arrow" and the package manager wrapper used will automatically attempt to install the matching toolchain package and figure out ucrt/clang64 etc by itself.

For Alpine, -dev packages exist so you apparently have to specifically install apache-arrow-dev.

For Ubuntu I think this currently sucks as is, we could I suppose extend ci_config.json to also support adding an apt repository.

eli-schwartz commented 1 month ago

if it's acceptable here to get the tests building/running on just one platform.

There is a high expectation the software works most places "as long as the dependencies can be installed goshdarnit" :D especially given the meson.build files are upstream, so I am okay merging it as is. Or at least as long as the trivially fixable stuff is hooked up, so, alpine and msys2, after which we should be able to say "yep, the wrapdb CI either shows this as passing or else fails because we just don't have support for setting it up, but the actual wrap is fine".

paleolimbot commented 1 month ago

especially given the meson.build files are upstream

...and require an Apache PMC release vote to get updated, which functionally means they just don't happen very often. The next release is probably early July and I'm confident we can remove the Arrow C++ test dependency by then (and make a PR here as a prerequisite to releasing so that we don't run into this again!)

eli-schwartz commented 1 month ago

Nice, all tests pass except Visual Studio and Ubuntu, which fail due to missing dependencies.

WillAyd commented 1 month ago

Sweet thanks @eli-schwartz - excited to see this go in. Whenever the next nanoarrow release gets out will try to add in testing for the other platforms