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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FFI Groundwork #395

Merged
merged 6 commits into from
Oct 12, 2023
Merged

FFI Groundwork #395

merged 6 commits into from
Oct 12, 2023

Conversation

JP-Ellis
Copy link
Contributor

@JP-Ellis JP-Ellis commented Sep 26, 2023

This PR sets the groundwork to use the FFI. It ensures that the FFI is built during the wheel creation process thereby avoiding runtime slowdowns.

I have made sure that the commits are self contained for the review process. The commit messages are:

  • chore(ci): update build targets

    As the upstream Pact reference library has a different set of targets,
    the build targets for this library have been updated to match. The most
    significant change is the dropping is 32-bit architectures altogether.
    This also adds a musllinux target (which was previously not
    supported).

  • feat(v3): add v3.ffi module

    This module provides a Python interface to the Pact library written in
    Rust. For this first commit, only the pactffi_version() function is
    implemented and tested.

    In the transition to v3, the new codebase will be located within the
    v3 submodule. This will allow the v2 code to remain in place for
    backwards compatibility, and will allow the v3 code to be tested
    independently of the v2 code.

    Once the v3 code is complete, the existing v2 code will be scoped to a
    new v2 submodule, and the v3 code will be moved to the root of the
    repository.

  • chore(tests): add ruff.toml for tests directory

    Adjust the lint rules that apply to tests to be more permissive.
    Specifically to allow the use of assert statements.

  • style!: refactor constants

    With the possibility of building wheels against systems for which no
    Ruby executables exist, the constants module has been refactored to
    allow for the use of system installed Pact executables.

    This also introduces the USE_SYSTEM_PACT environment variable which
    can be used to force the use of system installed Pact executables.

    In doing these changes, the module has been refactored to avoid
    redundancies, and avoids the complexities of Windows executables
    extensions by using the shutil.which function.

    The test was refactored accordingly to test the constants instead of
    the functions.

    BREAKING CHANGE: The public functions within the constants module have
    been removed. If you previously used them, please make use of the
    constants. For example, instead of pact.constants.broker_client_exe()
    use pact.constants.BROKER_CLIENT_PATH instead.

    BREAKING CHANGE: It is possible to use the system installed Pact
    executables by setting USE_SYSTEM_PACT to True or Yes.

  • chore(build): update packaging to build ffi

    As we will transition to using the Rust Pact library, we need to
    download it as part of the build process. This commit adds a step to the
    build process to download the library and extract it to the correct
    location and then build the Python bindings.

    As the Rust library is available for more platforms than the Ruby
    executables, failing to find the Ruby executables is no longer a fatal
    error and will instead be raised as a warning during the build process.

    So small adjustments to the build script were made to accommodate this
    change.

A note about the CI/CD: Python 3.12 drops support for distutils (see PEP 632). The latest stable version of cffi at the time of creating this PR is 1.15 which still relies on distutils. This is resolved in the 1.16 release candidate and there won't be a long term issue.

Resolves: #384
Resolves: #242

@JP-Ellis JP-Ellis self-assigned this Sep 26, 2023
@JP-Ellis JP-Ellis force-pushed the feat/initial-ffi branch 5 times, most recently from 799909c to b1bcf4f Compare September 26, 2023 05:14
@mefellows
Copy link
Member

This is great!

BREAKING CHANGE: It is possible to use the system installed Pact
executables by setting USE_SYSTEM_PACT to True or Yes.

I didn't quite follow this (what was the previous behaviour and what is the new behaviour). Would this be obvious to users what it means?

Once the v3 code is complete, the existing v2 code will be scoped to a
new v2 submodule, and the v3 code will be moved to the root of the
repository.

Just so I'm clear. Given that the change in this PR is a breaking change (and thus requires a major version bump), this proposal would then require another major version bump when that package rework happens? I think that's probably fine, and preferably to having a long lived fork.

One question arises about major version support lifetime, and if bugs will be addressed/backported to them - will you create release branches for the previous major versions?

library written in Rust. This will allow us to support all of the features of
Pact, and bring the Python library in line with the other Pact libraries.

The migration will happen in stages, and this module will be used to provide
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

@mefellows mefellows left a comment

Choose a reason for hiding this comment

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

I have a few questions, but I think the general gist of this is good.

@JP-Ellis
Copy link
Contributor Author

This is great!

Thanks 😁

BREAKING CHANGE: It is possible to use the system installed Pact
executables by setting USE_SYSTEM_PACT to True or Yes.

I didn't quite follow this (what was the previous behaviour and what is the new behaviour). Would this be obvious to users what it means?

The current implementation bundles the Ruby executables and explicitly calls them. This was fine as long as we only distributed wheels to platforms for which Ruby executables exist; however, the platforms supported by the Rust library are different.

This would not be a breaking change for the vast majority of people, and most users would not notice this. So while yes it is technically a breaking change, the impact on end users is rather negligible.

Just so I'm clear. Given that the change in this PR is a breaking change (and thus requires a major version bump), this proposal would then require another major version bump when that package rework happens? I think that's probably fine, and preferably to having a long lived fork.

One question arises about major version support lifetime, and if bugs will be addressed/backported to them - will you create release branches for the previous major versions?

This breaking change is the only one I think will be necessary while working on the pact.v3 module. As the impact is very very minimal (and the remedy is simple), I am thinking we might be able to get away with a minor version bump only. I still would want to make sure this is thoroughly explained in the release notes in case anyone has any issues.

@JP-Ellis
Copy link
Contributor Author

Just fixed a typo preventing the linux aarch64 library from being downloaded.

@YOU54F, whenever you have time, I'd love to hear your thoughts on this (and if you're too busy, happy to proceed with Matt's approval).

@JP-Ellis JP-Ellis mentioned this pull request Oct 2, 2023
@YOU54F
Copy link
Member

YOU54F commented Oct 5, 2023

This also adds a musllinux target (which was previously not
supported).

YASSSSS! This will be an epic win for Pact-Python over its other client language counterparts, nice work @JP-Ellis 👏🏾

Local Testing

You can take the boy out of testing, but you can't take the tester out of the boy!

  • Linux-aarch64 ✅ (docker on mac)
  • Linux-aarch64-musl ✅ (docker on mac)
  • Linux-x86_64 ✅ (docker on mac)
  • Linux-x86_64-musl ✅ (docker on mac)
  • Darwin-arm64 ✅ (m1 pro)
  • Darwin-x86_64 🚧 (intel i9)
  • Windows-x86_64 🚧 (intel i9/amd 5900x)

Notes

arch64 musl testing using my forked release here with an aarch64 musl .a release

Notes: We should probably get that on the list of additional targets to add to pact-reference libpact_ffi 👍🏾

Hatch - extra test args, or selective test running

I also updated in the my local testing, the hatch test command

test = "pytest {args:--cov-config=pyproject.toml --cov=pact --cov=tests tests}"

so I could pick up args for running with -rP now with hatch run test tests -rP or a single test with hatch run test/test_ffi.py -rP (i wanted to see the std out printing of the lib just for completeness in my world xD) Maybe you have a more elegant way! Basically I want to pass extra args, or run a subset of the tests

Dockerfiles

I've noted the need to update the Dockerfiles as they still use tox, I updated them locally as part of my testing

I'll drop a separate PR to update those

I will also

  • move the ones at the root of the repo, into the /docker folder.
  • remove the ubuntu one, it isn't necessary as we have a debian flavour

pact/constants.py Outdated Show resolved Hide resolved
hatch_build.py Show resolved Hide resolved
@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented Oct 5, 2023

Thanks for all of the testing!

Hatch - extra test args, or selective test running

I also updated in the my local testing, the hatch test command

test = "pytest {args:--cov-config=pyproject.toml --cov=pact --cov=tests tests}"

so I could pick up args for running with -rP now with hatch run test tests -rP or a single test with hatch run test/test_ffi.py -rP (i wanted to see the std out printing of the lib just for completeness in my world xD) Maybe you have a more elegant way! Basically I want to pass extra args, or run a subset of the tests

For some reason, I had it in my mind that I have already done this... but clearly not 😅. This was definitely the intention and let me fix that up now! EDIT: Created #408 to track this minor issue.

Dockerfiles

I've noted the need to update the Dockerfiles as they still use tox, I updated them locally as part of my testing

I'll drop a separate PR to update those

I will also

  • move the ones at the root of the repo, into the /docker folder.
  • remove the ubuntu one, it isn't necessary as we have a debian flavour

I was considering migrating the various Dockerfiles to a Dev Container. I have created an issue to track this at #407.

@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented Oct 6, 2023

I have updated this PR to:

  • Incorporate changes as suggested by @YOU54F. Specifically:
    • The USE_SYSTEM_PACT environment variable has been changed to PACT_USE_SYSTEM_BINS. Also updated the commit message and the BREAKING CHANGE docs.
    • I have fixed the exclusion of the lib/ directory.
  • Be rebased on top of the more recent changes in master, resolving the merge conflict.

@YOU54F Let me know if you think this is good to merge 🎉

@YOU54F
Copy link
Member

YOU54F commented Oct 6, 2023

Great thanks dude, will take a look this morning

@YOU54F YOU54F self-requested a review October 12, 2023 16:32
Copy link
Member

@YOU54F YOU54F left a comment

Choose a reason for hiding this comment

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

Tested this out locally and it was all good, nice work, thanks for getting the ruby runtime working again, and great to see alpine officially supported for the ffi.

🙌🏾

As we will transition to using the Rust Pact library, we need to
download it as part of the build process. This commit adds a step to the
build process to download the library and extract it to the correct
location and then build the Python bindings.

As the Rust library is available for more platforms than the Ruby
executables, failing to find the Ruby executables is no longer a fatal
error and will instead be raised as a warning during the build process.

So small adjustments to the build script were made to accommodate this
change.

Signed-off-by: JP-Ellis <[email protected]>
With the possibility of building wheels against systems for which no
Ruby executables exist, the constants module has been refactored to
allow for the use of system installed Pact executables.

This also introduces the `PACT_USE_SYSTEM_BINS` environment variable
which can be used to force the use of system installed Pact executables.

In doing these changes, the module has been refactored to avoid
redundancies, and avoids the complexities of Windows executables
extensions by using the `shutil.which` function.

The test was refactored accordingly to test the constants instead of
the functions.

BREAKING CHANGE: The public functions within the constants module have
been removed. If you previously used them, please make use of the
constants. For example, instead of `pact.constants.broker_client_exe()`
use `pact.constants.BROKER_CLIENT_PATH` instead.

BREAKING CHANGE: It is possible to use the system installed Pact
executables by setting `PACT_USE_SYSTEM_BINS` to `True` or `Yes` (case
insensitive).

Signed-off-by: JP-Ellis <[email protected]>
Adjust the lint rules that apply to tests to be more permissive.
Specifically to allow the use of `assert` statements.

Signed-off-by: JP-Ellis <[email protected]>
This module provides a Python interface to the Pact library written in
Rust. For this first commit, only the `pactffi_version()` function is
implemented and tested.

In the transition to v3, the new codebase will be located within the
`v3` submodule. This will allow the v2 code to remain in place for
backwards compatibility, and will allow the v3 code to be tested
independently of the v2 code.

Once the v3 code is complete, the existing v2 code will be scoped to a
new `v2` submodule, and the v3 code will be moved to the root of the
repository.

Signed-off-by: JP-Ellis <[email protected]>
As the upstream Pact reference library has a different set of targets,
the build targets for this library have been updated to match. The most
significant change is the dropping is 32-bit architectures altogether.
This also adds a `musllinux` target (which was previously not
supported).

Signed-off-by: JP-Ellis <[email protected]>
When packaging the Ruby Pact binaries, I initially removed the `lib` dir
naively believing that the Pact binaries were static. This is in fact
incorrect, and I adjusted the extraction to extract _all_ of the
content (and only remove the README.md).

The unit tests all passed which affirmed my initial belief.
Unfortunately (as I have now discovered), the unit tests mock out the
call to the binaries, and therefore the test suite did not actuall test
the execution of the binaries.

Signed-off-by: JP-Ellis <[email protected]>
@JP-Ellis
Copy link
Contributor Author

Did a rebase to resolve a merge conflict. Checked the failing tests, and they are purely related to the release of Python 3.12 (and can be fixed at a later date, through #405).

@JP-Ellis JP-Ellis merged commit 5688e3c into master Oct 12, 2023
26 of 30 checks passed
@JP-Ellis JP-Ellis deleted the feat/initial-ffi branch October 12, 2023 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Completed
Development

Successfully merging this pull request may close these issues.

Create a Python wheel to bundle the Pact-Rust shared libs Installer
3 participants