Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+244 −1
Open

GH-45800: [C++] Implement util configuration in Meson #45824

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67896cb
GH-45800: [C++] Implement util configuration in Meson
WillAyd Mar 14, 2025
f5cebe7
Fix installation directories for headers
WillAyd Mar 17, 2025
d1fcfed
kou feedback
WillAyd Mar 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion cpp/src/arrow/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ arrow_components = {
'util/logger.cc',
'util/logging.cc',
'util/key_value_metadata.cc',
'util/math_internal.cc',
'util/memory.cc',
'util/mutex.cc',
'util/ree_util.cc',
Expand Down Expand Up @@ -352,7 +353,7 @@ install_headers(
'visit_scalar_inline.h',
'visit_type_inline.h',
],
install_dir: 'arrow',
subdir: 'arrow',
)

if needs_testing
Expand All @@ -371,10 +372,12 @@ if needs_testing
filesystem_dep = boost_proj.dependency('boost_filesystem')
endif

gtest_dep = dependency('gtest')
gtest_main_dep = dependency('gtest_main')
gmock_dep = dependency('gmock')
else
filesystem_dep = disabler()
gtest_dep = disabler()
gtest_main_dep = disabler()
gmock_dep = disabler()
endif
Expand Down
1 change: 1 addition & 0 deletions cpp/src/arrow/testing/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ install_headers(
'util.h',
'visibility.h',
],
subdir: 'arrow/testing',
)

if needs_tests
Expand Down
240 changes: 240 additions & 0 deletions cpp/src/arrow/util/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,3 +92,243 @@ configure_file(
configuration: internal_conf_data,
format: 'cmake@',
)

install_headers(
[
'algorithm.h',
'aligned_storage.h',
'align_util.h',
'async_generator_fwd.h',
'async_generator.h',
'async_util.h',
'base64.h',
'basic_decimal.h',
'benchmark_util.h',
'binary_view_util.h',
'bit_block_counter.h',
'bitmap_builders.h',
'bitmap_generate.h',
'bitmap.h',
'bitmap_ops.h',
'bitmap_reader.h',
'bitmap_visit.h',
'bitmap_writer.h',
'bit_run_reader.h',
'bitset_stack.h',
'bit_util.h',
'bpacking64_default.h',
'bpacking_avx2.h',
'bpacking_avx512.h',
'bpacking_default.h',
'bpacking.h',
'bpacking_neon.h',
'byte_size.h',
'cancel.h',
'checked_cast.h',
'compare.h',
'compression.h',
'concurrent_map.h',
'converter.h',
'counting_semaphore.h',
'cpu_info.h',
'crc32.h',
'debug.h',
'decimal.h',
'delimiting.h',
'dict_util.h',
'dispatch.h',
'double_conversion.h',
'endian.h',
'float16.h',
'formatting.h',
'functional.h',
'future.h',
'hashing.h',
'hash_util.h',
'int_util.h',
'int_util_overflow.h',
'io_util.h',
'iterator.h',
'key_value_metadata.h',
'launder.h',
'list_util.h',
'logger.h',
'logging.h',
'macros.h',
'map.h',
'math_constants.h',
'memory.h',
'mutex.h',
'parallel.h',
'pcg_random.h',
'prefetch.h',
'print.h',
'queue.h',
'range.h',
'ree_util.h',
'regex.h',
'rows_to_batches.h',
'simd.h',
'small_vector.h',
'sort.h',
'spaced.h',
'span.h',
'stopwatch.h',
'string_builder.h',
'string.h',
'task_group.h',
'tdigest.h',
'test_common.h',
'thread_pool.h',
'time.h',
'tracing.h',
'trie.h',
'type_fwd.h',
'type_traits.h',
'ubsan.h',
'union_util.h',
'unreachable.h',
'uri.h',
'utf8.h',
'value_parsing.h',
'vector.h',
'visibility.h',
'windows_compatibility.h',
'windows_fixup.h',
],
subdir: 'arrow/util',
)

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
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
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


utility_test_srcs = [
'align_util_test.cc',
'atfork_test.cc',
'byte_size_test.cc',
'byte_stream_split_test.cc',
'cache_test.cc',
'checked_cast_test.cc',
'compression_test.cc',
'decimal_test.cc',
'float16_test.cc',
'fixed_width_test.cc',
'formatting_util_test.cc',
'key_value_metadata_test.cc',
'hashing_test.cc',
'int_util_test.cc',
'iterator_test.cc',
'list_util_test.cc',
'logger_test.cc',
'logging_test.cc',
'math_test.cc',
'queue_test.cc',
'range_test.cc',
'ree_util_test.cc',
'reflection_test.cc',
'rows_to_batches_test.cc',
'small_vector_test.cc',
'span_test.cc',
'stl_util_test.cc',
'string_test.cc',
'tdigest_test.cc',
'test_common.cc',
'time_test.cc',
'tracing_test.cc',
'trie_test.cc',
'uri_test.cc',
'utf8_util_test.cc',
'value_parsing_test.cc',
]

exc = executable(
'arrow-utility-test',
sources: utility_test_srcs + io_util_test_sources,
dependencies: [arrow_dep, filesystem_dep, gtest_dep, gmock_dep],
link_with: [arrow_test_lib],
implicit_include_directories: false,
)
test('arrow-utility-test', exc)

util_tests = {
'arrow-async-utility-test': {
'sources': [
'async_generator_test.cc',
'async_util_test.cc',
'test_common.cc',
],
},
'arrow-bit-utility-test': {
'sources': [
'bit_block_counter_test.cc',
'bit_util_test.cc',
'rle_encoding_test.cc',
],
},
'arrow-threading-utility-test': {
'sources': [
'cancel_test.cc',
'counting_semaphore_test.cc',
'future_test.cc',
'task_group_test.cc',
'test_common.cc',
'thread_pool_test.cc',
],
},
'arrow-crc32-test': {
'sources': ['crc32_test.cc'],
'dependencies': [filesystem_dep],
},
}

if needs_tests
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 this?

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Contributor Author

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!

foreach key, val : util_tests
exc = executable(
key,
sources: val['sources'],
dependencies: [arrow_test_dep, val.get('dependencies', [])],
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 gtest_main_dep here?

Copy link
Contributor Author

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

Copy link
Member

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?

Copy link
Contributor Author

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

implicit_include_directories: false,
)
test(key, exc)
endforeach
endif

util_benchmarks = [
'bit_block_counter',
'bit_util',
'bitmap_reader',
'cache',
'compression',
'decimal',
'hashing',
'int_util',
'machine',
'queue',
'range',
'small_vector',
'tdigest',
'thread_pool',
'trie',
]

foreach util_benchmark : util_benchmarks
benchmark_name = '@0@-benchmark'.format(util_benchmark.replace('_', '-'))
exc = executable(
benchmark_name,
sources: '@0@_benchmark.cc'.format(util_benchmark),
dependencies: [arrow_test_dep, benchmark_dep],
implicit_include_directories: false,
)
benchmark(benchmark_name, exc)
endforeach

# TODO: XSimd benchmark. See GH-45823
Loading