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

[BUG] Setuptools allow arbitrarily wrong configs in setup.cfg #4913

Open
abravalheri opened this issue Mar 24, 2025 · 11 comments
Open

[BUG] Setuptools allow arbitrarily wrong configs in setup.cfg #4913

abravalheri opened this issue Mar 24, 2025 · 11 comments

Comments

@abravalheri
Copy link
Contributor

The issue in #4910 unveiled something much deeper.
It seems that setuptools has been allowing arbitrary fields (like description_file described in #4910) in setup.cfg (possibly without a visible warning -- needs investigation)1.

Expected outcome

Setuptools should issue a deprecation warning when a invalid field in used in setup.cfg.

Footnotes

  1. Funny thing is that issue [BUG] Version 78.0.1 breaks install of ansible-vault package #4910 always involves description-file. I find it hard to believe that it is a coincidence, possibly many packages CTRC+C, CTRL+V the same config all over the place.

@zr40
Copy link

zr40 commented Mar 24, 2025

description-file in setup.cfg might have originated from Openstack's PBR in 2013 (then called oslo.packaging): https://github.com/openstack/pbr/blob/aa4641f0e6e668d4eb3a0c1f70a3e15e83097c6a/README.rst and they still support this field today, albeit in the underscore form: https://github.com/openstack/pbr/blob/master/pbr/util.py#L321

@fungi
Copy link
Contributor

fungi commented Mar 24, 2025

It goes back even further, PBR (nee oslo.packaging for a brief moment) was a fork of d2to1, which implemented support for description-file because Distutils2 had it.

@aman2454
Copy link

I have seen a different one than description-file,
setuptools.errors.InvalidConfigError: Invalid dash-separated key 'force-manifest' in 'sdist' (setup.cfg), please use the underscore name 'force_manifest' instead

@fungi
Copy link
Contributor

fungi commented Mar 24, 2025

The force-manifest entries look like they're for distutils, which allows passing default command-line options through setup.cfg files, and --force-manifest is one such distutils option.

I suppose these examples raise a question: should other packaging tools and SetupTools plugins be allowed to extend setup.cfg with their own directives alongside SetupTools use of the same file?

Apparently some do, and have for years. The heritage of setup.cfg files goes back beyond PBR/d2to1/Distutils2 to the original distutils which relied on it for declarative configuration decades before SetupTools (re)copied that design (you'll find references to setup.cfg files for distutils in documentation from 2001, possibly earlier). Allowing and ignoring unrecognized entries has provided backward-compatibility with existing packages already relying on this functionality for a very long time.

@abravalheri
Copy link
Contributor Author

abravalheri commented Mar 24, 2025

I suppose these examples raise a question: should other packaging tools and SetupTools plugins be allowed to extend setup.cfg with their own directives alongside SetupTools use of the same file?

This is something that needs to be part of the investigation for this issue. We don't want to break other tools. I suspect we should allow any keywords defined via distutils.setup_keywords entry-point, but I don't know if that is enough for compatibility.

This issue only applies to the [metadata] and [option] sections of setup.cfg, the rest can be arbitrary.

@abravalheri
Copy link
Contributor Author

I suspect we should allow any keywords defined via distutils.setup_keywords entry-point, but I don't know if that is enough for compatibility.

It seems this is not enough: https://inspector.pypi.io/project/pbr/6.1.1/packages/47/ac/684d71315abc7b1214d59304e23a982472967f6bf4bde5a98f1503f648dc/pbr-6.1.1-py2.py3-none-any.whl/pbr-6.1.1.dist-info/entry_points.txt#line.4

So we may have to find another alternative.

@herebebeasties
Copy link

We don't want to break other tools.

What you are still missing here despite the hundreds of thousands of man hours of disruption you caused globally yesterday is that you also don't want to break your own tool.

Given all the existing packages out there that have config keys that setuptools doesn't recognise, accepting those unknown keys isn't a bug, it is very much a feature.

At the very most you should log a warning that setuptools is ignoring them.

@eli-schwartz
Copy link
Contributor

This is something that needs to be part of the investigation for this issue. We don't want to break other tools. I suspect we should allow any keywords defined via distutils.setup_keywords entry-point, but I don't know if that is enough for compatibility.

It seems evident to me that the answer here is:

setuptools should not attempt to validate fields that don't belong to it. Let them be as right or as wrong as they please, it's not setuptools' place to tell them they are doing something wrong.

Either that, or else setuptools should declare that anyone passing arguments to a setuptools plugin is "in the wrong" and should be punished for it by failure. But setuptools has historically considered at least passing arbitrary plugin-defined kwargs to setuptools.setup() as "expected behavior of the extensibility interface". So I cannot see how e.g. pbr could be considered in the wrong for declaring that they own and will handle arbitrary additional keys in [metadata]. (This is entirely independent of the fact that pbr and friends owned [metadata] and setuptools was the interloper.)

@abravalheri
Copy link
Contributor Author

abravalheri commented Mar 27, 2025

setuptools should not attempt to validate fields that don't belong to it. Let them be as right or as wrong as they please, it's not setuptools' place to tell them they are doing something wrong.

Thanks @eli-schwartz, I was hopping that we could find a way to identify when it is safe to assume the field was intended to be consumed by setuptools. I do appreciate that it would be a huge challenge. For now this issue is in the stage of "brainstorming" the possibilities.

The problem of simply ignoring invalid fields is that users potentially think their configuration is doing something when it is not and then they end up publishing defective artifacts. Moreover, I suppose users would appreciate if they did not have to spend 5h debugging their package to find out that they should have written install_requires instead of install_dependencies (this is a crude hypothetical caricature, I just wanted to illustrate the point).

@jamesliu4c
Copy link

The problem of simply ignoring invalid fields is that users potentially think their configuration is doing something when it is not and then they end up publishing defective artifacts. Moreover, I suppose users would appreciate if they did not have to spend 5h debugging their package to find out that they should have written install_requires instead of install_dependencies (this is a crude hypothetical caricature, I just wanted to illustrate the point).

The way I see the tradeoff is, new packages don't get failure messages until further down the pipeline when debugging is harder (a few hours of work for a single developer, or a small group), or breaking the whole ecosystem when a deprecation becomes a removal (a few hours of work for hundreds or thousands of developers). Some tools, like ansible-vault are foundational infrastructure.

It is not as if there is much choice by the downstream consumers. We have to use setuptools because of a vast ocean of transitive dependencies that have been introduced by the dependencies we chose. So what do we do, pin 78.0.2? What if there are CVE fixes on the other side of the removal of a deprecation? Who is responsible for backporting?

It looks like we are discussing governance in a bug ticket again. Do we want to move this conversation, or keep it here?

@herebebeasties
Copy link

I was hopping that we could find a way to identify when it is safe to assume the field was intended to be consumed by setuptools

You have two distinct use cases here:

  1. Package authors using setuptools (who presumably don't also simultaneously use other things like distutils2 or pbr that use setup.cfg files)
  2. People building third party sdists that use setuptools, as part of pip install or uv sync or poetry install from another package

Provided setuptools can tell the difference (is there some context here you can use?), you could choose to validate more strictly under scenario #1 and be extremely permissive under scenario #2. This would solve your problem of "then they end up publishing defective artifacts" vs. breaking consumers at pip install time. (That won't solve the more general problem of actual backwards-incompatible changes in logic or similar, but that's not really what this issue is really about.)

(I think providing version constraints, backwards-compatibility guarantees, and branching strategies to cope with backporting critical/CVE fixes is somewhat outside the scope of this issue.)

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

No branches or pull requests

7 participants