Skip to content

Conversation

@mg-twentyone
Copy link
Contributor

Description

This PR adds the feature to build and upload the documentation for bdk-python. Sphinx is used as docs generator and sphinx_rtd_theme is utilized as the theme template.

CI: a new workflow has been implemented to manage the building and uploading process. A workflow_dispatch event trigger is set up; next improvement could be to add an trigger handler for external event when a new PR is merged in the bdk-ffi library.

Minor: small css modifications have been applied to the sphinx theme to improve readability.

Fixes #11

Notes to the reviewers

CI workflow's structure maintains coherence with other worflows to ensure cross-os compatibility. For the macos-arm64 job, the github runners VM has been updated from macos-13 to macos-14 to ensure compatibility with the arm64 architecture (github runners macos-13 vm runs on Intel architecture; check it here).

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

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

- name: "Upload API Docs"
uses: actions/upload-artifact@v4
with:
name: bdkpython-macos-x86_64-${{ matrix.python }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: add -api-docs because it looks like that's the pattern you're using in other places?

@reez
Copy link
Collaborator

reez commented Sep 15, 2025

This is looking nice. One nit comment.

Is this just the build part of #11 or is it addressing the docs website part too? I'm ok w separate PRs, just wanted to make sure in case it would automatically close out #11

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.

Works great for me locally!

Image

I have 2 requests:

  1. I think we can simplify the workflow a lot because building for all versions is not something we need. You don't need the matrix in this case, and you can build it just on the ubuntu-24.04 machine, which is I assume the fastest. The uploaded artifact I'll just PR on the bitcoindevkit.org website.
  2. Can you add a justfile command to build the docs locally?

This is really cool stuff!

@thunderbiscuit
Copy link
Member

thunderbiscuit commented Sep 15, 2025

And yes @reez I'd be ok with this closing #11. It won't automatically publish the docs, but I can do that later.

In fact, I wonder if we should just publish those here as GitHub pages instead of on the website. That could be something we expect some of those libraries developed outside of bdk-ffi to do? This could be done automatically in the action and triggered manually on tags (but we can leave that for a further PR).

@reez
Copy link
Collaborator

reez commented Sep 15, 2025

And yes @reez I'd be ok with this closing #11. It won't automatically publish the docs, but I can do that later.

In fact, I wonder if we should just publish those here as GitHub pages instead of on the website. That could be something we expect some of those libraries developed outside of bdk-ffi to do? This could be done automatically in the action and triggered manually on tags (but we can leave that for a further PR).

Sweet

@mg-twentyone
Copy link
Contributor Author

Great!
Let's try to clarify all the points:

  • The decision to extend the workflow to different OS is due to the fact that the Python package, which includes the compiled Rust library, is not pure Python. This can lead to different behaviors in Sphinx's importer across various OS. However, this is mainly related to the generation of the bindings and does not impact the structure of the produced documentation. It's more for testing purposes.
    Anyway, in this case, I agree with @thunderbiscuit; we can generate the documentation exclusively on Linux. This reduces complexity, optimizes resources, and speeds up the process.

  • Currently, the workflow produces only the artifact containing the HTML documentation (let me know if you prefer to create a zip folder). The direct docs upload is closely tied to the hosting provider (is bitcoindevkit.org hosted on Fastly?), which could make the implementation less flexible reducing portability.
    Imo, using GitHub Pages for libraries developed outside of bdk-ffi could be a good practice and may provide greater flexibility to the solution.
    Please, let me know how you prefer to proceed and whether we want to include this step in this PR or not.

  • Ok with adding a command to the justfile to build the docs locally.

I look forward to your feedback before proceeding with the changes.
Thank you both for the review!

@thunderbiscuit
Copy link
Member

thunderbiscuit commented Sep 17, 2025

Yes to both your points above @mg-twentyone!

We just need to build the docs to create the html files I'll add to the pages manually, so building only once makes sense, and for now I don't intend on making the deployment to bitcoindevkit.org automated, so that can also wait for a further PR. If we instead decide to just publish on the github pages of this repo, then that step should be easy to add in a further PR as well!

In other words, just generating the artifact is perfect for now.

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 3741c06.

@thunderbiscuit thunderbiscuit merged commit 3741c06 into bitcoindevkit:master Sep 22, 2025
17 checks passed
@mg-twentyone mg-twentyone deleted the docs/api-docs branch September 24, 2025 10:08
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 API documentation

3 participants