-
Notifications
You must be signed in to change notification settings - Fork 38
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
fix: Fix Meson include directories #532
Conversation
meson.build
Outdated
@@ -62,18 +38,20 @@ else | |||
libtype = 'library' | |||
endif | |||
|
|||
subdir('src') |
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'm a little less sure on the best way to make this work, but the ultimate problem I'm trying to solve is that when using nanoarrow as a subproject you end up with:
../subprojects/nanoarrow/src/nanoarrow/nanoarrow_types.h:24:10: fatal error: nanoarrow_config.h: No such file or directory
24 | #include "nanoarrow_config.h"
| ^~~~~~~~~~~~~~~~~~~~
The CMake file configuration allows you to write directly to the source which is why that relative import works better. For Meson, we would have to either generate this header in src/nanoarrow
and have the dependency include that path or generate in src
as is done here, which the dependency already resolves includes to for supporting things like #include nanoarrow/<>.h
Another approach would be to change the include to #include <nanoarrow/nanoarrow_config.h>
in nanoarrow_types.h and generate the configuration file in src/nanoarrow
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.
Another approach would be to change the include to
#include <nanoarrow/nanoarrow_config.h>
innanoarrow_types.h
and generate the configuration file insrc/nanoarrow
I think that #include <nanoarrow/nanoarrow_config.h>
is probably what I intended (but forgot to change!)
src/nanoarrow/nanoarrow_types.h
Outdated
@@ -21,7 +21,7 @@ | |||
#include <stdint.h> | |||
#include <string.h> | |||
|
|||
#include "nanoarrow_config.h" | |||
#include <nanoarrow/nanoarrow_config.h> |
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.
#include <nanoarrow/nanoarrow_config.h> | |
#include "nanoarrow/nanoarrow_config.h" |
(Perhaps less correct, but more consistent with all the other places we currently do this)
I think the current issue has to do with the bundler - is the current bundling script supposed to replace the "nanoarrow/" prefix in the include? |
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.
Is there any way to do this without modifying the CMake configuration? I don't have a canonical reference for it but having this file be generated outside the source tree has helped diagnose a few bugs over the course of the build system history.
CMakeLists.txt
Outdated
@@ -138,7 +138,7 @@ add_library(nanoarrow::nanoarrow ALIAS nanoarrow) | |||
|
|||
target_include_directories(nanoarrow | |||
PUBLIC $<BUILD_INTERFACE:${NANOARROW_BUILD_INCLUDE_DIR}> | |||
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/generated> | |||
$<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/src> |
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.
$<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/src> | |
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src> |
...maybe?
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 think this only matters for non-bundled builds, which appear to be working correctly (at least locally for me). The nanoarrow_config.h file is getting placed in the build directory /src/nanoarrow, so this include I think is correct
I believe the issue has to do with the bundler. On main it seems to be removing `#include "nanoarrow_config.h" but on this branch #include "nanoarrow/nanoarrow_config.h" remains
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.
Possibly, although ${CMAKE_BINARY_DIR}/src
never exists (which is why I suggested this). The CI seems to be failing for the IPC and device components also (which are not bundled).
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.
Yea I missed replacing those references that previously went to /generated
to /src
FWIW the current CMake configuration creates a variable via set(NANOARROW_BUILD_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/src")
. Though its called NANOARROW_BUILD_INCLUDE_DIR
it refers to the source directory, not the build location. Maybe worth renaming to keep that clearer?
Yea we could also have Meson generated the file in |
Got it! Generating in the source directory is fine, I was just curious 🙂 |
Ah sorry - should have been clearer in my previous comment that I mean Meson is super strict about where files can be generated. I'm not sure it would let you ever create something in the source directory, but if it did you'd be fighting against Meson to make it work. CMake is obviously a lot more relaxed, but I think it still makes sense to avoid generating anything in the source directory |
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.
Thank you!
Quick follow up to #508 - the array.c includes stopped working without this