-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
GH-45800: [C++] Implement util configuration in Meson #45824
base: main
Are you sure you want to change the base?
Conversation
|
|
@github-actions crossbow submit *meson |
Revision: 67896cb Submitted crossbow builds: ursacomputing/crossbow @ actions-b05af5f8a2
|
@github-actions crossbow submit *meson |
Revision: f5cebe7 Submitted crossbow builds: ursacomputing/crossbow @ actions-8cfff641da
|
cpp/src/arrow/util/meson.build
Outdated
if host_machine.system() == 'windows' | ||
# This manifest enables long file paths on Windows 10+ | ||
# See https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#enable-long-paths-in-windows-10-version-1607-and-later | ||
if cpp_compiler.get_id() == 'msvc' | ||
io_util_test_sources = ['io_util_test.cc', 'io_util_test.manifest'] | ||
else | ||
io_util_test_sources = ['io_util_test.cc', 'io_util_test.rc'] | ||
endif | ||
else | ||
io_util_test_sources = ['io_util_test.cc'] | ||
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.
if host_machine.system() == 'windows' | |
# This manifest enables long file paths on Windows 10+ | |
# See https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#enable-long-paths-in-windows-10-version-1607-and-later | |
if cpp_compiler.get_id() == 'msvc' | |
io_util_test_sources = ['io_util_test.cc', 'io_util_test.manifest'] | |
else | |
io_util_test_sources = ['io_util_test.cc', 'io_util_test.rc'] | |
endif | |
else | |
io_util_test_sources = ['io_util_test.cc'] | |
endif | |
io_util_test_sources = ['io_util_test.cc'] | |
if host_machine.system() == 'windows' | |
# This manifest enables long file paths on Windows 10+ | |
# See https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#enable-long-paths-in-windows-10-version-1607-and-later | |
if cpp_compiler.get_id() == 'msvc' | |
io_util_test_sources += ['io_util_test.manifest'] | |
else | |
io_util_test_sources += ['io_util_test.rc'] | |
endif | |
endif |
Can we use the following?
utility_test_srcs = [
...,
'io_util_test.cc'
...
]
if host_machine.system() == 'windows'
# This manifest enables long file paths on Windows 10+
# See https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#enable-long-paths-in-windows-10-version-1607-and-later
if cpp_compiler.get_id() == 'msvc'
utility_test_srcs += ['io_util_test.manifest']
else
utility_test_srcs += ['io_util_test.rc']
endif
endif
@github-actions crossbow submit *meson |
Revision: 3903125 Submitted crossbow builds: ursacomputing/crossbow @ actions-f5d42b5ad9
|
3903125
to
d1fcfed
Compare
@github-actions crossbow submit *meson |
Revision: d1fcfed Submitted crossbow builds: ursacomputing/crossbow @ actions-fbda2d1a9d
|
}, | ||
} | ||
|
||
if needs_tests |
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 this?
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 do. I think there are ways to improve the terminology, but technically the arrow_test_dep
becomes a disabler only if needs_testing
is false. It is possible for needs_tests
to be false while needs_testing
is true. Without this extra condition these tests would build when -Dtests=false
and -Dtesting=true
I do wonder if it wouldn't be better to have an arrow_test_dep
and separately an arrow_testing_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.
If we need this here, we need it for test('arrow-utility-test', exc)
too?
It seems that we don't need both arrow_test_dep
and arrow_testing_dep
. Can we use this instead?
if needs_tests
arrow_test_dep = declare_dependency(
link_with: [arrow_test_lib],
dependencies: [arrow_dep, filesystem_dep, gmock_dep, gtest_main_dep],
)
else
arrow_test_dep = disabler()
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.
Nice catch on the arrow-utility-test - I can do that.
With respect to the if needs_tests
, it depends how much we want to adhere to what CMake has in place. In fact, we used to use that same condition, but I had to change it from if needs_tests
to if needs_testing
in the benchmarks PR.
https://github.com/apache/arrow/pull/45793/files
Unless I am misreading the CMake configuration (which is certainly possible) it seems like it is possible to disable tests but turn the benchmarks on. In such a case, you would still need the current arrow_test_dep
dependency to build benchmarks; the change you are proposing would add a dependency between the test suite and benchmark suite
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 separated dependency for benchmarks something like the following?
if needs_tests
arrow_test_dep = declare_dependency(
link_with: [arrow_test_lib],
dependencies: [arrow_dep, filesystem_dep, gmock_dep, gtest_main_dep],
)
else
arrow_test_dep = disabler()
endif
if needs_benchmarks
arrow_benchmark_dep = declare_dependency(
dependencies: [benchmark_dep, arrow_test_dep],
)
else
arrow_benchmark_dep = disabler()
end
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.
Yes we can. Nice idea!
exc = executable( | ||
key, | ||
sources: val['sources'], | ||
dependencies: [arrow_test_dep, val.get('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.
Do we need gtest_main_dep
here?
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_test_dep
already provides gtest_main_dep
transitively
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, arrow_test_dep
doesn't link to only libarrow_testing.so
.
Then can we remove filesystem_dep
from arrow-crc32-test
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 nice catch - yea we can do that for now
Rationale for this change
This continues to add support for Meson in the Arrow C++ codebase
What changes are included in this PR?
The util directory has been added to the Meson configuration
Are these changes tested?
Yes
Are there any user-facing changes?
No