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

nanoarrow: Add new package #1536

Merged
merged 1 commit into from
May 29, 2024
Merged

Conversation

WillAyd
Copy link
Contributor

@WillAyd WillAyd commented May 28, 2024

@WillAyd
Copy link
Contributor Author

WillAyd commented May 28, 2024

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
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
Copy link
Contributor Author

WillAyd commented May 28, 2024

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

@WillAyd
Copy link
Contributor Author

WillAyd commented May 28, 2024

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
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.

ci_config.json Outdated Show resolved Hide resolved
@WillAyd WillAyd force-pushed the add-nanoarrow branch 2 times, most recently from fd414a1 to ec1904a Compare May 28, 2024 23:58
@WillAyd
Copy link
Contributor Author

WillAyd commented May 28, 2024

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

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

@paleolimbot
Copy link
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
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
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
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
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
5 of 9 checks passed
@WillAyd WillAyd deleted the add-nanoarrow branch May 29, 2024 03:09
@WillAyd
Copy link
Contributor Author

WillAyd commented May 29, 2024

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