Skip to content

GH-46827: [C++] Update Meson Configuration for compute shared lib#46839

Merged
raulcd merged 9 commits intoapache:mainfrom
WillAyd:fix-meson-compute2
Jun 25, 2025
Merged

GH-46827: [C++] Update Meson Configuration for compute shared lib#46839
raulcd merged 9 commits intoapache:mainfrom
WillAyd:fix-meson-compute2

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 from cd2f9f4 to 30afe0a Compare June 18, 2025 13:52
@WillAyd WillAyd force-pushed the fix-meson-compute2 branch 5 times, most recently from aa1e82b to 4d01c2c Compare June 19, 2025 02:02
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!

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

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 awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jun 19, 2025
@WillAyd WillAyd force-pushed the fix-meson-compute2 branch 3 times, most recently from 3ae0327 to 20dd3df Compare June 19, 2025 15:21
@WillAyd WillAyd force-pushed the fix-meson-compute2 branch from 927b44d to d8a3ca0 Compare June 23, 2025 18:52
@WillAyd
Copy link
Contributor Author

WillAyd commented Jun 24, 2025

We are all green - any other feedback on this?

@raulcd raulcd added the CI: Extra Run extra CI label Jun 25, 2025
Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

Thanks for following up this! I've taken a look and looks good to me, I am not an expert on this but matches what was done at CMake level.
I'll let @kou take a look and merge

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 25, 2025
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@raulcd raulcd merged commit e6cef22 into apache:main Jun 25, 2025
45 of 46 checks passed
@raulcd raulcd removed the awaiting merge Awaiting merge label Jun 25, 2025
@WillAyd WillAyd deleted the fix-meson-compute2 branch June 25, 2025 13:36
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit e6cef22.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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.

3 participants