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

Migrate tool settings from setup.cfg to pyproject.toml #363

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

pepoluan
Copy link
Collaborator

What do these changes do?

Practically all still-maintained projects are migrating to have a Single Source of Truth for their builds: pyproject.toml

This is in accordance with the guidelines in PEP518 and PEP621.

I have ascertained that all tools aiosmtpd uses -- both as dependency or for development -- now have support for putting their configuration in pyproject.toml (some needs a bit of workaround but works well). This left us with an empty setup.cfg file, which I removed because it's no longer useful (all setup information is now in pyproject.toml as well).

Are there changes in behavior for the user?

None if user has the latest pip and setuptools.

Related issue number

None.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • tox testenvs have been executed in the following environments:
    • GitHub CI
  • Documentation reflects the changes
  • Add a news fragment into the NEWS.rst file

@pepoluan pepoluan added tech-debt Things that needs to be tidied up to avoid being bitten in the future... integration labels Jan 13, 2023
@pepoluan pepoluan added this to the 1.5 milestone Jan 13, 2023
setup.cfg Show resolved Hide resolved
Comment on lines +54 to +56
echo "Running flake8 ... there will be no output if no errors:"
python -m flake8 aiosmtpd housekeep.py release.py
echo "flake8 done."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added just to ensure that flake8 actually runs. flake8's silence on no error sometimes make me wonder if it ran or not 😅

Copy link
Member

Choose a reason for hiding this comment

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

FWIW it's quite common to wrap linters with the pre-commit.com too.

@@ -28,8 +28,12 @@
f"dist/aiosmtpd-{ver_str}-py3-none-any.whl",
]

RELEASE_TOOLKIT = ["setuptools", "wheel", "twine", "build"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

build is the new build system created by PyPA that supports PEP 517.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. You need build for making dists and twine for linting them.

Copy link
Member

Choose a reason for hiding this comment

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

Also, PyPI no longer supports GPG. It's possible to use Sigstore, though.

My PyPUG article has an example of that: https://packaging.python.org/en/latest/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RELEASE_TOOLKIT = ["setuptools", "wheel", "twine", "build"]
RELEASE_TOOLKIT = ["build", "twine"]

Copy link
Collaborator Author

@pepoluan pepoluan left a comment

Choose a reason for hiding this comment

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

A few comments

@codecov
Copy link

codecov bot commented Jan 13, 2023

Codecov Report

Merging #363 (75cc9d5) into master (79c8671) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #363   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines         1706      1706           
  Branches       310       310           
=========================================
  Hits          1706      1706           
Impacted Files Coverage Δ
aiosmtpd/__init__.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Aaaawesome! A few bits of feedback - not sure if anything really needs to change...

@@ -38,7 +36,7 @@ jobs:
# language=bash
run: |
python -m pip install --upgrade pip setuptools wheel
python setup.py develop
python -m pip install .
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should that be install -e .?

that's more equivalent to develop

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe? Ahaha this "install using pyproject.toml" is so new to me 😅

Copy link
Member

Choose a reason for hiding this comment

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

Yes, -e would be the direct equivalent.

@@ -110,7 +112,7 @@ jobs:
# Test deps
pip install colorama "coverage>=7.0.1" coverage[toml] "coverage-conditional-plugin>=0.5.0" packaging pytest pytest-cov pytest-mock
# Package deps
python setup.py develop
python -m pip install .
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question here, -e?


$ python3 -m venv /path/to/venv
$ source /path/to/venv/bin/activate
$ python setup.py install
$ python -m pip install -U pip setuptools
$ python -m pip install .
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think here we should definitely recommend editable mode, if we're suggesting folks clone flr dev. Otherwise they can just install from pypi

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as before. Let me do some study / experiments.

Copy link
Member

Choose a reason for hiding this comment

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

@pepoluan feel free to ask me if you have questions about Python packaging. I maintain PyPUG.

dependencies = [
"atpublic",
"attrs",
"typing-extensions ; python_version < '3.8'",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are typing extensions require to run aiosmtpd?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

iirc it's to implement Protocol, I think that's in the Tproxy code.

Not really sure if that's actually needed to just run ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Assuming that it's not needed for running we can put it in an extras_require (yeah, not extra_requires) section. I'm not sure what the precise form is for that, tho.

Copy link
Member

Choose a reason for hiding this comment

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

If that were true, it should be here, not in extras. FWIW it's only used in tests:

from typing_extensions import Protocol
. Judging by the lack of it in runtime code, it might be fine to drop it from the tests and deps.

@waynew
Copy link
Collaborator

waynew commented Mar 2, 2023

@pepoluan FYI I'm still super interested in this getting in. There is a merge conflict in __init__.py. I've (re)reviewed PRs newer and up to this one, and will probably review more tomorrow afternoon.

# region #### setuptools settings ##########

[tool.setuptools]
zip-safe = false
Copy link
Member

Choose a reason for hiding this comment

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

Why is it unsafe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration tech-debt Things that needs to be tidied up to avoid being bitten in the future...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants