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.

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

python: Use meson-python instead of setuptools #644

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions .github/workflows/python-wheels.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,16 @@ jobs:
fetch-tags: true

- uses: actions/setup-python@v5
if: ${{ matrix.config.label != "windows" }}
with:
python-version: "3.12"

- uses: actions/setup-python@v5
if: ${{ matrix.config.label == "windows" }}
with:
python-version: "3.12"
architecture: x64
Copy link
Contributor Author

@WillAyd WillAyd Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like setup-python still installs a 32 bit Python by default on the windows runner, while cibuildwheel is expected 64 bit on that host

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 still get 32-bit wheels?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so...I think cibuildwheel controls that, and since we don't override anything in the pyproject.toml I think it should still generate the same types of wheels as specified here:

https://cibuildwheel.pypa.io/en/stable/options/#build-skip

Admittedly my knowledge of Windows and how it builds for different architectures is limited, so definitely something to verify when wheels actually get build

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might have to update the python-wheels.yaml workflow to fire on python/meson.build instead of python/setup.py to get it to run on this PR (in which case we can inspect the results and find out!)


- name: Install cibuildwheel
run: python -m pip install cibuildwheel==2.21.1

Expand Down
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,8 @@ __pycache__
subprojects/*
!subprojects/packagefiles
!subprojects/*.wrap
python/subprojects/*
!python/subprojects/packagefiles
!python/subprojects/*.wrap

compile_commands.json
1 change: 1 addition & 0 deletions dev/release/rat_exclude_files.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ dist/flatcc.c
src/nanoarrow/ipc/flatcc_generated.h
thirdparty/*
python/src/nanoarrow/dlpack_abi.h
python/subprojects/arrow-nanoarrow
2 changes: 1 addition & 1 deletion dev/release/release_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def set_python_dev_version_command(args):
_, last_dev_tag = find_last_dev_tag()
dev_distance = len(find_commits_since(last_dev_tag))

version_file = src_path("python", "src", "nanoarrow", "_static_version.py")
version_file = src_path("python", "meson.build")
file_regex_replace(
r'"([0-9]+\.[0-9]+\.[0-9]+)\.dev[0-9]+"',
f'"\\1.dev{dev_distance}"',
Expand Down
2 changes: 1 addition & 1 deletion dev/release/source_build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ main() {
# Resolve all hard and symbolic links
rm -rf "${base_name}.tmp/"
mv "${base_name}/" "${base_name}.tmp/"
cp -R -L "${base_name}.tmp" "${base_name}"
cp -R -d "${base_name}.tmp" "${base_name}"
rm -rf "${base_name}.tmp/"

# Create new tarball
Expand Down
8 changes: 4 additions & 4 deletions dev/release/utils-prepare.sh
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,10 @@ update_versions() {
git add DESCRIPTION
popd

pushd "${NANOARROW_DIR}/python/src/nanoarrow"
sed -i.bak -E "s/version = \".+\"/version = \"${python_version}\"/" _static_version.py
rm _static_version.py.bak
git add _static_version.py
pushd "${NANOARROW_DIR}/python/"
sed -i.bak -E "s/version = '.+'/version = '${python_version}'/" meson.build
rm meson.build.bak
git add meson.build
popd
}

Expand Down
6 changes: 5 additions & 1 deletion meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,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])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was a mistake to include the flatcc_dep transitively here; I think its only required to build the nanoarrow_ipc lib itself, but shouldn't be required by libraries that want to use nanoarrow_ipc (?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

endif

needs_device = get_option('device') or get_option('metal') or get_option('cuda')
Expand Down Expand Up @@ -137,6 +137,10 @@ if needs_device
install: true,
cpp_args: device_defines,
)

nanoarrow_device_dep = declare_dependency(include_directories: [incdir],
link_with: nanoarrow_device_lib,
dependencies: device_deps)
endif

needs_testing = get_option('testing') or get_option('tests')
Expand Down
1 change: 1 addition & 0 deletions python/MANIFEST.in
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,6 @@ recursive-include src/nanoarrow *.pxd
recursive-include src/nanoarrow *.h

exclude bootstrap.py
exclude generate_dist.py
exclude src/nanoarrow/*.c
recursive-exclude src/nanoarrow *.o *.so
18 changes: 6 additions & 12 deletions python/bootstrap.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ def generate_pxd(self, file_in, file_out):
output.write(header.encode("UTF-8"))

output.write(
f'\ncdef extern from "{file_in_name}" nogil:\n'.encode("UTF-8")
f'\ncdef extern from "nanoarrow/{file_in_name}" nogil:\n'.encode(
"UTF-8"
)
)

# A few things we add in manually
Expand Down Expand Up @@ -228,7 +230,9 @@ def _write_defs(self, output):
# strictly necessary for things like installing from GitHub.
def copy_or_generate_nanoarrow_c():
this_dir = pathlib.Path(__file__).parent.resolve()
source_dir = this_dir.parent
subproj_dir = this_dir / "subprojects"
source_dir = subproj_dir / "arrow-nanoarrow"

vendor_dir = this_dir / "vendor"

vendored_files = [
Expand All @@ -244,16 +248,6 @@ def copy_or_generate_nanoarrow_c():
for f in dst.values():
f.unlink(missing_ok=True)

is_cmake_dir = (source_dir / "CMakeLists.txt").exists()
is_in_nanoarrow_repo = (
is_cmake_dir and (source_dir / "src" / "nanoarrow" / "nanoarrow.h").exists()
)

if not is_in_nanoarrow_repo:
raise ValueError(
"Attempt to build source distribution outside the nanoarrow repo"
)

vendor_dir.mkdir(exist_ok=True)
subprocess.run(
[
Expand Down
53 changes: 53 additions & 0 deletions python/generate_dist.py
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor Author

@WillAyd WillAyd Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The amount of scripting here is unfortunate...would still be nice to replace bootstrap.py and/or bundle.py, but not sure I want to tackle that in this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "including a meson lib definition?"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running bootstrap.py will get you python/vendor/nanoarrow.c and friends, and you should be able to put the appropriate nanoarrow_c_lib = library(...) in python/meson.build?

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 OK - so you want to run bootstrap.py continuously, not just when the distribution is being created?

The current form uses the subproject symlink for live integration with the C project and only vendors when the distribution is created

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can get all the tests passing and all the wheels building with that go for it! I think you will have more success tackling those two battles separately but the time is yours 🙂

shutil.copy(src_dir / "CMakeLists.txt", subproj_dir / "CMakeLists.txt")
subproj_ci_scripts_dir = subproj_dir / "ci" / "scripts"
subproj_ci_scripts_dir.mkdir(parents=True)
shutil.copy(
src_dir / "ci" / "scripts" / "bundle.py", subproj_ci_scripts_dir / "bundle.py"
)


if __name__ == "__main__":
main()
26 changes: 19 additions & 7 deletions python/src/nanoarrow/_static_version.py → python/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,24 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
#
# This file is part of 'miniver': https://github.com/jbweston/miniver

# Replaced by version-bumping scripts at release time
version = "0.7.0.dev0"
project(
'nanoarrow',
'cpp', 'cython',
version: '0.7.0-SNAPSHOT',
license: 'Apache-2.0',
meson_version: '>=1.2.0',
default_options: [
'warning_level=2',
'cpp_std=c++17',
'default_library=static',
# We need to set these options at the project default_option level
# due to https://github.com/mesonbuild/meson/issues/6728
'arrow-nanoarrow:ipc=true',
'arrow-nanoarrow:device=true',
],
)

subdir('src/nanoarrow')

# These values are only set if the distribution was created with 'git archive'
refnames = "$Format:%D$"
git_hash = "$Format:%h$"
meson.add_dist_script('python', meson.current_source_dir() / 'generate_dist.py')
8 changes: 6 additions & 2 deletions python/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ Changelog = "https://github.com/apache/arrow-nanoarrow/blob/main/CHANGELOG.md"

[build-system]
requires = [
"setuptools >= 61.0.0",
"meson-python",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need a version constraint here?

"Cython"
]
build-backend = "setuptools.build_meta"
build-backend = "mesonpy"

[tool.meson-python.args]
install = ['--skip-subprojects']
dist = ['--include-subprojects']
149 changes: 0 additions & 149 deletions python/setup.py

This file was deleted.

5 changes: 4 additions & 1 deletion python/src/nanoarrow/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
Arrow C Data and Arrow C Stream interfaces.
"""

import importlib.metadata

from nanoarrow._utils import c_version
from nanoarrow.c_array import c_array_from_buffers, c_array
from nanoarrow.c_array_stream import c_array_stream
Expand Down Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I accidentally removed the comment, but the downside to this approach versus a dynamic version generator like miniver is that the git hash is not included as part of the project metadata.

In this case the project definition from pyproject.toml just imports whatever the build-backend provides, which is currently 0.7.0-SNAPSHOT


# Helps Sphinx automatically populate an API reference section
__all__ = [
Expand Down
Loading
Loading