-
Notifications
You must be signed in to change notification settings - Fork 137
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
chore: migrate to pyproject.toml and hatch #371
Conversation
4c80d82
to
b675f81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me, but I'm not qualified to properly understand some of these changes. A review/pass through from somebody who knows Python well would be ideal.
e10d91e
to
324a624
Compare
@YOU54F, @sergeyklay and @mikegeeves I understand that this is a pretty substantial change to a foundational part of this project. Happy to organise a chat if it is easier to talk through this review. |
Yeah I'm up for that, I'm pretty flexible on times |
do we currently publish one package which contains all the pact-ruby-standalone archives for each platform, and this will allow us to create platform/arch specific packages, therefore distributing smaller packages to end users? If so that will be nice! I note We have some guidance on Alpine, and Pact, which differs depending on the Pact backed (Ruby vs Rust) some of the docs are here TL;DR
|
"Operating System :: Microsoft :: Windows", | ||
"Operating System :: POSIX :: Linux", | ||
"Programming Language :: Python", | ||
"Programming Language :: Python :: 3.8", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we dropping Python 3.6 and 3.7 as part of this?
If so relates to #350
3.7 went eol on 27 Jun 2023
Will there be changes in here that are not backwards compatible with 3.6/3.7? I assume this additional to the toml will explicitly stop them being able to install it (assumedly because we are only testing against 3.8 + in CI)
If for example, it was still compat with 3.7 but not defined here, could
- would a user on python 3.7 be blocked from using the package entirely, or would they just be stopped from having a pre-built wheel and have to build from source?
I'm just curious that if an end user needed to support their app on an older version and still wanted to use new versions of pact-python, could they do so, even if in an unsupported manner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put Python 3.6 and 3.7 support in the back of my mind temporarily, as I wanted to get Hatch to work first. I am happy to add them back in if that is really controversial, but my opinion is we should not support them.
To be more specific:
- Nothing in the library code has changed, and therefore I don't expect 3.6 and 3.7 to cause any issues with the upgrade. If someone really needed to support these versions, we could add this back in as required.
- 3.6 and 3.7 are no longer maintained by Python and therefore any security vulnerabilities discovered now will remain.
- The main issue is that a lot of dependencies required by pact-python themselves do not support 3.6 and/or 3.7. This resulted in effectively two separate dependency definitions: one set for supported versions of Python, and another set with versions pinned to historical versions. In addition, a lot of these older pinned versions have security vulnerabilities which we would require end-user t take on.
If there's no objections, I'll mark this PR as resolving #350 as well :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I'm down with that. If users need to support older versions of Python for their application, they are welcome to propose backports. Should we consider snap shotting the branch prior to merging this.
I suppose only speaking to you yesterday, we could support 3.6/3.7 via hatch, ergo meaning a user could if required, so the snapshot possibly isn't required.
We are indeed currently publishing one (rather large) package which contains all The new build process with
When it comes to the Rust core, we should be able to create wheels that target musl linux quite straightforwardly. As for the Ruby core at the moment, as there are no upstream musl binaries generated, the build process didn't work. I can look later whether we want to add a compatibility layer, but I think that should be reserved to a separate PR. |
<< : *BUILD_TEST_TASK_TEMPLATE | ||
- python -m pip install --upgrade pip pipx | ||
- pyenv rehash | ||
- pipx install hatch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we always want to run against the latest version of hatch, or do we want to pin this so it could be picked up via say a dependabot config (or similar) to ensure we can test version updates and ensure consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a note, my first experience was that I needed to install hatch, which is where I came back to the CI scripts to see how we are installing it :)
pact-python on feat/pyproject@origin:feat/pyproject via 🐳 desktop-linux via 🐍 v3.11.4 on ☁️ (eu-west-2)
🕙15:02:03 ❯ make
clean to clear build and distribution directories
examples to run the example end to end tests (consumer, fastapi, flask, messaging)
consumer to run the example consumer tests
fastapi to run the example FastApi provider tests
flask to run the example Flask provider tests
messaging to run the example messaging e2e tests
package to create a distribution package in /dist/
release to perform a release build, including deps, test, and package targets
test to run all tests
pact-python on feat/pyproject@origin:feat/pyproject via 🐳 desktop-linux via 🐍 v3.11.4 on ☁️ (eu-west-2)
🕙15:02:40 ❯ make clean
hatch clean
make: hatch: No such file or directory
make: *** [clean] Error 1
pact-python on feat/pyproject@origin:feat/pyproject via 🐳 desktop-linux via 🐍 v3.11.4 on ☁️ (eu-west-2)
🕙15:02:44 [🔴 USAGE] ❯ make package
hatch build
make: hatch: No such file or directory
make: *** [package] Error 1
pact-python on feat/pyproject@origin:feat/pyproject via 🐳 desktop-linux via 🐍 v3.11.4 on ☁️ (eu-west-2)
🕙15:02:51 [🔴 USAGE] ❯ make test
hatch run all
make: hatch: No such file or directory
make: *** [test] Error 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I think installing the latest version of hatch should generally not be an issue. Hatch is not an actually dependency of pact-python
, and instead is just a CLI interface and a build backend. I would be very surprised if regular updates to hatch broke this.
Of course, should hatch come out with a completely overhaul in v2
, then we might need to pin it then, but I'm happy to leave this as future problem.
In the same vein, I'm of two minds as to whether we should update system dependencies (apt
and brew
) as part of the workflow. Personally, I typically lean towards faster CI/CD builds and therefore use whatever packages are installed in the image, and then trust the CI/CD provider to regularly update these packages.
Lastly, would you like me to add a make
command to install hatch? Or not really worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, should hatch come out with a completely overhaul in v2, then we might need to pin it then, but I'm happy to leave this as future problem.
Yeah I'm okay with that, the CI will catch any changes, and alert, and we can action accordingly
In the same vein, I'm of two minds as to whether we should update system dependencies (apt and brew) as part of the workflow. Personally, I typically lean towards faster CI/CD builds and therefore use whatever packages are installed in the image, and then trust the CI/CD provider to regularly update these packages.
You get a more repeatable build by not pulling in new updates each version, I've always just updated as a matter of course, going from practises used in Docker files, but also find it incredibly annoying when building things locally that link dynamically, and the dep has been updated with a new named shared lib, causing my built programs to fail, until they are rebuilt. Also why I like statically compiled executables, that will always work, even if they are chonkier.
Lastly, would you like me to add a make command to install hatch? Or not really worth it?
could do, if we did I would favour it being installed with python, so it saves x platform package manager concerns
if we don't, just have it as a pre-req for development somewhere in the docs.
you could have a make doctor command, which checks if the hatch executable is available and alerts people on how to install it
RE: Musl packages Here is the current advice for alpine users with the ruby packages I not sure if that helps in getting the neccessary deps in place to build a Either way the acid test is just being able to run a pact test in an alpine docker, as you can today, albeit with our workarounds we already have an example docker alpine file here https://github.com/pact-foundation/pact-python/blob/master/docker/Dockerfile I don't think its a show stopper for me though, I would much rather tackle the alpine problem with you for pact-reference/pact_ffi and its consumption in users projects to try and make life on Alpine a possibility with the rust core. and if neccessary we can circle back on ruby, I have been tickling some alternative ruby packagers which provide single file executables, and musl based builds https://gist.github.com/YOU54F/3775e66e6090e0371c11601e6b75c305 and got enough knowledge of the traveling ruby system to build musl based rubies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with this, I think we might want to do a major bump if dropping python versions though.
Unless the convention is minor in Python land. ( I had a bit of questioning around this in Ruby land the other day) See thread
also consideration given in the docs for whatever the outcome is for alpine users
Regarding the dropping of Python version, I'll reword the commit that introduces the change and make it clear it is a In my experience, and based on the way dependencies have done this too, a minor version upgrade is sufficient in Python when changing the supported versions of Python. |
Awesome thanks for confirming
no worries 👍🏾 thanks for the heads up |
Signed-off-by: JP-Ellis <josh@jpellis.me>
The previous build process relied on `distutils` which is due to be deprecated in 3.12. Furthermore, the use of `setup.py` is now discouraged. The choice to migrate to `hatch` was made for the following reasons: - It offers a very simple management of the venv. No more need to `python -m venv .venv` and `pip install`, `hatch` handles all of that automatically when creating the virtual environment. - `hatch` supercedes `tox`, allowing for multiple python versions to be tested in a single command. - `hatch` manages the build process, and offers a nicer way to hook in a custom build process to download the `pact` standalone binaries. A minor change to the packaging of the library now places the binaries in `pact/bin` instead of `pact/bin/pact/bin`. The `constants.py` file has been accordingly updated to reflect this change in case anyone was making direct use of the binaries. While this change is rather significant, it should not affect the end user experience. Users will still be able to `pip install pact-python` from PyPI. Other than for the aforementioned, there has been no changes to the library code. Official support to Python 3.6 and 3.7 is dropped as part of this change as security fixes for these versions are no longer provided (ended 21 months ago for 3.6, and 3 months ago for 3.7). Furthermore, a number of dependencies have dropped support for these versions, and pinning historical versions of these dependencies is introducing known security vulnerabilities. BREAKING CHANGE: Drop support for Python 3.6 and 3.7 Resolves: #369 Refs: https://docs.python.org/3/whatsnew/3.10.html#distutils-deprecated Refs: https://setuptools.pypa.io/en/latest/userguide/quickstart.html#setuppy-discouraged Signed-off-by: JP-Ellis <josh@jpellis.me>
With Hatch as the new build system, the previous GitHub actions no longer work to lint, test, and publish the package. This commit makes use of pypa/cibuildwheel to build the package for multiple platforms, and then publishes the package to PyPI using the existing secrets. Signed-off-by: JP-Ellis <josh@jpellis.me>
324a624
to
6f82d7e
Compare
The CONTRIBUTING.md file seems to have remained unchanged and still contains version 3.6. |
👋 Hey @sergeyklay PR #390 contains updates to the contributing document, removing the mention of the python versions altogether ( i've just reread the updated doc proposed ) it's probably worth a mention in that PR, as to where a user can see the versions in use ( the toml file and ci are possibly better sources of truth and could have that information on the main readme - supported python versions - as it's relevant not just for developers but end users) in which case it might be additional duplication having it in contributing guidelines. we can just say check the readme supported versions section or check the ci matrix/pyproject.toml |
In most cases, I think people don't really care which version of Python is supported other than "the current one". If someone is looking for it, I think there are really two main places to look for this:
As for this repository, we have the former covered (it is in the new |
This PR overhauls the underlying build process for the library.
Background
The previous build process relied on
distutils
which is due to be deprecated in 3.12. As a result, it is necessary for us to look at updating the build system.Furthermore, the use of
setup.py
is now discouraged, and the preference is to declare as much of a package's information inpyproject.toml
.Choice
The choice to migrate to
hatch
was made for the following reasons:python -m venv .venv
andpip install
,hatch
handles all of that automatically when creating the virtual environment.hatch
supercedestox
, allowing for multiple python versions to be tested in a single command.hatch
manages the build process, and offers a nicer way to hook in a custom build process to download thepact
standalone binaries.A minor change to the packaging of the library now places the binaries in
pact/bin
instead ofpact/bin/pact/bin
. Theconstants.py
file has been accordingly updated to reflect this change in case anyone was making direct use of the binaries.While this change is rather significant, it should not affect the end user experience. Users will still be able to
pip install pact-python
from PyPI. Other than for the aforementioned, there has been no changes to the library code.CI/CD Changes
This PR also includes a commit migrating the CI/CD pipelines to make use of Hatch, and now builds the wheels using the pypa/cibuildwheel GitHub action. This action handles most of the logic to build and distribute binary wheels for all supported Python versions and platforms. The current list of wheels built are:
cp{38,39,310,311,312}
on:macosx_10_16_arm64.whl
macosx_10_16_x86_64.whl
manylinux_2_17_arch64
manylinux_2_17_x86_64
musllinux_1_1_x86_64
win32
win_amd64
pp{38,39,310}
win_amd64
macosx_10_16_x86_64
manylinux_2_16_x86_64
pypy only has support up to Python 3.10, and only recently has supported for arm64 build
Resolves: #350
Resolves: #369
Resolves: #389
Refs: https://docs.python.org/3/whatsnew/3.10.html#distutils-deprecated
Refs: https://setuptools.pypa.io/en/latest/userguide/quickstart.html#setuppy-discouraged