paleolimbot commented on code in PR #644: URL: https://github.com/apache/arrow-nanoarrow/pull/644#discussion_r1803277715
########## python/src/nanoarrow/meson.build: ########## @@ -0,0 +1,99 @@ +# 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. + +# Try to resolve the symlink to a subproject first and fall back +# to the wrap entry if that is unsuccessful. Ideally Meson would +# take care of this for us, but there is a bug upstream +# https://github.com/mesonbuild/meson/issues/13746#issuecomment-2392510954 +nanoarrow_proj = subproject('arrow-nanoarrow') +nanoarrow_dep = nanoarrow_proj.get_variable('nanoarrow_dep') +nanoarrow_ipc_dep = nanoarrow_proj.get_variable('nanoarrow_ipc_dep') +nanoarrow_device_dep = nanoarrow_proj.get_variable('nanoarrow_device_dep') + +py = import('python').find_installation(pure: false) + +generated_pyx = custom_target( + 'generate-pyx', + output: 'nanoarrow_c.pxd', + command: [py, meson.current_source_dir() + '/../../bootstrap.py'], +) +nanoarrow_c_dep = declare_dependency(sources: generated_pyx) + +cyfiles = [ + '_array.pyx', + '_array_stream.pyx', + '_buffer.pyx', + '_device.pyx', + '_ipc_lib.pyx', + '_schema.pyx', + '_types.pyx', + '_utils.pyx', +] + +cython_args = [ + '--include-dir', + meson.current_source_dir(), + '--include-dir', + meson.project_source_root() / 'vendor', # for generated nanoarrow_c file +] +if get_option('buildtype') == 'debug' + cython_args += ['--gdb'] +endif + +fs = import('fs') +foreach cyf : cyfiles + cyfile_deps = [nanoarrow_c_dep, nanoarrow_dep] + + stem = fs.stem(cyf) + if stem in ['_array', '_device'] + cyfile_deps += [nanoarrow_device_dep] + elif stem == '_ipc_lib' + cyfile_deps += [nanoarrow_ipc_dep] + endif + + py.extension_module( + stem, + sources: [cyf], + cython_args: cython_args, + dependencies: cyfile_deps, + subdir: 'nanoarrow', + install: true, + ) +endforeach + +py_sources = [ + '__init__.py', + 'array.py', + 'array_stream.py', + 'c_array.py', + 'c_array_stream.py', + 'c_buffer.py', + 'c_schema.py', + 'device.py', + 'ipc.py', + 'iterator.py', + '_repr_utils.py', + 'schema.py', + 'visitor.py', +] Review Comment: Is there any way to make this a glob like `"*.py"` so that a new contributor doesn't have to remember to update this list? ########## python/pyproject.toml: ########## @@ -38,7 +38,11 @@ Changelog = "https://github.com/apache/arrow-nanoarrow/blob/main/CHANGELOG.md"; [build-system] requires = [ - "setuptools >= 61.0.0", + "meson-python", Review Comment: We probably need a version constraint here? ########## python/src/nanoarrow/meson.build: ########## @@ -0,0 +1,99 @@ +# 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. + +# Try to resolve the symlink to a subproject first and fall back +# to the wrap entry if that is unsuccessful. Ideally Meson would +# take care of this for us, but there is a bug upstream +# https://github.com/mesonbuild/meson/issues/13746#issuecomment-2392510954 +nanoarrow_proj = subproject('arrow-nanoarrow') +nanoarrow_dep = nanoarrow_proj.get_variable('nanoarrow_dep') +nanoarrow_ipc_dep = nanoarrow_proj.get_variable('nanoarrow_ipc_dep') +nanoarrow_device_dep = nanoarrow_proj.get_variable('nanoarrow_device_dep') Review Comment: Again, I think you want to vendor the files using `bootstrap.py` or whatever facility meson provides for that and not attempt to get the nanoarrow-as-a-subproject bit working (orthogonal battle to using meson-python vs. setuptools). ########## python/src/nanoarrow/__init__.py: ########## @@ -75,7 +77,8 @@ from nanoarrow.array import array, Array from nanoarrow.array_stream import ArrayStream from nanoarrow.visitor import nulls_as_sentinel, nulls_forbid, nulls_separate -from nanoarrow._version import __version__ # noqa: F401 + +__version__ = importlib.metadata.version("nanoarrow") Review Comment: @jorisvandenbossche is there any issues with versioning in this way? Is there any cost to importing `importlib.metadata` by default? (I am only skeptical because I haven't seen a Python package do this before) ########## python/generate_dist.py: ########## @@ -0,0 +1,53 @@ +# 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. + +import os +import pathlib +import shutil + + +def main(): + src_dir = pathlib.Path(os.environ["MESON_SOURCE_ROOT"]).parent.resolve() + dist_dir = pathlib.Path(os.environ["MESON_DIST_ROOT"]).resolve() + subproj_dir = dist_dir / "subprojects" / "arrow-nanoarrow" + + if subproj_dir.is_symlink(): + subproj_dir.unlink() + + subproj_dir.mkdir() + shutil.copy(src_dir / "meson.build", subproj_dir / "meson.build") + shutil.copy(src_dir / "meson.options", subproj_dir / "meson.options") + + subproj_subproj_dir = subproj_dir / "subprojects" + subproj_subproj_dir.mkdir() + for f in (src_dir / "subprojects").glob("*.wrap"): + shutil.copy(f, subproj_subproj_dir / f.name) + + target_src_dir = subproj_dir / "src" + shutil.copytree(src_dir / "src", target_src_dir) + + # this files are only needed by bootstrap.py Review Comment: For this PR I think you will have the most success just renaming `bootstrap.py` to `generate_dist.py` and including a meson lib definition for nanoarrow in `python/meson.build` (i.e., don't try to build nanoarrow C using a subproject). ########## meson.build: ########## @@ -103,7 +105,7 @@ if get_option('ipc') ) nanoarrow_ipc_dep = declare_dependency(include_directories: [incdir], link_with: nanoarrow_ipc_lib, - dependencies: [nanoarrow_dep, flatcc_dep]) + dependencies: [nanoarrow_dep]) Review Comment: I'm unclear on the difference but yes, the flatcc headers are deliberately not included in nanoarrow's public headers such that a library using nanoarrow does not need `flatcc/*` files in the include path. -- 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]
