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

Allow file: for requires statements in setup.cfg #3253

Merged
merged 2 commits into from Jun 19, 2022

Conversation

akx
Copy link
Contributor

@akx akx commented Apr 6, 2022

Summary of changes

As requested and discussed in #1951, this PR enables using file: in the various requires statements install_requires and extras_require statements in setup.cfg.

As @jaraco mentioned in #1951 (comment), this adds a (small) admonition advising against library packagers pinning their dependencies too tightly with this feature.

Closes #1951

Pull Request Checklist

  • Changes have tests
  • Changes are documented
  • News fragment added in changelog.d/.

Copy link
Contributor

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @akx, thank you very much for analysing the previous discussion and providing the PR.

I will leave the review to the maintainers that were participating in the original discussion, but I would like to note that this implementation does not seem to be forward compatible, because it only tackles setup.cfg.

According to #1688 (comment), the long term view is to compile setup.cfg into pyproject.toml, therefore it would be necessary to support this feature in pyproject.toml as well.

@akx
Copy link
Contributor Author

akx commented Apr 6, 2022

[...] the long term view is to compile setup.cfg into pyproject.toml, therefore it would be necessary to support this feature in pyproject.toml as well.

Ah, sure thing! I can also take a look at getting that in here, if the approach seems otherwise sound by maintainers :)

@abravalheri
Copy link
Contributor

abravalheri commented Apr 6, 2022

Thank you very much for considering that @akx. Yes, it is definitely good to wait for the feedback before making any change.

@jaraco
Copy link
Member

jaraco commented Apr 7, 2022

I’m okay to give it a try.

@akx
Copy link
Contributor Author

akx commented Apr 7, 2022

@abravalheri Hmm, by the looks of it enabling something like

[tool.setuptools.dynamic.optional-dependencies.bonus]
file = ["requirements-bonus.txt"]

(along the lines of

[tool.setuptools.dynamic.version]
attr = "pkg.__version__.VERSION"
[tool.setuptools.dynamic.readme]
file = ["README.md"]
content-type = "text/markdown"
)

would require changes to the JSON Schema for pyproject.toml, which... apparently is maintained in validate_pyproject? Am I on the right track there? apparently is maintained in validate_pyproject, and I took the plunge, adding akx/validate-pyproject@9019d53 ...
However, we presumably do want for a pyproject.toml to be able to contain statically defined optional dependency groups and file-based dependency groups, so setuptools.config._validate_pyproject.extra_validations.RedefiningStaticFieldAsDynamic would need to be augmented to allow that... so as not to muddy the waters here, maybe implementing this for pyproject.tomls should be done in a separate constellation of PRs?

@abravalheri
Copy link
Contributor

abravalheri commented Apr 7, 2022

Hi @akx, If you change the code in setuptools, I am more than happy to follow up and tackle the schemas myself.

For the time being, you can create a separated test case that uses you the ignore_option_errors parameter to bypass the validation. Once I modify the schemas, I will also modify that.

@akx
Copy link
Contributor Author

akx commented Apr 7, 2022

@abravalheri Please see #3255 for the TOML case :)

setup_requires list-semi 36.7.0
install_requires list-semi
extras_require section [#opt-2]_
setup_requires file:, list-semi 36.7.0 [#opt-5]_
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: would it make more sense to restrict this change to install_requires and extras_requires?

tests_require is deprecated, so I feel like we should not touch it.

setup_requires is largely obsoleted by PEP 518.
In very specific use cases people might still reach for it inside setup.py, but not in the context of setup.cfg. I am afraid that adding support for file: here would encourage people from using this (mostly obsolete) field instead of relying on PEP 518.

I understand that adding support for the file: directive for setup_requires and tests_require basically come for free, but I still think we should refrain from doing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get where you're coming from here, but various comments in #1951 say file: support is the last thing preventing them from moving from an imperative setup.py to a PEP 518 setup.cfg build. So, if those folks had been using files for their setup_requires and tests_require, not allowing file: for them would make that porting work less straightforward.

I have no other strong opinion on this, just that thought :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much @akx for pointing this out. I re-read the discussion in issue 1951 and I did not see any special request for setup_requires and tests_requires.

My view is that tests_requires, already does not work, so I don't think it would be helping anyone interested in the migration.

On the other hand, setup_requires only have value (right now) when computed dynamically via setup.py.

However I can see people starting to use it once it gets file: support just to workaround limitations in whichever GitHub bots they might be using for automatic dependency updates. In my opinion that would be an unfortunate side effect, since PEP 518 should be preferred. Any problems with these automation bots should be addressed by the bots themselves, not by undermining PEP 518.


On a side note, considering the forward compatibility with pyproject.toml:

  • Currently setup_requires can be automatically converted to [build-system] requires by an automatic generator.
  • If setup_requires gains the capability of being dynamically defined via file:, the translation will no longer be straight forward and we will have to update Allow file: for dependencies in TOML #3255 to handle this new use case.

@akx
Copy link
Contributor Author

akx commented Jun 8, 2022

@abravalheri @jaraco I rebased this to get rid of the merge conflicts. What are the next steps to get this merged..?

@abravalheri
Copy link
Contributor

Thank you very much @akx for rebasing the PR.

Personally, I have some strong reservations against handling dynamic tests_require and setup_requires (the main reasons are described in my previous comments #3253 (comment) and #3253 (comment)).

Can we restrict the change to install_requires and extras_require?

@akx
Copy link
Contributor Author

akx commented Jun 14, 2022

@abravalheri Okiedoke :) Removed the file: stuff related to tests_require and setup_requires (the code is still gently touched in setupcfg.py since parse_list_semicolon became a classmethod).

@akx akx requested a review from abravalheri June 14, 2022 11:55
@abravalheri
Copy link
Contributor

Thank you very much @akx and sorry for the trouble.
Since Jason seems to be OK with the change, I will try to merge the PRs yet this week.
(I am currently focusing on organising changes in the documentation and I had in the backlog the PEP 660, but I will try to make my best to not delay even more this merge).

docs/userguide/declarative_config.rst Outdated Show resolved Hide resolved
@abravalheri abravalheri merged commit 21d5b57 into pypa:main Jun 19, 2022
@abravalheri
Copy link
Contributor

Sorry for the delay @akx.

I merged the changes and published a new version of setuptools.
For the time being, I have added the status of beta, then we can see the feedback from the community.

@nedbat
Copy link

nedbat commented Jun 19, 2022

Thanks for this! :)

@pganssle
Copy link
Member

pganssle commented Jun 19, 2022

I'm still pretty negative on this "feature".

There really was no missing functionality, and this will almost certainly make the ecosystem as a whole worse off, as it encourages an anti-pattern. 🙁 It is basically a Norman Door.

I don't think we'll get feedback from the people affected by this because they are exactly the people who think it's a great feature because they don't know that they are doing anything wrong.

@henryiii
Copy link
Contributor

henryiii commented Nov 3, 2022

Just saw the first usage of this I've seen in the wild: https://github.com/Flexget/Flexget/blob/f55a297fae18e9779d9df9e545974727ad39b07a/requirements.txt :)

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.

Please allow "file:" for setup.cfg install_requires
6 participants