Skip to content

Conversation

@benalleng
Copy link
Collaborator

@benalleng benalleng commented Aug 21, 2025

This is a simplification for the python build and test workflow for linux and macos.
This also updates some of the setup.py to use the more up to date python -m build method

This is part 2 to supersede #980

TODO

look at the comments addressing migrating away from pip #864 (comment) actually since this change is touching more than just the workflow lets move it to a follow-up

Pull Request Checklist

Please confirm the following before requesting review:

  • A human has reviewed every single line of this code before opening the PR (no auto-generated, unreviewed LLM/robot submissions).
  • I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

@benalleng benalleng marked this pull request as draft August 21, 2025 18:46
@coveralls
Copy link
Collaborator

coveralls commented Aug 21, 2025

Pull Request Test Coverage Report for Build 17138567941

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 86.156%

Totals Coverage Status
Change from base Build 17137558247: 0.0%
Covered Lines: 7885
Relevant Lines: 9152

💛 - Coveralls

@benalleng benalleng force-pushed the python-workflow-optimize branch from c31f0d3 to 300c5c0 Compare August 21, 2025 18:57
Copy link
Collaborator

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

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

Concept ACK, same comments as the Dart PR with a few more suggestions specific to this one.

- name: "Build wheel"
run: python setup.py bdist_wheel --verbose
run: python -m build --wheel
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this change do?

Copy link
Collaborator Author

@benalleng benalleng Aug 21, 2025

Choose a reason for hiding this comment

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

It should in theory do nothing different, there were simply warnings that using setup.py directly was a deprecated method

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines +3 to +16
build-backend = "setuptools.build_meta"

[project]
name = "payjoin"
description = "The Python language bindings for the Payjoin Dev Kit"
readme = "README.md"
requires-python = ">=3.8"
license = "MIT"
dynamic = ["version"]

[tool.setuptools]
packages = ["payjoin"]
package-dir = { "payjoin" = "src/payjoin" }
include-package-data = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the gist of these update? Do they perhaps belong in a separate commit?

Copy link
Collaborator Author

@benalleng benalleng Aug 21, 2025

Choose a reason for hiding this comment

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

I can split them up, this was due to the setup.py usage being deprecated vs calling python -m build --wheel

@benalleng benalleng force-pushed the python-workflow-optimize branch 8 times, most recently from c362f8a to 92c1c78 Compare August 21, 2025 19:57
@benalleng benalleng marked this pull request as ready for review August 21, 2025 19:59
@benalleng benalleng force-pushed the python-workflow-optimize branch from 92c1c78 to 5c95feb Compare August 21, 2025 20:07
@benalleng
Copy link
Collaborator Author

benalleng commented Aug 21, 2025

Look at those SAVINGS!
dart:
https://github.com/payjoin/rust-payjoin/actions/runs/17137804396
python:
https://github.com/payjoin/rust-payjoin/actions/runs/17137804451

For comparison a build from before
dart:
https://github.com/payjoin/rust-payjoin/actions/runs/17133369618
python:
https://github.com/payjoin/rust-payjoin/actions/runs/17133369617

the one above is an extreme but we get on average ~20min in savings!

was it worth the entire day I've been stuck in this workflow doom loop?

I say YES!

@benalleng benalleng force-pushed the python-workflow-optimize branch from 5c95feb to e460446 Compare August 21, 2025 20:35
Copy link
Collaborator

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

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

utACK, awesome gainz

This is a simplification for the python build and test workflow for
linux and macos.
This also updates some of the setup.py to use the more up to date `python
-m build` method
setup.py is deprecated for use directly asa build tool
@benalleng benalleng force-pushed the python-workflow-optimize branch from e460446 to cff82bb Compare August 21, 2025 20:42
@benalleng
Copy link
Collaborator Author

latest commit just fixed a readme typo

@spacebear21 spacebear21 merged commit dd13330 into payjoin:master Aug 21, 2025
14 checks passed
benalleng added a commit to benalleng/rust-payjoin that referenced this pull request Aug 27, 2025
I moved this code incorrectly in payjoin#989 to a general binding build script but
others not using a debian based system immediately ran into problems running
this apt based package manager on their machine.

It was removed in payjoin#1000 from the binding script but This probably should
just be returned to the workflow it was originally.
benalleng added a commit to benalleng/rust-payjoin that referenced this pull request Aug 27, 2025
I moved this code incorrectly in payjoin#989 to a general binding build script but
others not using a debian based system immediately ran into problems running
this apt based package manager on their machine.

It was removed in payjoin#1000 from the binding script but This probably should
just be returned to the workflow it was originally.
DanGould pushed a commit to DanGould/rust-payjoin that referenced this pull request Aug 28, 2025
I moved this code incorrectly in payjoin#989 to a general binding build script but
others not using a debian based system immediately ran into problems running
this apt based package manager on their machine.

It was removed in payjoin#1000 from the binding script but This probably should
just be returned to the workflow it was originally.
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.

3 participants