feat: Meson build system for nanoarrow-ipc extension #483

WillAyd · 2024-05-23T19:47:38Z

No description provided.

WillAyd · 2024-05-23T19:48:34Z

Meson's handling of subprojects is way easier I think than CMake's. If down the road we wanted to replace CMake with Meson there is a ton that we could simplify within these extensions, and maybe could even consider them as separate projects

paleolimbot · 2024-05-23T19:51:31Z

Nice!

I've been wondering if the IPC and Device "extensions" should move into the main tree (e.g., #481 )...they started as separate projects but a few things like the "testing" module make it a little awkward to keep them separate. Any thoughts on that?

WillAyd · 2024-05-23T19:51:21Z

extensions/nanoarrow_ipc/subprojects/nanoarrow.wrap

+# under the License.
+
+[wrap-git]
+url = ../../..


In the main project right now we ask end users to run meson wrap install <lib>, but that doesn't make sense to do with this nanoarrow.wrap, so I ended up putting all the wrap files here

Maybe even in the main project we should just add the wrap files? That would technically pin a version for third party dependencies (if not installed elsewhere on the system) which may or may not be desired

It seems like this complexity would be solved if the IPC extension just lived with the main sources!

WillAyd · 2024-05-23T19:52:29Z

extensions/nanoarrow_ipc/subprojects/flatcc.wrap

+# under the License.
+
+[wrap-git]
+method = cmake


flatcc is working to replace (?) CMake with Meson. That would simplify the wrap / subproject usage for flatcc if and when it goes live

dvidelabs/flatcc#262

It seems like the intention is to support both, although we vendor flatcc anyway for now. I should be more active contributing to that upstream since the IPC reader 100% relies on it!

WillAyd · 2024-05-23T20:04:32Z

I haven't used these in particular so I'm just kind of guessing, but it feels like it would make sense to combine them. I think for consumers having to potentially install three different libraries for nanoarrow instead of just one library that you can choose the dependencies out of is overkill. And as you've mentioned already, there appears to be a ton of overlap between these projects' dependencies

WillAyd · 2024-05-23T20:06:23Z

extensions/nanoarrow_ipc/meson.build

+      instead of -DNANOARROW_IPC_CODE_COVERAGE=true''')
+endif
+
+if get_option('NANOARROW_IPC_BUNDLE')


@paleolimbot without being super knowledgeable on the end goal of these bundle options, are these used by consumers downstream or just for internal use? I see the Python build uses the bundle to auto generate a Cython header, but not sure of other uses cases and if its worth trying to replicate in Meson

I don't think there's any reason to expose this in Meson...this is currently how the files in dist/ are generated (e.g., the single-file bundles for each component). The fact that they are written in CMake is something that initially made it easier to test that the single-file distribution actually worked...I think it might make more sense to rewrite the amalgamation scripts in Python at some point though.

paleolimbot

I haven't used these in particular so I'm just kind of guessing, but it feels like it would make sense to combine them.

Great! I will aim for that in a near-term PR. I am happy to work through this and merge it before that refactor or afterward (your call!)

paleolimbot · 2024-05-24T01:14:12Z

extensions/nanoarrow_ipc/subprojects/flatcc.wrap

+# under the License.
+
+[wrap-git]
+method = cmake


It seems like the intention is to support both, although we vendor flatcc anyway for now. I should be more active contributing to that upstream since the IPC reader 100% relies on it!

paleolimbot · 2024-05-24T01:15:02Z

extensions/nanoarrow_ipc/subprojects/nanoarrow.wrap

+# under the License.
+
+[wrap-git]
+url = ../../..


It seems like this complexity would be solved if the IPC extension just lived with the main sources!

WillAyd · 2024-05-24T01:53:28Z

Let's just wait to merge this until after then - there is no rush for this and I think waiting will make that consolidation easier

WillAyd · 2024-06-10T20:38:56Z

meson.build

@@ -34,3 +34,14 @@ subdir('src/nanoarrow')
 if get_option('benchmarks')
  subdir('dev/benchmarks')
 endif
+
+
+if get_option('apps')


I should really move the meson.build from 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

WillAyd · 2024-06-10T20:39:31Z

src/nanoarrow/nanoarrow_testing_test.cc

@@ -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,


Not strictly related but it showed as a warning we could control with the Meson warning configuration

Makes sense to me!

WillAyd · 2024-06-10T23:15:18Z

Interesting to see the valgrind error in CI right now. I assume this is also why the windows build is failing, although surprised our CMake setup didn't catch it

https://github.com/apache/arrow-nanoarrow/actions/runs/9455933515/job/26046780100?pr=483#step:7:338

WillAyd · 2024-06-11T01:17:48Z

Seems like the error has to do with the fields being sent to ArrowErrorSet in this function:

arrow-nanoarrow/src/nanoarrow/array.c

Line 1247 in f443e46

if (array_view->storage_type == NANOARROW_TYPE_RUN_END_ENCODED) {

Not familiar with the run end encoded implementation but I think we are missing something with how fields are being initialized and accessed through that code block

paleolimbot

A few comments but generally looking great!

paleolimbot · 2024-06-11T12:26:45Z

src/apps/dump_stream.c

@@ -23,8 +23,6 @@

 void dump_schema_to_stdout(struct ArrowSchema* schema, int level, char* buf,
                           int buf_size) {
-  int n_chars = ArrowSchemaToString(schema, buf, buf_size, 0);


I think this is still needed (or else the top-level type will never appear in the dump). Maybe just remove int n_chars = (if the unused variable was a problem?)

paleolimbot · 2024-06-11T12:30:31Z

src/nanoarrow/nanoarrow_ipc_decoder_test.cc

+      auto result = memcmp(arrays[i].children[0]->buffers[1], one_two_three_le,
+                           sizeof(one_two_three_le));
+      // discard result to silence -Wunused-value
+      (void)result;


Would NANOARROW_UNUSED() work here?

paleolimbot · 2024-06-11T12:30:59Z

src/nanoarrow/nanoarrow_testing_test.cc

@@ -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,


Makes sense to me!

paleolimbot · 2024-06-11T12:32:25Z

dev/release/rat_exclude_files.txt

@@ -18,3 +18,4 @@ python/src/nanoarrow/dlpack_abi.h
 subprojects/google-benchmark.wrap
 subprojects/gtest.wrap
 subprojects/nlohmann_json.wrap
+subprojects/zlib.wrap


Is there a reason these are being ignored rather than just adding the Apache license header to the files? (It seems like those file types support comments and some of the wrap files already have the license header added)

The ones I have excluded were auto generated by meson wrap install <library> whereas the one that I created from scratch (flatcc.wrap) included the Arrow licensing agreement.

Not sure if that distinction matters. Happy to add license agreement to even the auto generated ones

I think it would be best to add the license headers rather than put it in the excluded files (although I think it is fine either way since it was generated for us).

paleolimbot · 2024-06-11T12:39:35Z

src/nanoarrow/meson.build

+    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')


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

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

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 subprojects/packagefiles/<patch_dir> and the wrap system still takes care of fetching / downloading the library for you, rather than requiring you to vendor into your source tree

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

paleolimbot · 2024-06-11T12:43:43Z

src/nanoarrow/meson.build

+              name + '-test',
+              name.replace('-', '_') + '_test.cc',
+              link_with: nanoarrow_ipc_lib,
+              dependencies: config['deps']


Would it be possible to factor more common dependencies like gtest_dep into link_with?

(Also: I don't think that nanoarrow_dep or flatcc_dep is needed because it should be handled as a transitive dependency of nanoarrow_ipc_lib? I might have put that in the CMake due to some weirdness about it being a library built on top of nanoarrow rather than part of nanoarrow itself)

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])

Not sure I understand the first question

It seemed like the loop was handling linking each test with nanoarrow_ipc_lib and I was wondering if the loop could also handle linking the dependencies that were common to every test executable. This is probably just me misunderstanding meson and feel free to ignore if it doesn't make sense 🙂

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

Great! I mostly just want to make sure my vauge mental model of what's going on here is correct 🙂

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 declare_dependency function:

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)

paleolimbot · 2024-06-11T12:53:08Z

ci/scripts/build-with-meson.sh

@@ -68,12 +68,12 @@ function main() {
    show_header "Run test suite"
    meson configure -Dtests=true -Db_coverage=true
    meson compile
-    meson test --wrap valgrind
+    meson test --wrap='valgrind --track-origins=yes' --print-errorlogs


I often use --tool=memcheck --leak-check=full (but this seems to give great output so take or leave!)

Sure can definitely add the leak-check=full - its just a tradeoff between performance and visibility. This runs pretty quick in CI as is

Got it! Let's leave it (the memcheck CI job + verification both already do the memcheck/full version).

paleolimbot

Thank you!

The Meson build failure is a separate issue related to attempting to print very large numbers (unrelated to this PR).

Follow up to #483

WillAyd commented May 23, 2024

View reviewed changes

paleolimbot reviewed May 24, 2024

View reviewed changes

WillAyd mentioned this pull request Jun 3, 2024

feat: Standalone Python installer using Meson #505

Closed

feat: Meson build support for IPC

e5133a7

WillAyd force-pushed the meson-for-ipc branch from 88f8ea8 to e5133a7 Compare June 10, 2024 20:37

WillAyd commented Jun 10, 2024

View reviewed changes

print-errorlogs

deb2f21

WillAyd added 13 commits June 10, 2024 19:58

Try track-origins=yes

0250989

debug build

668e110

remove debug

a6a7ea9

debugoptimized

a3d1812

remove debugoptimized

b5a9370

test code removal

61516cc

Comment out flush

e6f72df

try something else

702da7b

Try other error

262a973

Uncomment other error

1de36d2

try to src

aeea2c2

more comments

f5f8001

different message

df35957

revert

574bea9

paleolimbot reviewed Jun 11, 2024

View reviewed changes

feedback

859a3f2

WillAyd added 3 commits June 11, 2024 11:28

Merge remote-tracking branch 'upstream/main' into meson-for-ipc

10de273

License in subprojects

82a6b48

flatcc using meson

b878e23

paleolimbot approved these changes Jun 11, 2024

View reviewed changes

paleolimbot merged commit 41e9e71 into apache:main Jun 11, 2024
32 of 33 checks passed

WillAyd deleted the meson-for-ipc branch June 11, 2024 20:25

WillAyd mentioned this pull request Jun 12, 2024

refactor: Remove CMake requirement from Meson IPC config #522

Merged

paleolimbot pushed a commit that referenced this pull request Jun 12, 2024

refactor: Remove CMake requirement from Meson IPC config (#522)

d6eb52b

Follow up to #483

paleolimbot added this to the nanoarrow 0.6.0 milestone Sep 17, 2024

feat: Meson build system for nanoarrow-ipc extension #483

feat: Meson build system for nanoarrow-ipc extension #483

Conversation

WillAyd commented May 23, 2024

WillAyd commented May 23, 2024

paleolimbot commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paleolimbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Jun 10, 2024 • edited Loading

WillAyd commented Jun 11, 2024

paleolimbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paleolimbot left a comment

Choose a reason for hiding this comment

WillAyd commented Jun 10, 2024 •

edited

Loading

WillAyd Jun 11, 2024 •

edited

Loading