-
-
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
Support the empty-string extra #1503
Conversation
This will be super useful, but I think this should probably be documented explicitly. |
I'd rather see this as part of a PEP update. |
This does not work, generated wheels don't have the correct metadata (because an empty extra is not possible): > cat setup.py
from setuptools import setup
setup(
name='myproject', version='1.0',
install_requires='python-xlib',
extras_require={
'aiohttp': '''
aiohttp
''',
'tornado': '''
tornado
''',
'empty': '''
''',
'': '''
requests
''',
}
)
> python setup.py -q bdist_wheel && unzip -p dist/myproject-1.0-py3-none-any.whl '*/METADATA'
Metadata-Version: 2.1
Name: myproject
Version: 1.0
Summary: UNKNOWN
Home-page: UNKNOWN
License: UNKNOWN
Platform: UNKNOWN
Provides-Extra: aiohttp
Provides-Extra: empty
Provides-Extra: tornado
Requires-Dist: python-xlib
Requires-Dist: requests
Provides-Extra: aiohttp
Requires-Dist: aiohttp; extra == 'aiohttp'
Provides-Extra: empty
Provides-Extra: tornado
Requires-Dist: tornado; extra == 'tornado'
UNKNOWN Also, how would you tell pip to install only the core dependencies? Apart from the project declaring an explicit empty "no_frills" extra. |
Are you suggesting an update to PEP 508? If so I think I agree, the specification around extras could stand to be defined a little more strictly (e.g., it doesn't currently specify what makes a valid or invalid extra) and doesn't say anything about this edge case.
Yes, we'd need to make an update to from setuptools import setup
setup(
name='foo',
version='1.0',
extras_require={
'aiohttp': 'aiohttp',
'tornado': 'tornado',
'': 'requests',
},
) It would generate this metadata instead: Metadata-Version: 2.1
Name: foo
Version: 1.0
Summary: UNKNOWN
Home-page: UNKNOWN
License: UNKNOWN
Platform: UNKNOWN
- Requires-Dist: requests
+ Provides-Extra:
+ Requires-Dist: requests; extra == ''
Provides-Extra: aiohttp
Requires-Dist: aiohttp; extra == 'aiohttp'
Provides-Extra: tornado
Requires-Dist: tornado; extra == 'tornado'
UNKNOWN At least as far as wheel's metadata format goes, I think an empty extra is possible: >>> from email.parser import Parser
>>> from io import StringIO
>>> Parser().parse(StringIO('Provides-Extra:')).get('Provides-Extra')
'' The
it's just a matter of properly propagating it through the metadata, and defining the behavior in various bits of tooling.
You wouldn't, presumably any package specifying an "empty string extra" would intend for those requirements to be a part of the core dependencies |
How do you handle this case: extras_require={
';"linux" in sys_platform': ['package'],
}
''' |
I think currently that extra would turn into something like:
Which means that it's equivalent to putting it in
Which means that it would only be installed for the "empty string extra", and wouldn't be installed if the user specified a different extra. So technically the behavior would change, however, if the maintainer actually wanted that dependency to be installed across all possible extras, why not just put it in |
Here's the necessary changes to |
I am loathe to suggest this, because I don't feel like mailing list discussions are always super productive, but should we kick this over to distutils-SIG, or into a canonical issue somewhere? FWIW, my use case for this is actually that I would like to implement "negative dependencies", so I'm wondering if we can solve the more general problem by allowing extras to declare a replacement either for a package or a set of packages, e.g.: from setuptools import setup
setup(
name='foo',
version='1.0',
install_requires=['requests'],
extras_require={
'aiohttp': {'requests': 'aiohttp'},
'tornado': {'requests': 'tornado'}
},
) This would allow me to declare: extras_require={
'no-tzdata': {'dateutil.tzdata': ''}
} Note: This is not a concrete proposal for an API, I can already see some major objections to what I just suggested, I'm just thinking that it may be time to draft up a PEP (or whatever the current process ends up being) for discussion on this API. |
@pganssle I don't see why what I'm proposing wouldn't work as a negative dependency: setup(
name='foo',
version='1.0',
install_requires=[...] # Doesn't include dateutil.tzdata
extras_require={
'': ['dateutil.tzdata'],
'no-tzdata': [], # Maybe this has a replacement for dateutil.tzdata?
},
) IMO, this is cleaner than being able to declare replacements (and would be a lot easier to implement, as the "empty string extra" pretty much fits into the existing way that extras are implemented with minimal changes).
If I'm going to be making a PEP update to support this, I'll raise the changes to distutils-sig with the draft. I agree that the current "correct" behavior here is more or less undefined and could stand to be specified (even if it's "empty string extras are invalid"). |
Requirements with environment markers do end up in |
I see, so what you're saying is that: from setuptools import setup
setup(
name='foo',
version='1.0',
install_requires=[
'package_a;"linux" in sys_platform',
'package_b',
],
) will produce a
And this would create a breaking change for anyone who specified extras, since I would have expected it to produce a
As far as I can tell, that was the original behavior (and in fact, It seems like you added this in #1081 because the conditionals were getting stripped when building a wheel. Why didn't that get fixed downstream in |
The format of |
The original change (allowing environment markers unfiltered in |
Yes, I'm aware.
Would you mind going into more detail here? How does "moving" conditional requirements into extras fix this? |
Because
is the proper format? |
install_requires=['package_a; "linux" in sys_platform'], extras_require={':"linux" in sys_platform': ['package_a']}, and extras_require={'': ['package_a; "linux" in sys_platform']}, are all equivalent. They all result in the same wheel metadata. |
Sure, what I'm trying to say is that it seems to me like they shouldn't be equivalent, at least it's not what I would have expected before trying it myself. I would have expected that using
and would have expected that using
There seems to be some suprise/confusion about this elsewhere as well, such as here: pex-tool/pex#545 (comment) I haven't ever seen a specification for the |
There's no formal specification for |
OK, so aside from the fact that (BTW, thanks for all your responses so far!) |
There has been 3 behaviours:
In any case, I think the fact that your proposed patch would change the behaviour of some existing requirements is reason enough to not do it like that. I think ideally you'd want an additional metadata field, specifying the default list of extras. |
Really, I think the fact that pip supported 2 was just an happy accident (using the same standard API to parse requirements). |
I agree, and I'm definitely not trying to introduce a breaking change here. It's unfortunate that this is the current status quo, because otherwise this would have been a pretty clean change to apply. There might be a way to understand the difference between packages produced before this change and after this change. Ideally, a package using the empty-string-extra would say that it provides the empty string as an extra:
This isn't currently the behavior of |
@di For most use cases, it would probably be fine as a negative dependency (which is why it was initially exciting to me), but it kinda falls down when you have more than one extra. Sticking with the real-life example of
In the end it may be that using "negative dependencies" like this ends up being not as helpful as one would hope, because by default if you have any other dependencies that depend on I think the main thing I wanted to mention here was that this is a nice hack, but once you have more than one of these things where you can replace the default backend, you're back to being stuck. That said, just because the solution doesn't generalize doesn't mean we shouldn't implement it, assuming it's not hurting anything to have it there. |
Actually, now that I think about it, I realized that this may in fact generalize better than I thought if you just add an opt-in flag for the stuff that is initially opt-out, so: setup(
name='foo',
version='1.0',
install_requires=[...] # Doesn't include dateutil.tzdata
extras_require={
'': ['dateutil.tzdata'],
'tzdata': ['dateutil.tzdata'],
'no-tzdata': [], # Maybe this has a replacement for dateutil.tzdata?
'rust-backend': ['dateutil.backend.rust'],
},
) This is not ideal, but at least it makes the combinatorial logic I wanted possible, with:
|
Unfortunately I don't see a way this approach is going to work given the current state of things, without breaking backwards compatibility. As such I'm going to close this PR. Thanks for the discussion @benoit-pierre and @pganssle! |
Thanks for trying @di. It's a lot of work to manage a world-wide piece of software. So thanks for trying to improve it. And thanks to @pganssle for taking the time to examine the pros and cons. This discussion has been a useful reference for me. Do you think https://speakerdeck.com/uranusjr/lets-fix-extras-in-core-metadata-3-dot-0 will be able to address what you were hoping to address here? |
Summary of changes
Allows users to specify an "empty string extra", whose dependencies which will be included in a package's dependencies only if no extras have been requested (i.e., by default).
Closes #1139.
Pull Request Checklist