-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
There was a problem hiding this 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.
Ah, sure thing! I can also take a look at getting that in here, if the approach seems otherwise sound by maintainers :) |
Thank you very much for considering that @akx. Yes, it is definitely good to wait for the feedback before making any change. |
I’m okay to give it a try. |
@abravalheri Hmm, by the looks of it enabling something like
(along the lines of setuptools/setuptools/tests/config/test_pyprojecttoml.py Lines 66 to 71 in e5552d3
would require changes to the JSON Schema for pyproject.toml, which... |
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 |
@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]_ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 viafile:
, 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.
@abravalheri @jaraco I rebased this to get rid of the merge conflicts. What are the next steps to get this merged..? |
Thank you very much @akx for rebasing the PR. Personally, I have some strong reservations against handling dynamic Can we restrict the change to |
@abravalheri Okiedoke :) Removed the |
Thank you very much @akx and sorry for the trouble. |
6e0b885
to
916ed27
Compare
Sorry for the delay @akx. I merged the changes and published a new version of setuptools. |
Thanks for this! :) |
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. |
Just saw the first usage of this I've seen in the wild: https://github.com/Flexget/Flexget/blob/f55a297fae18e9779d9df9e545974727ad39b07a/requirements.txt :) |
Summary of changes
As requested and discussed in #1951, this PR enables using
file:
inthe variousrequires
statementsinstall_requires
andextras_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
changelog.d/
.