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

[Enhancement Request] _MissingDynamic with pyproject.toml should be a failing error, not a warning #4183

Open
pradyunsg opened this issue Jan 11, 2024 · 5 comments
Labels
Needs Discussion Issues where the implementation still needs to be discussed. proposal

Comments

@pradyunsg
Copy link
Member

setuptools version

setuptools == 69.0.3

Python version

N/A

OS

N/A

Additional environment information

No response

Description

(I was gonna ask if this behaviour is intentional as a comment on #4066, but figured a dedicated issue would be better)

Since #4066 / v69.0.0, setuptools does not raise an error when an metadata field is not declared as dynamic in pyproject.toml and ignores that metadata field entirely. The current behaviour is printing a warning like:

/private/var/folders/s0/qbw337m17yj9tq4xm822qwn40000gq/T/build-env-2mvjqejz/lib/python3.11/site-packages/setuptools/config/_apply_pyprojecttoml.py:75: _MissingDynamic: dependencies defined outside of pyproject.toml is ignored.

Full logs and source tree: https://gist.github.com/pradyunsg/e502bb5ca096e18308600f0009d86bb0

Expected behavior

Instead of printing this as a warning and silently ignoring information provided via the setup call, setuptools should error out on these cases since the transitory period has concluded.

How to Reproduce

  1. Create a directory containing...
❯ cat pyproject.toml
[project]
name = "example"
version = "1.0.0"
❯ cat setup.py
from setuptools import setup

setup(
    install_requires=["httpx"],
)
  1. Run python -m build on it (or python setup.py sdist, if you want a setuptools-only reproducer).

Output

https://gist.github.com/pradyunsg/e502bb5ca096e18308600f0009d86bb0

@pradyunsg pradyunsg added bug Needs Triage Issues that need to be evaluated for severity and status. labels Jan 11, 2024
@abravalheri
Copy link
Contributor

abravalheri commented Jan 11, 2024

Hi @pradyunsg, thank you very much for issue.

If I am not wrong the standard does not mandate an error in these circumstances1, right? The requirement is that the metadata should not be filled dynamically if dynamic is not set appropriately, and the setuptools implementation seems to be following that requirement.

The reason why there is a warning is the following: I have seem in the past several code bases where people add metadata to both pyproject.toml and setup.py in the hopes of handling old versions of setuptools. Hyrum's law observed, having a hard error there has the potential to backlash in the best style "it was working before and setuptools vXX broke it".

In the future, I hope to reduce the defensive programming in this part of the code (while respecting the standard), because it takes a toll in terms of complexity...

Users that want to be on the safe side and prefer harder checks can use -Werror or configure the warnings filter when appropriate.

Footnotes

  1. The other way around is required to have an error: if the user specifies a field in dynamic and the value cannot be determined, or is also given in project.<something>.

@pradyunsg
Copy link
Member Author

Hyrum's law observed, having a hard error there has the potential to backlash in the best style "it was working before and setuptools vXX broke it".

Well, without a hard error, it's still going to have breakage -- albeit it'll be a subtle breakage rather than a clean one.

For example, I had to spend a bunch of time diagnosing this for someone else because "this used to work, and now pip doesn't install dependencies anymore" was the observed behaviour change. An explicit error could have (1) clearly indicated to the user what changed and (2) pointed the user in the direction of what they should've done.

@abravalheri
Copy link
Contributor

abravalheri commented Jan 12, 2024

Hi @pradyunsg thank you very much for the clarification.
I will reclassify this message because to my best understanding setuptools is correctly following the project.dynamic spec1, so it is not really a bug, but rather an enhancement request. If my interpretation of the spec is wrong, please let me know.

Personally I am still not 100% certain about the impacts of the proposed change, if there will be any unintended consequence and the backlash it can cause (I am not sure to which extent it might affect other projects because it is very hard to foresee what changes in setuptools can cause... sometimes we change something small and then we have a very huge reaction). But if any member of the community is interested in carry out the PR and support the change (e.g. by properly addressing the feedback we might receive and reverting if necessary or implementing further changes that might be identified), I am happy to review and help to cut out a new release.

For example, I had to spend a bunch of time diagnosing this for someone else because "this used to work, and now pip doesn't install dependencies anymore" was the observed behaviour change. An explicit error could have (1) clearly indicated to the user what changed and (2) pointed the user in the direction of what they should've done.

I think one easy way out of this situation is to check the logs provided by setuptools.
The warning message produced by setuptools clearly indicates to the user a direction to solve the problem and explain why it is happening. It also is formatted in a way that it jumps out and is clearly evidenced. So just scrolling through the logs should be enough to catch the users attention. We can also apply any further formatting suggestions if that is going to help.

Footnotes

  1. I know we are falling behind in many other specs, but at least in this regard we should be good 😅.

@abravalheri abravalheri changed the title [BUG] _MissingDynamic with pyproject.toml should be a failing error, not a warning [Enhancement Request] _MissingDynamic with pyproject.toml should be a failing error, not a warning Jan 12, 2024
@abravalheri abravalheri added proposal Needs Discussion Issues where the implementation still needs to be discussed. and removed bug Needs Triage Issues that need to be evaluated for severity and status. labels Jan 12, 2024
@pradyunsg
Copy link
Member Author

it is not really a bug, but rather an enhancement request.

Seems reasonable to me!

Personally I am still not 100% certain about the impacts of the proposed change, if there will be any unintended consequence and the backlash it can cause (I am not sure to which extent it might affect

That's fair.

I think useful context to add: one of the reasons I've advocated for more explicit errors from setuptools in multiple cases... it aligns with the feedback we'd gotten during the UX discussions for pip in 2020 as well as my personal experience supporting developers diagnose "Python packaging issues" as a part of my day job for the last few years. :)

@abravalheri
Copy link
Contributor

abravalheri commented Jan 13, 2024

Thank you very much for the context. UX discussions and user studies are very useful, and are a wonderful methodology to guide software projects. But more often than not we also should take their conclusions with a pinch of salt and analyse situation by situation. For example in the setuptools code base we regularly receive requests contradicting each other, and usually what is good for a group of users, not necessarily is good for another... We constantly have to balance out multiple trade-offs.

We are trying our best to provide the most useful information for users, so that by scrolling the logs they can easily find the relevant warning message, and this message should contain tips of how to solve the problem1.

In this particular cause, you should be able to see something like the following message in your logs:

_MissingDynamic: `dependencies` defined outside of `pyproject.toml` is ignored.
!!

        ********************************************************************************
        The following seems to be defined outside of `pyproject.toml`:

        `dependencies = ['httpx']`

        According to the spec (see the link below), however, setuptools CANNOT
        consider this value unless `dependencies` is listed as `dynamic`.

        https://packaging.python.org/en/latest/specifications/declaring-project-metadata/

        To prevent this problem, you can list `dependencies` under `dynamic` or alternatively
        remove the `[project]` table from your file and rely entirely on other means of
        configuration.
        ********************************************************************************

!!
  _handle_missing_dynamic(dist, project_table)
/tmp/build-env-s8mz_ua3/lib/python3.8/site-packages/setuptools/config/_apply_pyprojecttoml.py:82: SetuptoolsWarning: `install_requires` overwritten in `pyproject.toml` (dependencies)
  corresp(dist, value, root_dir)

which should give the user a good indication of why the problem is happening and how to solve it.

Footnotes

  1. Last year we did a huge effort to improve warning messages making them more easy to spot and with more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion Issues where the implementation still needs to be discussed. proposal
Projects
None yet
Development

No branches or pull requests

2 participants