Skip to content

Conversation

@mg-twentyone
Copy link
Contributor

@mg-twentyone mg-twentyone commented Aug 5, 2025

Description

This PR updates the packaging and build system of bdkpython to align with modern Python packaging standards via pyproject.toml.
Changes:

  • Update pyproject.toml file according to PEP 621 and PEP 517
  • Removed the legacy setup.py file and usage
  • Add metadata such as keywords, classifiers, and URLs
  • Ensure the correct inclusion of Rust extension via [tool.setuptools-rust]
  • Update README.md file by including the new build process.

Fixes #12

Changelog notice

This pull request breaks the current building wheel process

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@thunderbiscuit
Copy link
Member

Hey this is awesome stuff @mg-twentyone! Are you on our Discord server for BDK? If so please send me a DM!

@mg-twentyone
Copy link
Contributor Author

hey @thunderbiscuit. I noticed a bug with the new configuration (project.toml) using setuptools-rust to include the Rust library. I am working to fix it. I will also try to update the workflow in the GitHub Actions by updating the build process for different operating systems. I joined the Discord channel, feel free to reach to me whenever you want. I will get in touch with you as soon as I finish it to provide u with all the details and simplify the review process.

@mg-twentyone mg-twentyone changed the title Migrate build process to PEP 621 compliance Migrate build process to PEP 621/517 compliance Aug 8, 2025
@thunderbiscuit
Copy link
Member

thunderbiscuit commented Aug 20, 2025

I have tested this and it works flawlessly. I'm impressed! Big update. I'd love to leave time for @reez to take a look, and maybe one of our production users to also take a look if he has time (@andreasgriffin).

Overall I think this is a major improvement! A few questions are left for me, particularly regarding the naming of the wheels (this is not new and not related to this PR so I don't think of it as a blocker though). But it just feels like the whole "you have to name your artifact with the correct string otherwise pip will not consider it valid for a given platform" (instead of leveraging package metadata) is super weird and cannot possibly be optimal. Anywa this is probably for a follow-up issue or PR because it's a long-standing issue with how we build and release this library. It works, but... yeah.

While we wait for review, 2 little things you can do @mg-twentyone:

  1. This PR needs a rebase on master
  2. The commits' messages will have to be slightly fixed to follow the BDK convention (conventional commits, with all lowercase lettering if possible).

@mg-twentyone
Copy link
Contributor Author

Great! I agree with you, the current management of the artifact name is not optimal, and we need to find a better solution to handle it.

Imo, we could consider opening a new dedicated issue on the current repo, transferring what @thunderbiscuit has already produced in the bdk-ffi repo (350), and perhaps including some details to reproduce the problem to facilitate development. This would make the flow cleaner and more traceable.

In the meantime, while waiting for the review, I rebased the PR on master and renamed the commit messages according to the convention.

@reez
Copy link
Collaborator

reez commented Aug 28, 2025

I have tested this and it works flawlessly. I'm impressed! Big update. I'd love to leave time for @reez to take a look, and maybe one of our production users to also take a look if he has time (@andreasgriffin).

Overall I think this is a major improvement! A few questions are left for me, particularly regarding the naming of the wheels (this is not new and not related to this PR so I don't think of it as a blocker though). But it just feels like the whole "you have to name your artifact with the correct string otherwise pip will not consider it valid for a given platform" (instead of leveraging package metadata) is super weird and cannot possibly be optimal. Anywa this is probably for a follow-up issue or PR because it's a long-standing issue with how we build and release this library. It works, but... yeah.

While we wait for review, 2 little things you can do @mg-twentyone:

  1. This PR needs a rebase on master
  2. The commits' messages will have to be slightly fixed to follow the BDK convention (conventional commits, with all lowercase lettering if possible).

Awesome modernization changes.

After reviewing, nothing to add to thunders comments/questions, although I do see the CI https://github.com/bitcoindevkit/bdk-python/actions/runs/17245037275/job/49120507517?pr=13 failures which I think are just due to the renaming of bdkpython to bdk-python?

@thunderbiscuit
Copy link
Member

Yep I think the renaming of the repo is the cause of the failures. @mg-twentyone can you fix these lines in the yaml:

      - name: "Configure Git safe directory"
        run: git config --global --add safe.directory /__w/bdkpython/bdkpython

They should reflect the new repo name bdk-python.

@thunderbiscuit
Copy link
Member

Awesome. This is a big step forward.

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK 17a58dd.

@thunderbiscuit thunderbiscuit merged commit 17a58dd into bitcoindevkit:master Aug 29, 2025
17 checks passed
@mg-twentyone mg-twentyone deleted the feat/new-build branch September 1, 2025 17:17
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.

Build warning relating to setup.py file

3 participants