-
Notifications
You must be signed in to change notification settings - Fork 203
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
Conversation
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. |
That's my take too. Maybe related to some of the discussion/work in mesonbuild/meson#10199 as well? |
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 |
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 |
fd414a1
to
ec1904a
Compare
Looks like Arrow is distributed via apt outside of the debian repository |
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.
We can add proper exporting in the next release if there's a way to get a workaround implemented in the meantime. |
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 For Ubuntu I think this currently sucks as is, we could I suppose extend ci_config.json to also support adding an apt repository. |
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". |
...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!) |
Nice, all tests pass except Visual Studio and Ubuntu, which fail due to missing dependencies. |
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 |
@paleolimbot