fix: Fix Meson build for separated nanoarrow_testing target#574
fix: Fix Meson build for separated nanoarrow_testing target#574paleolimbot merged 2 commits intoapache:mainfrom
Conversation
WillAyd
left a comment
WillAyd
left a comment
There was a problem hiding this comment.
Looks great - just a few comments / questions
| link_with: nanoarrow_lib, | ||
| include_directories: incdir, | ||
| dependencies: config['deps'], | ||
| dependencies: [nanoarrow_testing_dep, nanoarrow_dep, arrow_dep, gtest_dep, gmock_dep], |
There was a problem hiding this comment.
Not sure if it matters too much, but nanoarrow_dep is duplicative here, since nanoarrow_testing_dep already includes that in its declare_dependency declaration
There was a problem hiding this comment.
Does it matter that nanoarrow_testing_dep doesn't list link_with: nanoarrow_lib? Or maybe that would only matter with hidden visiblity? There were some places that listed nanoarrow_ipc_dep and nanoarrow_dep, which I tried to replicate here, but I really have no idea 😬
There was a problem hiding this comment.
Don't think so - I believe the nanoarrow_dep it has as a dependency will take care of that. Using CMake as an analogy, I think:
nanoarrow_dep = declare_dependency(link_with: nanoarrow_lib, ...)is akin to using the PUBLIC keyword when setting the link libraries of a target
target_link_libraries(nanoarrow PUBLIC nanoarrow_lib)i.e. it becomes transitive for any target that references the dependency
There was a problem hiding this comment.
(in the CMake example, nanoarrow would probably be an INTERFACE library)
| option('namespace', type: 'string', | ||
| description: 'A prefix for exported symbols') | ||
| option('device', type: 'boolean', description: 'Build device libraries', value: false) | ||
| option('testing', type: 'boolean', description: 'Build testing libraries', value: false) |
There was a problem hiding this comment.
Is there any advantage to adding this as a new build option versus just having it tied to the existing tests option?
There was a problem hiding this comment.
I think so...one could use the testing library from another project, too (in which case you probably wouldn't want to build nanoarrow's tests too).
WillAyd
left a comment
WillAyd
left a comment
There was a problem hiding this comment.
nice work. you are becoming a meson expert
2 participants
After #561 , the nanoarrow_testing library is no longer header-only and requires linking!