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

Restructure project to use pyproject.toml #712

Closed
wants to merge 32 commits into from
Closed

Restructure project to use pyproject.toml #712

wants to merge 32 commits into from

Conversation

semuadmin
Copy link

@semuadmin semuadmin commented Mar 31, 2023

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
Copy link
Author

semuadmin commented Apr 2, 2023

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
Copy link
Contributor

cclauss commented Apr 16, 2023

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
Copy link
Author

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.

MANIFEST.in Show resolved Hide resolved
@cclauss
Copy link
Contributor

cclauss commented Apr 16, 2023

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

I wrote that before ruff existed.

@semuadmin
Copy link
Author

@semuadmin
Copy link
Author

semuadmin commented Apr 16, 2023

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
Copy link
Contributor

cclauss commented Apr 16, 2023

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
Copy link
Author

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

@cclauss
Copy link
Contributor

cclauss commented Apr 17, 2023

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.

Co-authored-by: Christian Clauss <cclauss@me.com>
@semuconsulting semuconsulting closed this by deleting the head repository May 31, 2023
@cclauss
Copy link
Contributor

cclauss commented May 31, 2023

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

@semuadmin
Copy link
Author

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

PR reinstated as #729

@akx akx mentioned this pull request Dec 21, 2023
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.

pip install deprecation warning
3 participants