Py bindings ci by arminsabouri · Pull Request #60 · LtbLightning/payjoin-ffi

arminsabouri · 2025-03-22T01:48:25Z

closes #3
closes #57

These files should have never been commited to source. By removing these shared libraries we encourage developers to build from source instead of using this repo as a quasi package.

It's not compatible with venv.

The version should, at least for now, without python-specific breaks, match payjoin-ffi/Cargo.toml

DanGould · 2025-03-24T17:41:25Z

We should follow bdk-ffi's test-python GitHub workflow as an example

DanGould · 2025-03-24T22:53:15Z

I see BDK's generate scripts, at least the linux one for python do build the lib before running uniffi-bindgen. That's probably why CI is still failing.

arminsabouri · 2025-03-25T17:05:22Z

Finally done wrestling CI. Major changes include:

Removing support for window bindings
Skipping broken Python tests (all execept one) and the int. tests. Running the tests are still useful b/c they show there are no build time errors.
Missing features flags when building FAT Macos binary

cc @DanGould @spacebear21

DanGould · 2025-03-25T17:14:04Z

The one thing I would hope for here is that the CI changes are squashed together so that intermediate commits won't fail the CI checks they implement

arminsabouri · 2025-03-25T17:19:36Z

CI changes are squashed together so that intermediate commits won't fail the CI checks they implement

Do you mean the changes needed to make CI pass?
There are a couple duplicated change across linux and MacOS scripts that can be squashed. And I think it makes sense to move the CI commit it self as the last commit

We depend on bitcoin-ffi now.

DanGould · 2025-03-25T17:31:25Z

yes

arminsabouri · 2025-03-25T17:49:52Z

yes

Moved the commit which adds the CI jobs as the last commit and squashed relevant commits.

The other commits atomically fix different things, not neccecarily for CI. For example removing windows support and fixing the x86 MacOS binary. Even without the last commit we would want these and they achieve independently useful things. I could make a monster commit that fixes the various problems that exist in main and then a CI commit but I do think there is an added benefit to having seperate atomic commits. For instance we could revert just the commit that removes window support in the future. Let me know how you would proceed

DanGould

Removing the windows script entirely seems not to be done.

Otherwise this is looking pretty darn good

DanGould · 2025-03-25T17:54:50Z

python/scripts/bindgen_generate.sh


 #!/bin/bash
 chmod +x ./scripts/generate_linux.sh
-chmod +x ./scripts/generate_windows.sh


Do you wish not also to remove the generate_windows script?

Agh Yeah this was done in a seperate commit. I will move that change to this commit

DanGould · 2025-03-25T17:57:58Z

.github/workflows/build-python.yml

+      - name: "Build wheel"
+        # Specifying the plat-name argument is necessary to build a wheel with the correct name,
+        # see issue BDK#350 for more information
+        run: python3 setup.py bdist_wheel --plat-name macosx_11_0_x86_64 --verbose


Should we mention, as bdk does, the rationale for testing only the x86 wheel? https://github.com/bitcoindevkit/bdk-ffi/blob/master/.github/workflows/test-python.yaml#L97

Yeah it could be worth having this

good comment. I actually realized that we are not building the arm64 wheel on CI. Will add this step.

DanGould · 2025-03-25T17:58:18Z

python/scripts/generate_windows.sh

 cd ../
-cargo run --features uniffi --bin uniffi-bindgen generate src/payjoin_ffi.udl --language python --out-dir python/src/payjoin/
-
+cargo run --features uniffi --bin uniffi-bindgen generate --library target/release/$LIBNAME --language python --out-dir python/src/payjoin/


It would seem this windows script should have been removed with its support a few commits earlier.

yes you are right

The windows binding support has gotten stale and there is no demand for it. Supporting these bindings is not worth the overhead of maintaining them.

We need a release built *.so and *.dylib file before running uniffi-bindgen. Without the share object file the uniffi command will fail to find the shared object and error out. For example: https://github.com/LtbLightning/payjoin-ffi/actions/runs/14045076123/job/39323956701?pr=60#step:4:474

`setup.py` depends on toml.

Skipping any unit test that is currently broken due to FFI bindings. Issue to re-enable them: LtbLightning#62

New CI job to build and run python unit tests. This gh action workflow builds for MacOS x86 and arm archs. While only running the tests on an x86 machine.

spacebear21

tACK 57aabaf

Tested on macOS, it took a bit of fiddling with python to get the incantations right but got build and test passing with:

python3 -m venv pjvenv
source pjvenv/bin/activate
bash scripts/generate_macos.sh
python3 setup.py bdist_wheel --verbose
pip3 install ./dist/*.whl
python3 -m unittest --verbose test/payjoin_unit_test.py

----------------------------------------------------------------------
Ran 6 tests in 0.000s

OK (skipped=5)

arminsabouri · 2025-03-25T19:46:57Z

tACK 57aabaf

Tested on macOS, it took a bit of fiddling with python to get the incantations right but got build and test passing with:

python3 -m venv pjvenv
source pjvenv/bin/activate
bash scripts/generate_macos.sh
python3 setup.py bdist_wheel --verbose
pip3 install ./dist/*.whl
python3 -m unittest --verbose test/payjoin_unit_test.py

----------------------------------------------------------------------
Ran 6 tests in 0.000s

OK (skipped=5)

@spacebear21 thanks for testing! Was this on an arm or x86 mac?

spacebear21 · 2025-03-25T19:55:14Z

Ah yes I should've specified, this was on ARM.

DanGould

ack 76282f1

arminsabouri force-pushed the py-bindings-ci branch 2 times, most recently from 140fecb to 20bfbb0 Compare March 22, 2025 02:21

arminsabouri and others added 3 commits March 24, 2025 13:31

Remove Python shared libraries

76282f1

These files should have never been commited to source. By removing these shared libraries we encourage developers to build from source instead of using this repo as a quasi package.

Remove extraneous '/' from build.yml

876d3bc

Rm --user from macos python gen script

18a72af

It's not compatible with venv.

arminsabouri force-pushed the py-bindings-ci branch from 20bfbb0 to b6efb20 Compare March 24, 2025 17:37

Get payjoin python version from payjoin-ffi

473498c

The version should, at least for now, without python-specific breaks, match payjoin-ffi/Cargo.toml

DanGould force-pushed the py-bindings-ci branch from b6efb20 to 75a1e46 Compare March 24, 2025 17:40

DanGould force-pushed the py-bindings-ci branch 2 times, most recently from 907c629 to d8a23f5 Compare March 24, 2025 17:50

arminsabouri force-pushed the py-bindings-ci branch 6 times, most recently from 1095192 to b12d171 Compare March 24, 2025 20:18

arminsabouri force-pushed the py-bindings-ci branch 12 times, most recently from 05ab71e to 1e7b834 Compare March 25, 2025 14:25

arminsabouri force-pushed the py-bindings-ci branch 5 times, most recently from fcd0baf to ad80cee Compare March 25, 2025 16:45

arminsabouri marked this pull request as ready for review March 25, 2025 17:00

Get OutPoint from the bitcoin module

00f7d7f

We depend on bitcoin-ffi now.

arminsabouri force-pushed the py-bindings-ci branch from ad80cee to 3d1a951 Compare March 25, 2025 17:30

DanGould reviewed Mar 25, 2025

View reviewed changes

arminsabouri added 5 commits March 25, 2025 14:16

Remove python bindings support for Windows

d681348

The windows binding support has gotten stale and there is no demand for it. Supporting these bindings is not worth the overhead of maintaining them.

Install dev-dependencies for python bindings

8358396

`setup.py` depends on toml.

Skip python unit tests that are broken

c9f9927

Skipping any unit test that is currently broken due to FFI bindings. Issue to re-enable them: LtbLightning#62

Add missing feature flag when gen x86 macos python bindings

5ad436f

arminsabouri force-pushed the py-bindings-ci branch from 3d1a951 to afd45e4 Compare March 25, 2025 18:22

Build python bindings in CI

57aabaf

New CI job to build and run python unit tests. This gh action workflow builds for MacOS x86 and arm archs. While only running the tests on an x86 machine.

arminsabouri force-pushed the py-bindings-ci branch from afd45e4 to 57aabaf Compare March 25, 2025 18:43

arminsabouri requested a review from DanGould March 25, 2025 19:12

spacebear21 approved these changes Mar 25, 2025

View reviewed changes

DanGould approved these changes Mar 25, 2025

View reviewed changes

DanGould merged commit 07eb8bf into LtbLightning:main Mar 26, 2025
13 checks passed

arminsabouri deleted the py-bindings-ci branch March 26, 2025 12:07

spacebear21 mentioned this pull request Apr 10, 2025

Update README for python setup #76

Closed

Conversation

arminsabouri commented Mar 22, 2025 • edited by DanGould Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould commented Mar 24, 2025

Uh oh!

DanGould commented Mar 24, 2025

Uh oh!

arminsabouri commented Mar 25, 2025

Uh oh!

DanGould commented Mar 25, 2025

Uh oh!

arminsabouri commented Mar 25, 2025

Uh oh!

DanGould commented Mar 25, 2025

Uh oh!

arminsabouri commented Mar 25, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

arminsabouri commented Mar 25, 2025

Uh oh!

spacebear21 commented Mar 25, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arminsabouri commented Mar 22, 2025 •

edited by DanGould

Loading