Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructure project to use maturin to build pyo3 bindings #88

Merged
merged 10 commits into from
Mar 13, 2024

Conversation

Ce11an
Copy link
Contributor

@Ce11an Ce11an commented Mar 10, 2024

Thank you for submitting this pull request! We appreciate you spending the time to work on these changes.

Motivation for the Change

Objective

Adopt maturin as the build system for surrealdb.py.

Background

The primary goal behind this change is to streamline the build process for Rust's pyo3 bindings within surrealdb.py. maturin stands out as a robust and reliable build system, a fact well-documented by its successful adoption in projects such as pydantic-core and ruff. Its integration into surrealdb.py is expected to significantly enhance the development workflow, alongside improving testing and release processes.

Type of Change

  • 馃 Improvement (non-breaking change which improves an existing feature)

What does this change do?

Adds maturin as a build-system and removes now redundant files.

What is your testing strategy?

All tests are working. The "cross-build" jobs may need work. However, I cannot run those.

Is this related to any issues?

Have you read the Contributing Guidelines?

@Ce11an
Copy link
Contributor Author

Ce11an commented Mar 10, 2024

@maxwellflitton, what do you think about this change? I still need to adjust the CI jobs to install surrealdb.py with maturin. Linters, formatters, and static type-checkers should also be added. I can whack it all in one PR, or split it up. Thought I would get your opinion first. I probably should have made an issue... but it has nevertheless been fun to explore maturin.

@Ce11an Ce11an changed the title Restructured project to use maturin to build pyo3 bindings Restructure project to use maturin to build pyo3 bindings Mar 10, 2024
These scripts are replaced by `maturin`
`maturin` simplifies testing
Incorrectly added in previous commit. Also removing extra build jobs. We can see if some bits can be used for the "cross-build" jobs
I have a sneaky suspicions that the jobs failed due to an incorrect path
I cannot get it to work... I do not want to block this PR. This was going to be an addition to the codebase. This PR does not change the current behaviour of the tests.
@Ce11an
Copy link
Contributor Author

Ce11an commented Mar 10, 2024

@maxwellflitton, what do you think about this change? I still need to adjust the CI jobs to install surrealdb.py with maturin. Linters, formatters, and static type-checkers should also be added. I can whack it all in one PR, or split it up. Thought I would get your opinion first. I probably should have made an issue... but it has nevertheless been fun to explore maturin.

I fix the GitHub Actions. I have not changed the "cross-build" workflow. Some improvements could be made using maturin something like the below to build the distribution artefacts:

jobs:
  linux:
    runs-on: ubuntu-latest
    strategy:
      matrix:
        target: [x86_64, x86, aarch64, armv7, s390x, ppc64le]
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-python@v5
        with:
          python-version: '3.10'
      - name: Build wheels
        uses: PyO3/maturin-action@v1
        with:
          target: ${{ matrix.target }}
          args: --release --out dist --find-interpreter
          sccache: 'true'
          manylinux: auto
      - name: Upload wheels
        uses: actions/upload-artifact@v4
        with:
          name: wheels-linux-${{ matrix.target }}
          path: dist

  windows:
    runs-on: windows-latest
    strategy:
      matrix:
        target: [x64, x86]
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-python@v5
        with:
          python-version: '3.10'
          architecture: ${{ matrix.target }}
      - name: Build wheels
        uses: PyO3/maturin-action@v1
        with:
          target: ${{ matrix.target }}
          args: --release --out dist --find-interpreter
          sccache: 'true'
      - name: Upload wheels
        uses: actions/upload-artifact@v4
        with:
          name: wheels-windows-${{ matrix.target }}
          path: dist

  macos:
    runs-on: macos-latest
    strategy:
      matrix:
        target: [x86_64, aarch64]
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-python@v5
        with:
          python-version: '3.10'
      - name: Build wheels
        uses: PyO3/maturin-action@v1
        with:
          target: ${{ matrix.target }}
          args: --release --out dist --find-interpreter
          sccache: 'true'
      - name: Upload wheels
        uses: actions/upload-artifact@v4
        with:
          name: wheels-macos-${{ matrix.target }}
          path: dist

  sdist:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Build sdist
        uses: PyO3/maturin-action@v1
        with:
          command: sdist
          args: --out dist
      - name: Upload sdist
        uses: actions/upload-artifact@v4
        with:
          name: wheels-sdist
          path: dist

  release:
    name: Release
    runs-on: ubuntu-latest
    if: "startsWith(github.ref, 'refs/tags/')"
    needs: [linux, windows, macos, sdist]
    steps:
      - uses: actions/download-artifact@v4
      - name: Publish to PyPI
        uses: PyO3/maturin-action@v1
        env:
          MATURIN_PYPI_TOKEN: ${{ secrets.PYPI_API_TOKEN }}
        with:
          command: upload
          args: --non-interactive --skip-existing wheels-*/*

I did want to add the ability to test the package on Windows and MacOS. However, there seems to be an issue between GitHub, requests, docker, MacOS, and Windows. I have managed to run the tests locally on my Mac so I am not sure what the issue is. Maybe one for another PR. See the below for more details:

I appreciate this is a big change, with a lot of clean up. If I got time, I will add in some standardisation with linters, formatters, and static type-checkers.

Let me know what you think!

These were not included previously
@Ce11an Ce11an marked this pull request as ready for review March 10, 2024 22:05
@maxwellflitton
Copy link
Contributor

@Ce11an thanks for this amazing work. it was on my to-do list and you've knocked it out of the park. Can you make the pull request to the test-deployment branch so I can start checking out the CI for deployment builds?

@Ce11an Ce11an changed the base branch from main to test-deployment March 12, 2024 14:12
@Ce11an
Copy link
Contributor Author

Ce11an commented Mar 12, 2024

@Ce11an thanks for this amazing work. it was on my to-do list and you've knocked it out of the park. Can you make the pull request to the test-deployment branch so I can start checking out the CI for deployment builds?

Thanks, I had good fun doing it! I have updated the branch. Let me know if there is anything else I can assist with 馃憤馃徎

@maxwellflitton maxwellflitton merged commit 1b96693 into surrealdb:test-deployment Mar 13, 2024
5 checks passed
@Ce11an Ce11an deleted the maturin-build branch March 13, 2024 16:22
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.

None yet

2 participants