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

Add support for pyproject.toml #415

Merged
merged 65 commits into from
Jul 12, 2023
Merged

Add support for pyproject.toml #415

merged 65 commits into from
Jul 12, 2023

Conversation

elisalle
Copy link
Contributor

@elisalle elisalle commented Jul 4, 2023

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.

zest/releaser/pypi.py Outdated Show resolved Hide resolved
elisalle and others added 6 commits July 11, 2023 09:51
Co-authored-by: Reinout van Rees <reinout@vanrees.org>
Co-authored-by: Reinout van Rees <reinout@vanrees.org>
Co-authored-by: Reinout van Rees <reinout@vanrees.org>
Co-authored-by: Reinout van Rees <reinout@vanrees.org>
@reinout
Copy link
Collaborator

reinout commented Jul 11, 2023

@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
Copy link
Collaborator

reinout commented Jul 11, 2023

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

@elisalle
Copy link
Contributor Author

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

@reinout
Copy link
Collaborator

reinout commented Jul 12, 2023

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

@mauritsvanrees
Copy link
Member

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

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

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 plone/buildout.coredev#876.

@elisalle
Copy link
Contributor Author

elisalle commented Jul 14, 2023

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

reinout commented Jul 14, 2023

@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
Copy link
Collaborator

reinout commented Jul 14, 2023

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

@mauritsvanrees
Copy link
Member

@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.

mauritsvanrees added a commit that referenced this pull request Jul 14, 2023
We only need to look at the exit code to see if it worked.
See #415 (comment)
mauritsvanrees added a commit that referenced this pull request Jul 14, 2023
We only need to look at the exit code to see if it worked.
See #415 (comment)
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.

3 participants