zestsoftware / zest.releaser

Python software releasing made easy and repeatable
https://zestreleaser.readthedocs.io
GNU General Public License v2.0
198 stars 62 forks source link

Add support for pyproject.toml #415

Closed elisalle closed 12 months ago

elisalle commented 1 year ago

Implements #412 by adding support for pyproject.toml. To facilitate this, a new ZestReleaserConfig class has been made, which reads the zest.releaser settings from the SetupConfig, PypiConfig and PyprojectTomlConfig classes. Python packages are now built using pypa/build instead of invoking setup.py directly.

reinout commented 12 months ago

@mauritsvanrees this is the PR I mentioned yesterday. I'm going to take a look myself too, of course. As you liked the idea of 'build' support a lot, I'll have you take a look, too.

elisalle commented 12 months ago

Tests should pass now.

reinout commented 12 months ago

@mauritsvanrees : I talked with @elisallenens. Let's ignore the isort/black stuff and do that in a separate PR. We don't have a make beautiful or pre-commit with black and isort and even no github action that checks it. That's something I'll fix later on.

reinout commented 12 months ago

@mauritsvanrees : tests pass and it looks good to me now. So it is ready for you to give it a quick peek.

elisalle commented 12 months ago

Forgot one docs section, but I think I've got everything now.

reinout commented 12 months ago

I'll merge it, then I can more easily do some tests on 'master'.

mauritsvanrees commented 12 months ago

Two things I wondered, but maybe it is in the comments above.

Why [tool.zest-releaser]? I would have expected [tool.zest.releaser] and would slightly prefer that. I don't know if there is an agreement in the wider Python community about this.

I wonder if we should just be calling python -mbuild instead of importing build and calling its code. It reminds me of twine which might pull the rug out from under us if it decides to change its internals (see remarks in PR #309). But build does advertise an api, so we are probably safe.

mauritsvanrees commented 12 months ago

Why [tool.zest-releaser]? I would have expected [tool.zest.releaser] and would slightly prefer that. I don't know if there is an agreement in the wider Python community about this.

I found an example that uses both. It has for example:

[tool.hatch.version]
...
[project.optional-dependencies]
...
[tool.hatch.metadata.hooks.fancy-pypi-readme]
...
[tool.ruff.flake8-quotes]
...

So should be fine the way we have it now.

mauritsvanrees commented 12 months ago

Some more reports for good measure. I am releasing a couple of Plone packages. In general it works fine. No real problems.

What is not nice though, is that build gives several errors/warnings that I want to ignore for now, but that hold up the release, asking me "There were errors or warnings. Are you sure you want to continue? (y/N)?".

One is this:

/Users/maurits/community/plone-coredev/6.0/lib/python3.11/site-packages/setuptools/dist.py:955:
SetuptoolsDeprecationWarning: The namespace_packages parameter is deprecated.
!!

********************************************************************************
Please replace its usage with implicit namespaces (PEP 420).

See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages for details.
********************************************************************************

!!
ep.load()(self, ep.name, value)

The other is the following, possibly repeated for several directories:

/Users/maurits/community/plone-coredev/6.0/lib/python3.11/site-packages/setuptools/command/build_py.py:201: _Warning: Package 'Products.PlonePAS.profiles.default' is absent from the `packages` configuration.
!!

********************************************************************************
############################
# Package would be ignored #
############################
Python recognizes 'Products.PlonePAS.profiles.default' as an importable package[^1],
but it is absent from setuptools' `packages` configuration.

This leads to an ambiguous overall configuration. If you want to distribute this
package, please make sure that 'Products.PlonePAS.profiles.default' is explicitly added
to the `packages` configuration field.

Alternatively, you can also rely on setuptools' discovery methods
(for example by using `find_namespace_packages(...)`/`find_namespace:`
instead of `find_packages(...)`/`find:`).

You can read more about "package discovery" on setuptools documentation page:

- https://setuptools.pypa.io/en/latest/userguide/package_discovery.html

If you don't want 'Products.PlonePAS.profiles.default' to be distributed and are
already explicitly excluding 'Products.PlonePAS.profiles.default' via
`find_namespace_packages(...)/find_namespace` or `find_packages(...)/find`,
you can try to use `exclude_package_data`, or `include-package-data=False` in
combination with a more fine grained `package-data` configuration.

You can read more about "package data files" on setuptools documentation page:

- https://setuptools.pypa.io/en/latest/userguide/datafiles.html

[^1]: For Python, any directory (with suitable naming) can be imported,
even if it does not contain any `.py` files.
On the other hand, currently there is no concept of package data
directory, all directories are treated like packages.
********************************************************************************

!!
check.warn(importable)

It is good that I see this, but I would want to ignore it, without getting asked that question. It also interferes with the --no-input option, because it will quit then.

We could add this to the KNOWN_WARNINGS to ignore. But that would mean adding all those individual lines, or the first few words per line. I can easily imagine that these lines are different per Python/setuptools/build version, especially the line breaks being on different spots.

So: not really much that we can do about I fear. Something to get used to.

cc @gforcada as you will see this too. This is when using the fullrelease command created with the versions from https://github.com/plone/buildout.coredev/pull/876.

elisalle commented 11 months ago

Why [tool.zest-releaser]? I would have expected [tool.zest.releaser] and would slightly prefer that. I don't know if there is an agreement in the wider Python community about this.

pyproject.toml (and toml in general) uses dots for namespacing. If you write something like this:

[tool.zest.releaser]
release = false
[tool.pytest]
minversion = "6.0"

a parsing library, like tomli, will parse it like this:

{
    "tool": {
        "zest": {
            "releaser": {
                "release": False
            }
        },
        "pytest": {
            "minversion": "6.0"
        }
    }
}

which is fine if releaser is a subpackage of the zest package, and you will also have other zest subpackages, as is the case in the examples posted above; with [tool.ruff.flake8-quotes] ruff is the package. However, the other zest packages such as zest.zodbupdate (as far as I'm aware) do not work together with zest.releaser, so to me it made more sense to do

[tool.zest-releaser]
release = false
[tool.pytest]
minversion = "6.0"

which would be parsed as

{
    "tool": {
        "zest-releaser": {
            "release": False
        },
        "pytest": {
            "minversion": "6.0"
        }
    }
}

I couldn't find any other packages with dots in the package name itself, so I don't know about the wider Python consensus.

reinout commented 11 months ago

@mauritsvanrees : so "build" is effectively educating us all to modernize our packages :-) Irritating, but probably good.

Perhaps a ignore-build-warnings option to explicitly only look at errors and to ignore the warnings? Would that be a good idea?

reinout commented 11 months ago

@elisallenens : agree, zest-releaser seems to fit best.

mauritsvanrees commented 11 months ago

@elisallenens Thanks for the explanation, makes sense.

@reinout The problem is that we only have stdout and stderr. There is no stdwrn where commands send their warnings. We currently go over the stderr lines, and for each line try to determine if it is a warning. We could extend the KNOWN_WARNINGS a bit.

Actually, your suggestion could work. We could optionally ignore whatever is in stderr, and only look at the exit code of a command. Most commands will have an exit code larger than zero when something went wrong, so there it would work. But there can be commands that do not do this: that is probably the main reason why we have the current setup where we try to check if stderr has actual errors or only warnings.

Let's see. We only do the fancy errors/warning detection here. We do this when the process has a non-zero exit code, or stderr has a traceback, or show_stderr is true.

For the build commands, show_stderr is true. That is determined in these lines. I actually added the possible build commands there yesterday, at first it was only upload and register.

Previously, we would call python setup.py sdist or bdist_wheel, and this part of the code would ignore stderr and only look at the exit code. If we can rely on build always having a non-zero exit code in case of a real error, then we can keep show_stderr=False and my problem would be gone. That seems to be the case. I will try it out a bit more and make a PR.