-
Notifications
You must be signed in to change notification settings - Fork 3.8k
GH-46797: [C++] Support Meson on Windows #47282
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
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format?
or
See also: |
@@ -347,7 +347,7 @@ Result<std::shared_ptr<ArrayData>> PreallocateRunEndsArray( | |||
/// \param has_validity_buffer a validity buffer must be allocated | |||
/// \param length the length of the values array | |||
/// \param data_buffer_size the size of the data buffer for string and binary types | |||
ARROW_COMPUTE_EXPORT Result<std::shared_ptr<ArrayData>> PreallocateValuesArray( | |||
ARROW_EXPORT Result<std::shared_ptr<ArrayData>> PreallocateValuesArray( |
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.
Although this file is in the compute/kernels tree, the associated module is built as part of the arrow lib irrespective of whether compute is enabled or not. As such, I figured it made more sense to use the EXPORT semantics from the arrow lib rather than arrow_compute
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.
Really?
It seems that it's used by libarrow_compute
not libarrow
:
arrow/cpp/src/arrow/CMakeLists.txt
Line 767 in 97d7cdb
compute/kernels/ree_util_internal.cc |
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 you are right - I can fix this
@@ -52,6 +52,7 @@ if(ARROW_TESTING AND ARROW_COMPUTE) | |||
target_link_libraries(arrow_compute_testing | |||
PUBLIC $<TARGET_OBJECTS:arrow_compute_core_testing> | |||
PUBLIC ${ARROW_GTEST_GTEST_MAIN}) | |||
target_compile_definitions(arrow_compute_testing PRIVATE COMPILED_BY_CMAKE) |
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.
Not a huge fan of this, but there appears to be an issue with the registry on MSVC with the Meson configuration that isn't in the CMake config.
I think this has to do with the CMake config forcing a certain type of linkage, but I'm not sure yet. I am also under the impression that the Meson/non-MSVC path is preferred, so we might prefer to "fix" CMake rather than make the Meson configuration match it
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. What is the difference between Meson build and CMake build? GTest version?
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/util/meson.build
Outdated
@@ -224,18 +222,25 @@ utility_test_srcs = [ | |||
] | |||
|
|||
if host_machine.system() == 'windows' | |||
# meson does not natively support manifest files like CMake does |
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 don't think the Meson support for manifest files is as nice as CMake's, but I'm also not super familiar with them. Need to research this a bit more...
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.
mesonbuild/meson#13041 may be related.
/MANIFEST:EMBED /MANIFESTINPUT:io_util_test.manifest
linker options may work.
See also: https://learn.microsoft.com/en-us/cpp/build/reference/manifestinput-specify-manifest-input?view=msvc-170
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.
Could you open an issue for this?
.github/workflows/cpp_extra.yml
Outdated
key: cpp-ccache-windows-${{ env.CACHE_VERSION }}-${{ hashFiles('cpp/**') }} | ||
restore-keys: cpp-ccache-windows-${{ env.CACHE_VERSION }}- |
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.
key: cpp-ccache-windows-${{ env.CACHE_VERSION }}-${{ hashFiles('cpp/**') }} | |
restore-keys: cpp-ccache-windows-${{ env.CACHE_VERSION }}- | |
key: cpp-ccache-windows-meson-${{ env.CACHE_VERSION }}-${{ hashFiles('cpp/**') }} | |
restore-keys: cpp-ccache-windows-meson-${{ env.CACHE_VERSION }}- |
.github/workflows/cpp_extra.yml
Outdated
shell: cmd | ||
run: | | ||
call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x64 | ||
bash -c "ARROW_USE_MESON=ON; ci/scripts/cpp_build.sh $(pwd) $(pwd)/build ${{ matrix.run-options || '' }}" |
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.
bash -c "ARROW_USE_MESON=ON; ci/scripts/cpp_build.sh $(pwd) $(pwd)/build ${{ matrix.run-options || '' }}" | |
bash -c "ci/scripts/cpp_build.sh $(pwd) $(pwd)/build" |
.github/workflows/cpp_extra.yml
Outdated
run: | | ||
# For ORC | ||
export TZDIR=/c/msys64/usr/share/zoneinfo | ||
ARROW_USE_MESON=ON ci/scripts/cpp_test.sh $(pwd) $(pwd)/build ${{ matrix.run-options || '' }} |
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.
ARROW_USE_MESON=ON ci/scripts/cpp_test.sh $(pwd) $(pwd)/build ${{ matrix.run-options || '' }} | |
ci/scripts/cpp_test.sh $(pwd) $(pwd)/build |
.github/workflows/cpp_extra.yml
Outdated
# We can invalidate the current cache by updating this. | ||
CACHE_VERSION: "2022-09-13" | ||
- name: Build | ||
shell: cmd |
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.
shell: cmd | |
shell: cmd | |
env: | |
ARROW_USE_MESON: ON |
call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x64 | ||
bash -c "ARROW_USE_MESON=ON; ci/scripts/cpp_build.sh $(pwd) $(pwd)/build ${{ matrix.run-options || '' }}" | ||
- name: Test | ||
shell: bash |
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.
shell: bash | |
shell: bash | |
env: | |
ARROW_USE_MESON: ON |
@@ -347,7 +347,7 @@ Result<std::shared_ptr<ArrayData>> PreallocateRunEndsArray( | |||
/// \param has_validity_buffer a validity buffer must be allocated | |||
/// \param length the length of the values array | |||
/// \param data_buffer_size the size of the data buffer for string and binary types | |||
ARROW_COMPUTE_EXPORT Result<std::shared_ptr<ArrayData>> PreallocateValuesArray( | |||
ARROW_EXPORT Result<std::shared_ptr<ArrayData>> PreallocateValuesArray( |
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.
Really?
It seems that it's used by libarrow_compute
not libarrow
:
arrow/cpp/src/arrow/CMakeLists.txt
Line 767 in 97d7cdb
compute/kernels/ree_util_internal.cc |
cpp/src/arrow/meson.build
Outdated
if host_machine.system() != 'windows' | ||
dl_dep = dependency('dl') | ||
else | ||
dl_dep = declare_dependency() | ||
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.
Could you swap these branches for easy to read? (In general, no "not" is easier to read.)
if host_machine.system() != 'windows' | |
dl_dep = dependency('dl') | |
else | |
dl_dep = declare_dependency() | |
endif | |
if host_machine.system() == 'windows' | |
dl_dep = declare_dependency() | |
else | |
dl_dep = dependency('dl') | |
endif |
cpp/src/arrow/meson.build
Outdated
) | ||
|
||
arrow_compile_args = [] | ||
if host_machine.system() == 'windows' and get_option('default_library') != 'shared' |
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.
We can always define ARROW_STATIC
:
if host_machine.system() == 'windows' and get_option('default_library') != 'shared' | |
if get_option('default_library') != 'shared' |
BTW, does this work with default_library=both
?
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 haven't gotten default_library=both
to work yet, but haven't put a ton of effort into debugging it either. Happy to do so after getting shared/static support stabilized
@@ -484,14 +505,22 @@ if needs_compute | |||
arrow_compute_lib = library( | |||
'arrow-compute', | |||
sources: arrow_compute_lib_sources, | |||
dependencies: arrow_dep, | |||
dependencies: [arrow_dep] + int128_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.
Hmm. Why do we need this?
If libarrow
is a static library, it must have int128_deps
dependency. libarrow
consumers should not care about int128_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.
This is to build the library itself, not to expose it to consumers. I believe that MSVC/Windows do not offer a native 128 bit integer, so the int128_deps
grabs it from the boost multiprecision header. On other platforms, this dependency is empty
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, compute also uses arrow::internal::int128_t
. Sorry. I thought that decimal in libarrow only uses arrow::internal::int128_t
.
cpp/src/arrow/util/meson.build
Outdated
@@ -224,18 +222,25 @@ utility_test_srcs = [ | |||
] | |||
|
|||
if host_machine.system() == 'windows' | |||
# meson does not natively support manifest files like CMake does |
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.
mesonbuild/meson#13041 may be related.
/MANIFEST:EMBED /MANIFESTINPUT:io_util_test.manifest
linker options may work.
See also: https://learn.microsoft.com/en-us/cpp/build/reference/manifestinput-specify-manifest-input?view=msvc-170
|
Sorry about that - there is an open issue I just messed up the commit name. I've updated the title now and can squash on next push |
c3b74be
to
d7c42c4
Compare
.github/workflows/cpp_extra.yml
Outdated
call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x64 | ||
echo %PATH% | ||
dir "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\bin\HostX64\x64" | ||
bash -c "ci/scripts/cpp_build.sh $(pwd) $(pwd)/build ${{ matrix.run-options || '' }}" |
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 believe the current error has to do with the fact that calling `bash -c link ..." invokes the linker that comes bundled with bash, and not the linker that the vcvarsall.bat script is supposed to activate. You can reproduce this locally:
> link /help
Microsoft (R) Incremental Linker Version 14.44.35214.0
Copyright (C) Microsoft Corporation. All rights reserved.
For help on Linker, type `link /link' or `link'
For help on Library Manager, type `link /lib' or `lib'
For help on Dumper, type `link /dump' or `dumpbin'
For help on Editor, type `link /edit' or `editbin'
For help on CvtCIL, type `link /cvtcil'
For help on PushThunkObj Generator, type `link /pushthunkobj'
> bash -c "link /help"
link: missing operand after ‘/help’
Try 'link --help' for more information.
> bash -c "link --help"
Usage: link FILE1 FILE2
or: link OPTION
Call the link function to create a link named FILE2 to an existing FILE1.
--help display this help and exit
--version output version information and exit
GNU coreutils online help: <https://www.gnu.org/software/coreutils/>
Report any translation bugs to <https://translationproject.org/team/>
Full documentation <https://www.gnu.org/software/coreutils/link>
or available locally via: info '(coreutils) link invocation'
I guess the current CMake configuration doesn't care about this, although it makes sense for Meson to be suspect of this environment.
Maybe its worth creating a separate cpp_build.bat
script for Windows platforms to use? What do you think @kou ?
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.
How about trying meson setup --vsenv
instead of calling vcvarsall.bat
manually?
https://mesonbuild.com/Using-with-Visual-Studio.html
If no Visual Studio Command Prompt was detected, and no mingw compilers are detected either, meson will attempt to find "a" Visual Studio installation for you automatically, by asking Microsoft's "vswhere" program. If you want to ignore mingw compilers, pass the --vsenv option on the meson command line. If you need to guarantee a specific Visual Studio version, set it up manually.
aca8728
to
39c6cf0
Compare
24047ce
to
d9d1eed
Compare
OK looks like this is all green now. The Ubuntu Meson failure is unrelated and will be fixed by #47041 |
59aa36c
to
bc7694e
Compare
bc7694e
to
6d39114
Compare
I don't think the glib & ruby failure is related |
Correct: #46690 |
@kou do you have any thoughts on the content of the PR at this stage? |
.github/workflows/cpp_extra.yml
Outdated
restore-keys: cpp-ccache-windows-meson-${{ env.CACHE_VERSION }}- | ||
env: | ||
# We can invalidate the current cache by updating this. | ||
CACHE_VERSION: "2022-09-13" |
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.
Could you update this date?
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 not sure what date this is supposed to be so just picked today's. Hope that is OK!
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!
ci/scripts/cpp_test.sh
Outdated
--max-lines=0 \ | ||
--timeout-multiplier 10 \ |
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.
Can we unify --XXX=V
and -XXX V
style?
@@ -52,6 +52,7 @@ if(ARROW_TESTING AND ARROW_COMPUTE) | |||
target_link_libraries(arrow_compute_testing | |||
PUBLIC $<TARGET_OBJECTS:arrow_compute_core_testing> | |||
PUBLIC ${ARROW_GTEST_GTEST_MAIN}) | |||
target_compile_definitions(arrow_compute_testing PRIVATE COMPILED_BY_CMAKE) |
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. What is the difference between Meson build and CMake build? GTest version?
cpp/src/arrow/flight/flight_test.cc
Outdated
#ifndef ARROW_ENABLE_THREADING | ||
GTEST_SKIP() << "This test requires Boost.Process to run"; | ||
#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.
How about adding arrow::testing::Process::IsAvailable()
static method or something and use it here?
cpp/src/arrow/flight/meson.build
Outdated
@@ -44,7 +44,17 @@ install_headers( | |||
subdir: 'arrow/flight', | |||
) | |||
|
|||
grpc_dep = dependency('grpc++') | |||
# We specifically need to use the grpc subproject and cannot use a system installion | |||
# due to https://github.com/mesonbuild/meson/issues/14905 |
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.
Can we use https://mesonbuild.com/Reference-manual_returned_external_program.html#external_programfull_path as workaround?
If we can use grpc_cpp_plugin.full_path()
, can we keep supporting system gRPC?
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.
Using full_path like that is unfortunately not an option and will yield errors like:
ninja: error: 'grpc_cpp_plugin.exe', needed by 'src/arrow/flight/Flight.grpc.pb.h', missing and no known rule to make it
In any case the pkg-config name was always wrong. I've updated that here - maybe that was the problem?
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.
Actually I think this current iteration will have the same issue of mesonbuild/meson#14905 when there is a system install of grpc. Happy to leave as is though as it won't affect CI.
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.
I think the remaining issues are related to CMake and how it chooses to link the various test libraries, and what effects that has on the execution of them. I've opened the testing issue accordingly to take a look at that outside of this PR. Current CI failures I believe are also unrelated |
Rationale for this change
This allows users on the Windows platform to use the Meson build system
What changes are included in this PR?
Updates to the Meson configurations and source files to accommodate Windows
Are these changes tested?
Yes
Are there any user-facing changes?
No