eclipse / paho.mqtt.python

paho.mqtt.python
Other
2.19k stars 723 forks source link

Restructure project to use pyproject.toml #712

Closed semuadmin closed 1 year ago

semuadmin commented 1 year ago

setup.py is now deprecated and is scheduled to be dropped altogether in pip 23.

This PR makes a number of changes to update the project build framework:

  1. It updates the paho.mqtt project structure to use pyproject.toml, while retaining setuptools as a back-end build platform.
  2. It uses pyproject.toml tool configuration sections were available.
  3. It drops support for Python versions <=3.6 as these are now end of life.
  4. It adds a minimal VS Code workflow including the following tasks (Eclipse IDE users can simply ignore these):
    • The Build task will produce wheel (*.whl) and sdist (*.tar.gz) install packages in the dist folder which can be uploaded to PyPi using twine.
    • The Install Locally task will install this wheel using pip.
    • Other VS Code tasks have been added mirroring the current GitHub Actions (GHA) CI worfklows.

NB: this PR only changes the build setup - no changes have been made to existing application code or test cases, so some existing CI workflows may still fail if they were failing previously.

Fixes #706

semuadmin commented 1 year ago

Note to maintainers:

I've tested this pyproject.toml build and deployment framework against the existing PyPi v1.6.1 codebase and it builds, deploys and runs successfully. You can try for yourself using:

python3 -m pip install git+https://github.com/semuconsulting/paho.mqtt.python.git@revert-to-PyPi-1.6.1-codebase

I note, however, that the source code in the repo's master branch has moved on from the source currently deployed in PyPi, though the former's version is still marked as 1.6.1. If you actually package and install the current master branch, the code doesn't appear to work.

Just a suggestion, but it might be worth keeping the master branch in line with the corresponding release in PyPi and putting any intervening changes in a different 'release candidate' branch?

Also, I believe PEP518 pedants will argue that the application should take its version number from the [project].version tag in pyproject.toml, rather than the other way round. I understand the use of __version__ attributes in the source code (and other project artefacts) is deprecated in this new world. This avoids having to keep version numbers in sync across different project artefacts, but I appreciate that there are differences of opinion in this area and will leave it to your discretion. In the meantime the version is 'hard-wired' at 1.6.1 in pyproject.toml - I haven't touched the source code in any way.

cclauss commented 1 year ago

My suggestion would be to replace all the linters (flake8, pylint, pylama, bandit, pyupgrade) with a single, fast (<10 sec) GitHub Action which runs ruff as in #717

Then a separate GHA can focus on formatting (psf/black) and testing (pytest) and possibly add type hints (mypy).

Did you use tools to convert setup.py (setuptools-py2cfg) and setup.cfg (ini2toml) to pyproject.toml or was this a manual conversion?

semuadmin commented 1 year ago

My suggestion would be to replace all the linters (flake8, pylint, pylama, bandit, pyupgrade) with a single, fast (<10 sec) GitHub Action which runs ruff as in #717

Another day, another linter ;-)

I noticed that the existing project workflow has references to a variety of linters - I'm guessing that may reflect the historical predilections of the individual maintainers. I agree that it would probably be sensible to settle on one, but I'm happy to leave this to the discretion of the maintainers - it's not the main focus of this PR.

Having said that, I can't help but think the maintainers' precious time might be better spent addressing more urgent issues (like moving off setup.py asap) than beta-testing a new (though doubtless very capable) linter. In the meantime, pylint and flake are mature, proven, capable, robust and natively supported by all popular IDEs, and the ≈3.9 seconds it takes to run pylint against this library on a moderately capable laptop may not be such a big deal.

Did you use tools to convert setup.py (setuptools-py2cfg) and setup.cfg (ini2toml) to pyproject.toml or was this a manual conversion?

Manual - I already had a template I've used for other Python projects.

cclauss commented 1 year ago

I noticed that the existing project workflow has references to a variety of linters

I wrote that before ruff existed.

semuadmin commented 1 year ago

https://github.com/eclipse/paho.mqtt.python/pull/712/files#diff-a5de3e5871ffcc383a2294845bd3df25d3eeff6c29ad46e3a396577c413bf357R9

sorry, should be OK now

semuadmin commented 1 year ago

Re. the failed merge checks - I've not touched the source code or the existing GitHub Actions so these are presumably pre-existing issues.

I've amended tox.yml to remove reference to python 3.6 (which is now end-of-life) and update the checkout and setup-python actions to the latest node.js 16 versions (as per the deprecation warnings), but the lint_python step is failing because bandit detects "Use of weak SHA1 hash for security" in client.py. This either needs a code fix to use e.g. SHA2, or a relaxation of your bandit fail criteria.

Who are the maintainers expecting to resolve this issue?

cclauss commented 1 year ago

Please add the failing bandit code to the skip list like: https://github.com/eclipse/paho.mqtt.python/pull/717/files#diff-14721317565b7c66355f276ec27762054b12f390afd6c35fba85cfaf77d311c7R12

It is a newer code added since that GitHub Action was created.

semuadmin commented 1 year ago

Hi @cclauss So I take it you're dropping this PR in favour of your own #719?

cclauss commented 1 year ago

Not at all. This PR makes the tests pass. #719 --ignores two test files. I have approved this PR and believe that it must be merged.

cclauss commented 1 year ago

What happened here?!? These fixes were vital for getting the tests green again.

semuadmin commented 1 year ago

What happened here?!? These fixes were vital for getting the tests green again.

PR reinstated as #729