Skip to content

nanoarrow: Add new package - #1536

Merged
eli-schwartz merged 1 commit into
mesonbuild:masterfrom
WillAyd:add-nanoarrow
May 29, 2024
Merged

eli-schwartz merged 1 commit into
mesonbuild:masterfrom
WillAyd:add-nanoarrow

Conversation

@WillAyd

@WillAyd WillAyd commented May 28, 2024

Copy link
Copy Markdown
Contributor

@WillAyd

WillAyd commented May 28, 2024

Copy link
Copy Markdown
Contributor Author

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:

#1387
#1345

@paleolimbot

Copy link
Copy Markdown
Contributor

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

WillAyd commented May 28, 2024

Copy link
Copy Markdown
Contributor Author

That's my take too. Maybe related to some of the discussion/work in mesonbuild/meson#10199 as well?

@WillAyd

WillAyd commented May 28, 2024

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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.

Comment thread ci_config.json Outdated
@WillAyd
WillAyd force-pushed the add-nanoarrow branch 2 times, most recently from fd414a1 to ec1904a Compare May 28, 2024 23:58
@WillAyd

WillAyd commented May 28, 2024

Copy link
Copy Markdown
Contributor Author

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

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

@paleolimbot

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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

@eli-schwartz
eli-schwartz merged commit ad887f5 into mesonbuild:master May 29, 2024
@WillAyd
WillAyd deleted the add-nanoarrow branch May 29, 2024 03:09
@WillAyd

WillAyd commented May 29, 2024 •

Copy link
Copy Markdown
Contributor Author

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

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.

3 participants