From cee8a5d33db5dd7cf40f9a07cc6e2764c818db6c Mon Sep 17 00:00:00 2001 From: Yifan Chen Date: Wed, 9 Sep 2026 23:37:54 -0700 Subject: [PATCH] fix: allow Rust consumers to link datafusion-python Gate PyO3 extension-module behind a default-on crate feature, retain it in wheel builds, and run a separate executable linking regression in CI. Generated-by: OpenAI Codex --- .github/actions/build-wheel/action.yml | 4 ++- .github/workflows/build.yml | 27 +++++++++++++++ AGENTS.md | 19 +++++------ crates/core/Cargo.toml | 9 +++-- crates/core/build.rs | 1 + crates/core/tests/rust_link.rs | 33 +++++++++++++++++++ docs/source/contributor-guide/introduction.md | 19 +++++++++++ 7 files changed, 95 insertions(+), 17 deletions(-) create mode 100644 crates/core/tests/rust_link.rs diff --git a/.github/actions/build-wheel/action.yml b/.github/actions/build-wheel/action.yml index 25f75d1f8..84ca94d8d 100644 --- a/.github/actions/build-wheel/action.yml +++ b/.github/actions/build-wheel/action.yml @@ -60,13 +60,15 @@ runs: run: | set -euo pipefail FEATURES="${{ inputs.features }}" + # All wheels need extension-module, including builds that disable defaults. + FEATURES="extension-module${FEATURES:+,${FEATURES}}" TAG="${{ inputs.python-tag }}" if [ "$TAG" = "abi3" ]; then # Default features include the `abi3` cargo feature. # One wheel covers Python 3.10..3.14 (GIL builds only). BUILD_ARGS="--features ${FEATURES}" else - # Free-threaded build: disable abi3, force mimalloc back in, pin interpreter. + # Disable abi3, restore wheel features, and pin the free-threaded interpreter. if [ "${RUNNER_OS:-}" = "Windows" ]; then # Windows free-threaded builds ship as `python.exe` (no `tN` # suffix). Resolve sys.executable so the path is independent of diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index d7af9b663..d867b8098 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -59,6 +59,33 @@ jobs: - name: Check formatting run: cargo +nightly fmt --all -- --check + test-rust: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + + - name: Setup Rust + uses: dtolnay/rust-toolchain@4cda84d5c5c54efe2404f9d843567869ab1699d4 + + - name: Setup Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Install Protoc + uses: arduino/setup-protoc@v3 + with: + version: "27.4" + repo-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Cache Cargo + uses: Swatinem/rust-cache@v2 + + # Select core explicitly: the FFI example workspace members enable + # pyo3/extension-module, which prevents executables from linking libpython. + - name: Run Rust tests, including the downstream linking smoke test + run: cargo test --locked -p datafusion-python --no-default-features --features substrait + lint-python: runs-on: ubuntu-latest steps: diff --git a/AGENTS.md b/AGENTS.md index 659094ec0..69b804292 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -88,19 +88,16 @@ Always prefer Python coverage — a doctest example in a docstring, or a pytest case. The user-facing Python surface is the first line of defense and the primary focus, so behavior should be pinned where users actually meet it. -**CI does not run Rust tests.** No workflow invokes `cargo test`; the only -Rust checks are `cargo fmt --check` and -`cargo clippy --no-deps --all-targets`. `--all-targets` compiles -`#[cfg(test)]` code, so a Rust test cannot rot into a non-compiling state, but -it is never executed and a behavioral regression will not fail the build. A -Rust test added today is dead weight. - -Adding a `cargo test` job is not a one-line change: `crates/core/Cargo.toml` -enables `pyo3/extension-module` unconditionally, so the test binary fails to -link against `Py_*` symbols on Linux. The feature would have to be gated first. +**CI runs core Rust tests** with +`cargo test --locked -p datafusion-python --no-default-features --features substrait`. +Disabling the default `extension-module` feature allows test executables to +link libpython. Select core explicitly rather than `--workspace`: the FFI +example crates enable `pyo3/extension-module` through Cargo feature unification. +The `rust_link` integration test consumes the rlib in a separate executable +and executes Python bindings, guarding against unresolved `Py_*` symbols. Write a Rust test only when the behavior is genuinely unreachable from Python, -and wire up CI in the same change so it actually runs. Before concluding it is +and ensure CI actually runs it. Before concluding it is unreachable, check the suites that already exist: - `python/tests/` — the main suite. Run `pytest python/`, **not** diff --git a/crates/core/Cargo.toml b/crates/core/Cargo.toml index c5f1e0167..d53a3fe89 100644 --- a/crates/core/Cargo.toml +++ b/crates/core/Cargo.toml @@ -40,10 +40,7 @@ tokio = { workspace = true, features = [ "rt-multi-thread", "sync", ] } -pyo3 = { workspace = true, features = [ - "extension-module", - "generate-import-lib", -] } +pyo3 = { workspace = true, features = ["generate-import-lib"] } pyo3-async-runtimes = { workspace = true, features = ["tokio-runtime"] } pyo3-log = { workspace = true } chrono = { workspace = true } @@ -76,7 +73,9 @@ prost-types = { workspace = true } pyo3-build-config = { workspace = true } [features] -default = ["mimalloc", "abi3"] +default = ["mimalloc", "abi3", "extension-module"] +# Disable for Rust tests and executables, which need to link libpython. +extension-module = ["pyo3/extension-module"] # Stable ABI build — single wheel covers Python 3.10..3.14 (GIL builds only). # Mutually exclusive with free-threaded interpreters (cp313t / cp314t); the # free-threaded wheel build must pass --no-default-features. diff --git a/crates/core/build.rs b/crates/core/build.rs index 4878d8b0e..1b105ef04 100644 --- a/crates/core/build.rs +++ b/crates/core/build.rs @@ -16,5 +16,6 @@ // under the License. fn main() { + #[cfg(feature = "extension-module")] pyo3_build_config::add_extension_module_link_args(); } diff --git a/crates/core/tests/rust_link.rs b/crates/core/tests/rust_link.rs new file mode 100644 index 000000000..4c0ee5800 --- /dev/null +++ b/crates/core/tests/rust_link.rs @@ -0,0 +1,33 @@ +// 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. + +use datafusion_python::context::PySessionContext; +use pyo3::prelude::*; + +// An integration test is a separate executable consuming the rlib. Exercise +// Python calls as well as Rust construction so linking must resolve Py_* symbols. +#[test] +fn rust_consumer_can_execute_python_bindings() -> PyResult<()> { + Python::initialize(); + Python::attach(|py| { + let context = Bound::new(py, PySessionContext::new(None, None)?)?; + let dataframe = context.call_method1("sql_with_options", ("SELECT 1 AS value",))?; + let count = dataframe.call_method0("count")?.extract::()?; + assert_eq!(count, 1); + Ok(()) + }) +} diff --git a/docs/source/contributor-guide/introduction.md b/docs/source/contributor-guide/introduction.md index 24d56ed90..773197297 100644 --- a/docs/source/contributor-guide/introduction.md +++ b/docs/source/contributor-guide/introduction.md @@ -70,6 +70,25 @@ maturin develop --uv python -m pytest ``` +### Rust tests and downstream Rust dependencies + +Run the core Rust tests, including the executable linking smoke test, with: + +```shell +cargo test --locked -p datafusion-python --no-default-features --features substrait +``` + +An installed Python interpreter and its development libraries are required. +Set `PYO3_PYTHON` to select an interpreter if needed. Do not use `--workspace` +for these tests: the FFI example crates enable `pyo3/extension-module`, which +prevents linking libpython into an executable. + +Rust executables depending on `datafusion-python` should set +`default-features = false` and enable optional features such as `substrait` +as needed. The default `extension-module` feature is for Python extension +builds. Free-threaded wheel builds disable defaults to avoid `abi3` and must +explicitly enable `extension-module` (and `mimalloc` to retain the default allocator). + ## Running & Installing pre-commit hooks arrow-datafusion-python takes advantage of [pre-commit](https://pre-commit.com/) to assist developers with code linting to help reduce the number of commits that ultimately fail in CI due to linter errors. Using the pre-commit hooks is optional for the developer but certainly helpful for keeping PRs clean and concise.