Skip to content

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

WillAyd
Copy link
Contributor

@WillAyd WillAyd commented Jun 17, 2025

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

@WillAyd
Copy link
Contributor Author

WillAyd commented Jun 17, 2025

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

@WillAyd WillAyd closed this Jun 17, 2025
@WillAyd WillAyd requested a review from westonpace as a code owner June 17, 2025 19:56
@WillAyd WillAyd reopened this Jun 17, 2025
@WillAyd WillAyd force-pushed the fix-meson-compute2 branch from c2de059 to 5123728 Compare June 17, 2025 23:43
@WillAyd WillAyd changed the title Fix meson compute2 GH-46827: [C++] Update Meson Configuration for compute shared lib Jun 18, 2025
@WillAyd WillAyd force-pushed the fix-meson-compute2 branch from 5123728 to cd2f9f4 Compare June 18, 2025 01:18
@WillAyd
Copy link
Contributor Author

WillAyd commented Jun 18, 2025

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:

 RUN      ] Substrait.ExecReadRelWithLocalFiles
C:/projects/arrow/cpp/src/arrow/engine/substrait/serde_test.cc(1132): error: Failed
'_error_or_value131.status()' failed with Invalid: Cannot parse URI: 'file://C:projectsarrowcppsubmodulesparquet-testingdata/byte_stream_split.zstd.parquet' due to syntax error at character '/' (position 54)

In the AppVeyor build we set a variable like:

set PARQUET_TEST_DATA=%CD%\cpp\submodules\parquet-testing\data

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

@WillAyd WillAyd force-pushed the fix-meson-compute2 branch 4 times, most recently from aa1e82b to 4d01c2c Compare June 19, 2025 02:02
if needs_compute
arrow_c_bridge_deps = [arrow_compute_test_dep]
else
arrow_c_bridge_deps = []
Copy link
Member

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?

Suggested change
arrow_c_bridge_deps = []
arrow_c_bridge_deps = [arrow_test_dep]

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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!

PUBLIC $<TARGET_OBJECTS:arrow_compute_core_testing>
PUBLIC ${ARROW_GTEST_GTEST})
endif()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

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
Copy link
Member

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?

Copy link
Contributor Author

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:

Copy link
Contributor Author

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

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jun 19, 2025
@WillAyd WillAyd force-pushed the fix-meson-compute2 branch from 4d01c2c to 06ca403 Compare June 19, 2025 03:43
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jun 19, 2025
@github-actions github-actions bot added the awaiting changes Awaiting changes label Jun 19, 2025
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 19, 2025
@WillAyd WillAyd force-pushed the fix-meson-compute2 branch from 0b0100d to 166e38c Compare June 19, 2025 13:58
@WillAyd WillAyd force-pushed the fix-meson-compute2 branch from 166e38c to 66cae6a Compare June 19, 2025 14:20
@WillAyd WillAyd force-pushed the fix-meson-compute2 branch from 3ae0327 to 20dd3df Compare June 19, 2025 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants