SAML-Toolkits / python3-saml

MIT License
704 stars 309 forks source link

improve packaging, migrate to setup.cfg #397

Closed deronnax closed 5 months ago

deronnax commented 9 months ago

declarative config (putting all the packaging information into the setup.cfg) is superior to imperative config (using code in setup.py) (source: setuptools themselve)

You lose the ability to do python setup.py test but managing tests with setup.py is deprecated and so is calling setup.py via the command line.

deronnax commented 9 months ago

398 should be merged first

sergei-maertens commented 9 months ago

Shouldn't this entirely setup.py just be removed? As far as I can tell all the packing info is already in pyproject.toml and poetry-based. I found out about this the hard way in our fork, and there's a serious lack of documentation on how to actually build and publish the package:

poetry build
poetry publish
Viicos commented 9 months ago

The relevant info is here:

https://github.com/SAML-Toolkits/python3-saml/blob/a1211a8695c855b74591a607cf589682307572a6/pyproject.toml#L79-L85

the build backend points to Poetry so any setuptools related files (setup.py, setup.cfg) is currently/will be ignored.

If you want to stick with Poetry, I would suggest using:

[build-system]
requires = ["poetry-core>=1.0.0"]
build-backend = "poetry.core.masonry.api"

There's no need to require poetry and all the dependencies when building

deronnax commented 9 months ago

Ah damn. All this work for almost nothing! I wasn't sure Poetry was the way to go, because it's not used in CI, so I was mislead thinking Poetry was not used (and actually it might not be, since setup.py metadata are still relevant. But since the package is not built in the CI, we can't know). Is the maintainer still out there? @pitbulk ?

sergei-maertens commented 9 months ago

Yeah I don't know a lot about poetry, but it looks like the pip install -e . in the makefile does somehow use pyproject.toml and poetry build systems etc.

I've recently gone through the same surprises and frustrations - so some documentation improvements and cleanups (removing setup.py completely) would definitely clarify the situation.

Viicos commented 9 months ago

I was mislead thinking Poetry was not used (and actually it might not be, since setup.py metadata are still relevant)

Build frontend tools such as pip will make use the build-system.build-backend key if present in pyproject.toml. So doing a plain pip install . locally will use poetry to install it (if the key is not present, build frontends should default to setuptools iirc).

However, the CI doesn't seem to handle packaging and publishing (as you mentioned), so maintainers might be forcing setuptools to be used in some way.

The metadata and packaging configuration would probably benefit from a large cleanup. We couldn't really do it in our fork to avoid merge conflicts

deronnax commented 9 months ago

Agreed. I will do all these cleanup if the maintainer comes back

pitbulk commented 9 months ago

@deronnax, Maintainer here, what do you need?

deronnax commented 9 months ago

You direction to do some clean up work. Do you use the setup.py or Poetry to manage dependencies and build a release?

pitbulk commented 9 months ago

To build the release I use setup.py

python setup.py sdist bdist_wheel upload -r pypi

or

python setup.py sdist bdist_wheel
twine upload dist/<package_name_and_version>-py3-none-any.whl
sergei-maertens commented 9 months ago

But the setup.py is not picked up for local development due to the presence of pyproject.toml which specifies poetry as build tool.

In the current situation you need to make the same change to setup.py and pyproject.toml (e.g. when specifying new or newer dependencies, as that's how I found this out).

Please pick one method.

pitbulk commented 9 months ago

When I was porting from setup.py to pyproject.toml I experienced some challenges, that why I ended with both.

You can read this thread where people experienced similar issues: https://discuss.python.org/t/user-experience-with-porting-off-setup-py/37502/18

Viicos commented 9 months ago

I can give it a shot if you are ok with it?

deronnax commented 5 months ago

I'll let @Viicos do the work on his PR.