-
Notifications
You must be signed in to change notification settings - Fork 49
feat: Meson build system for nanoarrow-ipc extension #483
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
Changes from all commits
e5133a7
deb2f21
0250989
668e110
a6a7ea9
a3d1812
b5a9370
61516cc
e6f72df
702da7b
262a973
1de36d2
aeea2c2
f5f8001
df35957
574bea9
859a3f2
10de273
82a6b48
b878e23
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ | |
|
|
||
| void dump_schema_to_stdout(struct ArrowSchema* schema, int level, char* buf, | ||
| int buf_size) { | ||
| int n_chars = ArrowSchemaToString(schema, buf, buf_size, 0); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is still needed (or else the top-level type will never appear in the dump). Maybe just remove |
||
| ArrowSchemaToString(schema, buf, buf_size, 0); | ||
|
|
||
| for (int i = 0; i < level; i++) { | ||
| fprintf(stdout, " "); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,26 @@ incdir = include_directories('..') | |
| nanoarrow_dep = declare_dependency(include_directories: [curdir, incdir], | ||
| link_with: nanoarrow_lib) | ||
|
|
||
| if get_option('ipc') | ||
| cmake = import('cmake') | ||
| cmake_opts = cmake.subproject_options() | ||
| cmake_opts.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': true}) | ||
| flatcc_subproj = cmake.subproject('flatcc', options: cmake_opts) | ||
| flatcc_dep = flatcc_subproj.dependency('flatccrt') | ||
|
Comment on lines
+68
to
+72
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would mean that CMake is required to build the flatcc runtime? I get that it is more "meson"ic to use the wrap file; however, it might also be a little strange that building nanoarrow_ipc via CMake will give you a slightly different result than using Meson. I think I'd prefer that the default build uses the vendored version (via a pure Meson compile of the four files in thirdparty/flatcc/src/runtime), and perhaps our dependency relationship with flatcc could be improved in both CMake/Meson (perhaps with some contributions to the upstream setup).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK cool will take a look. I believe flatcc is trying to switch to Meson as well, so yea can look and see if that makes for a good upstream contribution
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is solved in https://github.com/apache/arrow-nanoarrow/pull/483/files/82a6b48e4218f24b320cfb9e05f972c59d2cdd37..b878e23bb721f07a058e9f63a4db3aa6588f0faa Basically Meson allows you to provide your own "patch" file for systems that don't use Meson. The file(s) go in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh set up the subproject here correctly but forgot to come back and update the code block here. Will push up a follow up PR to correct |
||
|
|
||
| nanoarrow_ipc_lib = build_target( | ||
| 'nanoarrow_ipc', | ||
| 'nanoarrow_ipc_decoder.c', | ||
| 'nanoarrow_ipc_reader.c', | ||
| dependencies: [nanoarrow_dep, flatcc_dep], | ||
| install: true, | ||
| target_type: libtype, | ||
| ) | ||
| nanoarrow_ipc_dep = declare_dependency(include_directories: [incdir], | ||
| link_with: nanoarrow_ipc_lib, | ||
| dependencies: [nanoarrow_dep, flatcc_dep]) | ||
| endif | ||
|
|
||
| if get_option('tests') or get_option('integration_tests') | ||
| nlohmann_json_dep = dependency('nlohmann_json') | ||
|
|
||
|
|
@@ -147,4 +167,36 @@ if get_option('tests') | |
| include_directories: incdir) | ||
| test('c_data_integration test', c_data_integration_test) | ||
|
|
||
| if get_option('ipc') | ||
| zlib_dep = dependency('zlib') | ||
| ipc_test_files = { | ||
| 'nanoarrow-ipc-decoder': { | ||
| 'deps': [nanoarrow_ipc_dep, arrow_dep, gtest_dep], | ||
| }, | ||
| 'nanoarrow-ipc-reader': { | ||
| 'deps': [nanoarrow_ipc_dep, arrow_dep, gtest_dep], | ||
| }, | ||
| 'nanoarrow-ipc-files': { | ||
| 'deps': [ | ||
| nanoarrow_ipc_dep, | ||
| zlib_dep, | ||
| arrow_dep, | ||
| gtest_dep, | ||
| nlohmann_json_dep | ||
| ], | ||
| }, | ||
| 'nanoarrow-ipc-hpp': { | ||
| 'deps': [nanoarrow_ipc_dep, gtest_dep], | ||
| }, | ||
| } | ||
|
|
||
| foreach name, config : ipc_test_files | ||
| exc = executable( | ||
| name + '-test', | ||
| name.replace('-', '_') + '_test.cc', | ||
| dependencies: config['deps'] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible to factor more common dependencies like (Also: I don't think that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I understand the first question - what problem are you trying to solve by making that a direct link argument? The way the dependency is declared (see wrapdb source) I believe Meson will take care of that linkage for you, alongside having the proper include and threads dependency As far as the second question goes the way I had this previously the dependencies were not transitive, but I've since updated the nanoarrow_ipc_dep to include those: nanoarrow_ipc_dep = declare_dependency(..., dependencies: [nanoarrow_dep, flatcc_dep])
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It seemed like the loop was handling linking each test with
Great! I mostly just want to make sure my vauge mental model of what's going on here is correct 🙂
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think when coming from CMake it is helpful to understand that the word "dependency" means something else in Meson. In CMake, dependency refers to "something that needs to be built before this target can be". In Meson, a dependency helps you transitively use include directories / link targets. You declare this via the base_dep = declare_dependency(
include_directories: ['foo_dir'],
link_with: [threads_lib],
)So when you end up using that dependency on another target, it will transitively carry over the include / link flags for you, i.e. child_lib = library('child', sources: ['child.cc'], dependencies: [base_dep])expands into child_lib = library('child',
sources: ['child.cc'],
include_directories: ['foo_dir'],
link_with: [threads_lib],
)(at least as far as I understand things) |
||
| ) | ||
| test(name, exc) | ||
| endforeach | ||
| endif | ||
| endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -470,7 +470,7 @@ TEST(NanoarrowTestingTest, NanoarrowTestingTestFieldMetadata) { | |
| NANOARROW_RETURN_NOT_OK(ArrowSchemaSetMetadata(schema, "\0\0\0\0")); | ||
| return NANOARROW_OK; | ||
| }, | ||
| [](ArrowArray* array) { return NANOARROW_OK; }, &WriteFieldJSON, | ||
| [](ArrowArray*) { return NANOARROW_OK; }, &WriteFieldJSON, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not strictly related but it showed as a warning we could control with the Meson warning configuration
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense to me! |
||
| R"({"name": null, "nullable": true, "type": {"name": "null"}, "children": []})", | ||
| [](TestingJSONWriter& writer) { writer.set_include_metadata(false); }); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| # Licensed to the Apache Software Foundation (ASF) under one | ||
| # or more contributor license agreements. See the NOTICE file | ||
| # distributed with this work for additional information | ||
| # regarding copyright ownership. The ASF licenses this file | ||
| # to you under the Apache License, Version 2.0 (the | ||
| # "License"); you may not use this file except in compliance | ||
| # with the License. You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, | ||
| # software distributed under the License is distributed on an | ||
| # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| # KIND, either express or implied. See the License for the | ||
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| [wrap-file] | ||
| directory = flatcc-0.6.1 | ||
| source_url = https://github.com/dvidelabs/flatcc/archive/refs/tags/v0.6.1.tar.gz | ||
| source_filename = flatcc-0.6.1.tar.gz | ||
| source_hash = 2533c2f1061498499f15acc7e0937dcf35bc68e685d237325124ae0d6c600c2b | ||
| patch_directory = flatcc |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| # Licensed to the Apache Software Foundation (ASF) under one | ||
| # or more contributor license agreements. See the NOTICE file | ||
| # distributed with this work for additional information | ||
| # regarding copyright ownership. The ASF licenses this file | ||
| # to you under the Apache License, Version 2.0 (the | ||
| # "License"); you may not use this file except in compliance | ||
| # with the License. You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, | ||
| # software distributed under the License is distributed on an | ||
| # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| # KIND, either express or implied. See the License for the | ||
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| project('flatcc', | ||
| 'c', | ||
| version : '0.6.1', | ||
| license : 'Apache-2.0', | ||
| ) | ||
|
|
||
| incdir = include_directories('include') | ||
|
|
||
| flatcc_lib = library( | ||
| 'flatcc', | ||
| sources: [ | ||
| 'src/runtime/builder.c', | ||
| 'src/runtime/emitter.c', | ||
| 'src/runtime/verifier.c', | ||
| 'src/runtime/refmap.c', | ||
| ], | ||
| include_directories: [incdir], | ||
| ) | ||
|
|
||
| flatcc_dep = declare_dependency( | ||
| include_directories: [incdir], | ||
| link_with: [flatcc_lib], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| # Licensed to the Apache Software Foundation (ASF) under one | ||
| # or more contributor license agreements. See the NOTICE file | ||
| # distributed with this work for additional information | ||
| # regarding copyright ownership. The ASF licenses this file | ||
| # to you under the Apache License, Version 2.0 (the | ||
| # "License"); you may not use this file except in compliance | ||
| # with the License. You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, | ||
| # software distributed under the License is distributed on an | ||
| # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| # KIND, either express or implied. See the License for the | ||
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| [wrap-file] | ||
| directory = zlib-1.3.1 | ||
| source_url = http://zlib.net/fossils/zlib-1.3.1.tar.gz | ||
| source_fallback_url = https://github.com/mesonbuild/wrapdb/releases/download/zlib_1.3.1-1/zlib-1.3.1.tar.gz | ||
| source_filename = zlib-1.3.1.tar.gz | ||
| source_hash = 9a93b2b7dfdac77ceba5a558a580e74667dd6fede4585b91eefb60f03b72df23 | ||
| patch_filename = zlib_1.3.1-1_patch.zip | ||
| patch_url = https://wrapdb.mesonbuild.com/v2/zlib_1.3.1-1/get_patch | ||
| patch_hash = e79b98eb24a75392009cec6f99ca5cdca9881ff20bfa174e8b8926d5c7a47095 | ||
| wrapdb_version = 1.3.1-1 | ||
|
|
||
| [provide] | ||
| zlib = zlib_dep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should really move the
meson.buildfrom the src/nanoarrow directory up to here to mirror what is done for CMake, but am leaving that for a separate PR. For now just put this apps piece up here since it does not live in src/nanoarrow