Skip to content

chore(ci): Move pyo3 extension module feature to Python args#50

Merged
jiayuasu merged 2 commits intoapache:mainfrom
paleolimbot:pyo3-extension-module
Sep 11, 2025
Merged

chore(ci): Move pyo3 extension module feature to Python args#50
jiayuasu merged 2 commits intoapache:mainfrom
paleolimbot:pyo3-extension-module

Conversation

@paleolimbot
Copy link
Member

The pyo3 feature extension-module enables the actual Python extension, which causes problems for some cargo commands of the parent repo (e.g., cargo build --all). I had added this so that we could add the s2geography feature optionally for MacOS and Linux builds and they failed without it; however, that can be specified elsewhere.

datafusion-ffi = { workspace = true }
futures = { workspace = true }
pyo3 = { version = "0.25.1", features = ["extension-module"] }
pyo3 = { version = "0.25.1" }
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! That was definitely the issue (and I think that the pyprojects.toml value was overridden when I passed --features in the wheel build 😬 )

@paleolimbot paleolimbot marked this pull request as ready for review September 11, 2025 02:42
@Kontinuation
Copy link
Member

Can we add a plain cargo build to our GitHub Workflow to catch this issue?

diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml
index 1c57218..1c6bd19 100644
--- a/.github/workflows/rust.yml
+++ b/.github/workflows/rust.yml
@@ -49,7 +49,7 @@ jobs:
   rust:
     strategy:
       matrix:
-        name: ["clippy", "docs", "test"]
+        name: ["clippy", "build", "docs", "test"]
 
     name: "${{ matrix.name }}"
     runs-on: ubuntu-latest
@@ -117,6 +117,11 @@ jobs:
         run: |
           cargo clippy --workspace --all-targets --all-features -- -Dwarnings
 
+      - name: Build
+        if: matrix.name == 'build'
+        run: |
+          cargo build --workspace --all-targets --all-features
+
       - name: Test
         if: matrix.name == 'test'
         run: |

@paleolimbot
Copy link
Member Author

Sure, in parallel maybe? I don't use cargo build for anything...is it useful? (cargo clippy and cargo test have always worked for me).

@Kontinuation
Copy link
Member

Sure, in parallel maybe? I don't use cargo build for anything...is it useful? (cargo clippy and cargo test have always worked for me).

I use cargo build to build everything including sedona-cli. I think keeping the project building cleanly using the most commonly used command for building most Rust project is a good idea. Unless using cargo build is generally a bad practice for building any Rust projects.

@jiayuasu jiayuasu merged commit 14e6fb8 into apache:main Sep 11, 2025
15 checks passed
@paleolimbot paleolimbot deleted the pyo3-extension-module branch October 8, 2025 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants