wgtmac commented on code in PR #233:
URL: https://github.com/apache/iceberg-cpp/pull/233#discussion_r2386877456
##########
.github/workflows/test.yml:
##########
@@ -96,3 +96,36 @@ jobs:
run: |
call "C:\Program Files\Microsoft Visual
Studio\2022\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x64
bash -c "ci/scripts/build_example.sh $(pwd)/example"
+ meson:
+ name: Meson - ${{ matrix.title }}
+ runs-on: ${{ matrix.runs-on }}
+ strategy:
+ fail-fast: false
+ matrix:
+ include:
+ - title: AMD64 Ubuntu 24.04
+ runs-on: ubuntu-24.04
+ - title: AMD64 Windows 2022
+ runs-on: windows-2022
Review Comment:
Should we consider windows-2025?
##########
src/iceberg/catalog/meson.build:
##########
@@ -0,0 +1,18 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+install_headers(['in_memory_catalog.h'], subdir: 'iceberg/catalog')
Review Comment:
Is there any syntax like `glob` so we don't need to bother with manual
addition?
##########
src/iceberg/CMakeLists.txt:
##########
@@ -94,17 +94,24 @@ add_iceberg_lib(iceberg
STATIC_INSTALL_INTERFACE_LIBS
${ICEBERG_STATIC_INSTALL_INTERFACE_LIBS}
SHARED_INSTALL_INTERFACE_LIBS
- ${ICEBERG_SHARED_INSTALL_INTERFACE_LIBS})
+ ${ICEBERG_SHARED_INSTALL_INTERFACE_LIBS}
+ OUTPUTS
+ ICEBERG_LIBRARIES)
+
+foreach(LIB_TARGET ${ICEBERG_LIBRARIES})
Review Comment:
Can we embed this into `add_iceberg_lib` in the IcebergBuildUtils.cmake?
##########
src/iceberg/meson.build:
##########
@@ -0,0 +1,139 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+iceberg_include_dir = include_directories('..')
+iceberg_sources = files(
+ 'arrow_c_data_guard_internal.cc',
+ 'catalog/in_memory_catalog.cc',
+ 'expression/expression.cc',
+ 'expression/literal.cc',
+ 'file_reader.cc',
+ 'file_writer.cc',
+ 'inheritable_metadata.cc',
+ 'json_internal.cc',
+ 'manifest_entry.cc',
+ 'manifest_list.cc',
+ 'manifest_reader.cc',
+ 'manifest_reader_internal.cc',
+ 'manifest_writer.cc',
+ 'metadata_columns.cc',
+ 'name_mapping.cc',
+ 'partition_field.cc',
+ 'partition_spec.cc',
+ 'schema.cc',
+ 'schema_field.cc',
+ 'schema_internal.cc',
+ 'schema_util.cc',
+ 'snapshot.cc',
+ 'sort_field.cc',
+ 'sort_order.cc',
+ 'statistics_file.cc',
+ 'table.cc',
+ 'table_metadata.cc',
+ 'table_scan.cc',
+ 'transform.cc',
+ 'transform_function.cc',
+ 'type.cc',
+ 'util/decimal.cc',
+ 'util/gzip_internal.cc',
+ 'util/murmurhash3_internal.cc',
+ 'util/timepoint.cc',
+)
+
+nanoarrow_dep = dependency('nanoarrow')
+nlohmann_json_dep = dependency('nlohmann_json')
+spdlog_dep = dependency('spdlog')
+zlib_dep = dependency('zlib')
+
+iceberg_deps = [nanoarrow_dep, nlohmann_json_dep, spdlog_dep, zlib_dep]
+
+iceberg_lib = library(
Review Comment:
Any plan to add `iceberg_bundle_lib`?
##########
src/iceberg/meson.build:
##########
@@ -0,0 +1,139 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+iceberg_include_dir = include_directories('..')
+iceberg_sources = files(
+ 'arrow_c_data_guard_internal.cc',
+ 'catalog/in_memory_catalog.cc',
+ 'expression/expression.cc',
+ 'expression/literal.cc',
+ 'file_reader.cc',
+ 'file_writer.cc',
+ 'inheritable_metadata.cc',
+ 'json_internal.cc',
+ 'manifest_entry.cc',
+ 'manifest_list.cc',
+ 'manifest_reader.cc',
+ 'manifest_reader_internal.cc',
+ 'manifest_writer.cc',
+ 'metadata_columns.cc',
+ 'name_mapping.cc',
+ 'partition_field.cc',
+ 'partition_spec.cc',
+ 'schema.cc',
+ 'schema_field.cc',
+ 'schema_internal.cc',
+ 'schema_util.cc',
+ 'snapshot.cc',
+ 'sort_field.cc',
+ 'sort_order.cc',
+ 'statistics_file.cc',
+ 'table.cc',
+ 'table_metadata.cc',
+ 'table_scan.cc',
+ 'transform.cc',
+ 'transform_function.cc',
+ 'type.cc',
+ 'util/decimal.cc',
+ 'util/gzip_internal.cc',
+ 'util/murmurhash3_internal.cc',
+ 'util/timepoint.cc',
+)
+
+nanoarrow_dep = dependency('nanoarrow')
+nlohmann_json_dep = dependency('nlohmann_json')
+spdlog_dep = dependency('spdlog')
+zlib_dep = dependency('zlib')
+
+iceberg_deps = [nanoarrow_dep, nlohmann_json_dep, spdlog_dep, zlib_dep]
+
+iceberg_lib = library(
+ 'iceberg',
+ sources: iceberg_sources,
+ dependencies: iceberg_deps,
+ include_directories: iceberg_include_dir,
+ install: true,
+ gnu_symbol_visibility: 'inlineshidden',
+ cpp_shared_args: ['-DICEBERG_EXPORTING'],
+ cpp_static_args: ['-DICEBERG_STATIC'],
+)
+
+iceberg_interface_args = []
+if get_option('default_library') != 'shared'
+ iceberg_interface_args += ['-DICEBERG_STATIC']
+endif
+
+iceberg_dep = declare_dependency(
+ link_with: iceberg_lib,
+ dependencies: iceberg_deps,
+ include_directories: iceberg_include_dir,
+ compile_args: iceberg_interface_args,
+)
+meson.override_dependency('iceberg', iceberg_dep)
+pkg = import('pkgconfig')
+pkg.generate(iceberg_lib)
Review Comment:
I think you mean `IcebergConfig.cmake` and `IcebergTarget.cmake`, right? We
can switch to `iceberg-config.cmake` and `iceberg-target.cmake` if this makes
sense.
##########
meson.options:
##########
@@ -0,0 +1,18 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+option('tests', type: 'feature', description: 'Build tests', value: 'enabled')
Review Comment:
Do we need to add other CMake options?
##########
src/iceberg/util/meson.build:
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+install_headers(
Review Comment:
Do we have auto formatter for Meson build files?
##########
.pre-commit-config.yaml:
##########
@@ -39,3 +39,15 @@ repos:
rev: v0.6.10
hooks:
- id: cmake-format
+
+ - repo: https://github.com/trim21/pre-commit-mirror-meson
+ rev: v1.9.0
+ hooks:
+ - id: meson-fmt
Review Comment:
Ah, this answers my previous question on the formatter.
##########
src/iceberg/json_internal.h:
##########
@@ -38,7 +38,7 @@ namespace iceberg {
///
/// \param sort_field The `SortField` object to be serialized.
/// \return A JSON object representing the `SortField` in the form of
key-value pairs.
-nlohmann::json ToJson(const SortField& sort_field);
+ICEBERG_EXPORT nlohmann::json ToJson(const SortField& sort_field);
Review Comment:
Why do we need to add export to a non-installed header file?
##########
test/meson.build:
##########
@@ -0,0 +1,74 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+gmock_main_dep = dependency('gmock_main')
+
+conf_data = configuration_data()
+conf_data.set('ICEBERG_TEST_RESOURCES', meson.current_source_dir() /
'resources')
+configure_file(
+ input: 'test_config.h.in',
+ output: 'test_config.h',
+ configuration: conf_data,
+ install: true,
+ install_dir: get_option('includedir') / 'iceberg/test',
+)
+
+iceberg_tests = {
+ 'schema_test': files(
+ 'name_mapping_test.cc',
+ 'partition_field_test.cc',
+ 'partition_spec_test.cc',
+ 'schema_field_test.cc',
+ 'schema_test.cc',
+ 'schema_util_test.cc',
+ 'snapshot_test.cc',
+ 'sort_field_test.cc',
+ 'sort_order_test.cc',
+ 'transform_test.cc',
+ 'type_test.cc',
+ ),
+ 'table_test': files(
+ 'json_internal_test.cc',
+ 'schema_json_test.cc',
+ 'table_test.cc',
+ 'test_common.cc',
+ ),
+ 'expression_test': files('expression_test.cc', 'literal_test.cc'),
+ 'json_serde_test': files(
+ 'json_internal_test.cc',
+ 'metadata_serde_test.cc',
+ 'schema_json_test.cc',
+ 'test_common.cc',
+ ),
+ 'util_test': files(
+ 'config_test.cc',
+ 'decimal_test.cc',
+ 'endian_test.cc',
+ 'formatter_test.cc',
+ 'string_util_test.cc',
+ 'visit_type_test.cc',
+ ),
+}
+
+foreach test_name, sources : iceberg_tests
Review Comment:
I'm just a little bit worried that it may add more maintenance overhead to
contributors who don't know Meson before. In addition, people may forget to
update this file when adding a new test case to the CMakeLists.txt. Any best
practice for this?
##########
src/iceberg/CMakeLists.txt:
##########
@@ -94,17 +94,24 @@ add_iceberg_lib(iceberg
STATIC_INSTALL_INTERFACE_LIBS
${ICEBERG_STATIC_INSTALL_INTERFACE_LIBS}
SHARED_INSTALL_INTERFACE_LIBS
- ${ICEBERG_SHARED_INSTALL_INTERFACE_LIBS})
+ ${ICEBERG_SHARED_INSTALL_INTERFACE_LIBS}
+ OUTPUTS
+ ICEBERG_LIBRARIES)
+
+foreach(LIB_TARGET ${ICEBERG_LIBRARIES})
+ target_compile_definitions(${LIB_TARGET} PRIVATE ICEBERG_EXPORTING)
+endforeach()
+
+if(ICEBERG_BUILD_STATIC)
Review Comment:
Same for this
##########
subprojects/zlib.wrap:
##########
@@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+[wrap-file]
+directory = zlib-1.3.1
+source_url = http://zlib.net/fossils/zlib-1.3.1.tar.gz
Review Comment:
Does Meson support using system installed dependencies instead of building
everything?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
