-
Notifications
You must be signed in to change notification settings - Fork 3.7k
GH-46827: [C++] Update Meson Configuration for compute shared lib #46839
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
base: main
Are you sure you want to change the base?
Conversation
This was a continuation of #46830 which I thought got borked somehow, but I'm guessing github is just in the middle of some service outages |
c2de059
to
5123728
Compare
5123728
to
cd2f9f4
Compare
Unfortunately I don't think my prior PR is recoverable - I think it got borked in an outage, so we maybe need to continue along here @kou @raulcd I think I've addressed all the feedback. With respect to the AppVeyor failure, it looks like the previous failures occurred with Visual Studio 2019, and I've made some macros to handle that accordingly. However, I now see the following AppVeyor error:
In the AppVeyor build we set a variable like:
and the failing test does a string replace with that against: "uriFile": "file://[DIRECTORY_PLACEHOLDER]/byte_stream_split.zstd.parquet", So I guess Visual Studio is not happy with the mix of forward/backslashes, although I'm still stumped as to why the changes in this PR would bring that to light |
cd2f9f4
to
30afe0a
Compare
30afe0a
to
438f7b1
Compare
aa1e82b
to
4d01c2c
Compare
cpp/src/arrow/c/meson.build
Outdated
if needs_compute | ||
arrow_c_bridge_deps = [arrow_compute_test_dep] | ||
else | ||
arrow_c_bridge_deps = [] |
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.
Do we need arrow_test_dep
here?
arrow_c_bridge_deps = [] | |
arrow_c_bridge_deps = [arrow_test_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.
It is not required because the arrow_compute_test_dep
includes arrow_test_dep_no_main
transitively. Maybe there is better naming we can use for the dependencies?
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.
Ah, I want to focus on if not needs_compute
branch here. In the branch, we want to run bridge_test.cc
without compute support.
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.
Oh I see what you mean - nice catch!
cpp/src/arrow/compute/CMakeLists.txt
Outdated
PUBLIC $<TARGET_OBJECTS:arrow_compute_core_testing> | ||
PUBLIC ${ARROW_GTEST_GTEST}) | ||
endif() | ||
|
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.
cpp/src/arrow/compute/CMakeLists.txt
Outdated
target_link_libraries(arrow_compute_testing | ||
PUBLIC $<TARGET_OBJECTS:arrow_compute_core_testing> | ||
PUBLIC ${ARROW_GTEST_GTEST_MAIN}) | ||
if(MSVC AND MSVC_VERSION LESS 1930) # gtest linkage needs compat on Visual Studio 2019 |
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.
Hmm. Is it really related to Visual Studio version?
It seems that this will be happened with newer Visual Studio.
Could you share the build log URL for this change?
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.
It appears so. Here is the last build log URL before I added this:
https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/52249396
So the failure messages looked like:
[----------] 18 tests from TestPivotKernel
[ RUN ] TestPivotKernel.Basics
unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.
Stack trace:
[ FAILED ] TestPivotKernel.Basics (2 ms)
[ RUN ] TestPivotKernel.BinaryKeyTypes
unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.
Stack trace:
[ FAILED ] TestPivotKernel.BinaryKeyTypes (1 ms)
[ RUN ] TestPivotKernel.IntegerKeyTypes
unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.
Stack trace:
[ FAILED ] TestPivotKernel.IntegerKeyTypes (1 ms)
[ RUN ] TestPivotKernel.Numbers
unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.
Stack trace:
[ FAILED ] TestPivotKernel.Numbers (1 ms)
[ RUN ] TestPivotKernel.Binary
unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.
Stack trace:
[ FAILED ] TestPivotKernel.Binary (0 ms)
[ RUN ] TestPivotKernel.NullType
unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.
Stack trace:
[ FAILED ] TestPivotKernel.NullType (0 ms)
[ RUN ] TestPivotKernel.NullValues
unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.
Stack trace:
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.
OK without the macro the SEH exceptions are gone, so maybe that was a temporal issue with AppVeyor. However, I still get the error about the invalid file path without this macro, which you can see in the latest run here:
https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/52266070
In the interim, I fixed up the macro (changed _MSVC_VER
-> _MSC_VER
) and it looks like things are all green, so hopefully that works for now
4d01c2c
to
06ca403
Compare
0b0100d
to
166e38c
Compare
166e38c
to
66cae6a
Compare
3ae0327
to
20dd3df
Compare
This reverts commit 787016b.
Rationale for this change
The major refactor of the compute sources in #46261 addressed updates to the CMake configuration but not Meson. This is a follow up to get the Meson builds working again
What changes are included in this PR?
Meson configuration files are updated to reflect new source structure. gtest has also been bumped to a new WrapDB version, which fixes some undefined behavior that was compounded by updates to the compute test structure
Are these changes tested?
Yes
Are there any user-facing changes?
No