setup.py: Conditional package dependencies are recorded unconditionally

[zvezdan]

This package uses Python code in setup.py to provide conditional dependencies instead of using metadata settings. On the line 18 in setup.py there is this code:

## Conditionally require the correct ipaddr package in Python2 vs Python3
if sys.version_info[0]<3:
    IPADDR = "ipaddr>=2.1.11"
    DNSPYTHON = "dnspython"
else:
    IPADDR = "ipaddress"
    DNSPYTHON = "dnspython3"

Thus, depending on the version of Python that the package is built with, some of the dependencies are never recorded.
Here's requires.txt from egg-info in the source distribution (sdist) package from PyPI:

ipaddr>=2.1.11
dnspython
colorama

Similarly, the METADATA file in the wheel released on PyPI has this:

...
Requires-Dist: ipaddr (>=2.1.11)
Requires-Dist: dnspython
Requires-Dist: colorama

As you can see in both requires.txt and METADATA files, dnspython3 is not recorded, nor are dnspython and ipaddr listed as conditional dependencies. They are both included unconditionally.
This breaks the installation of the sdist with Python 3 if the sdist is built with Python 2 and the build version should not matter.

Instead, the requires.txt file should have this content to be equivalent of the if statement above:

colorama

[:python_version<'3']
ipaddr>=2.1.11
dnspython

[:python_version>='3']
ipaddress
dnspython3

The wheel metadata should have:

...
Requires-Dist: ipaddr (>=2.1.11); python_version<'3'
Requires-Dist: dnspython; python_version<'3'
Requires-Dist: colorama
Requires-Dist: ipaddress; python_version>='3'
Requires-Dist: dnspython3; python_version>='3'

However, newer versions of dnspython support both Python 2 and 3. So, there's no need for that dependency to be conditional at all. I'm assuming here the full API compatibility and no code changes necessary. Also, ipaddress is in the standard library for Python 3.3+ and there's no need to make that dependency conditional either or have an external dependency at all. Python 3.0, 3.1, and 3.2 are not supported and there's no need to use a 3rd party dependency only for these versions. Also, ipaddress backport could be used conditionally instead of ipaddr with Python<3 to keep code compatibility simpler. Let's assume that this package still uses ipaddr, though, since the choice of that package is unrelated to conditional/unconditional dependency problem.

So, the requires.txt should really look like this:

colorama
dnspython

[:python_version<'3']
ipaddr>=2.1.11

To accomplish that, the setup.py file needs to be changed to replace the if statement on lines 19-24 with the following:

requires = ['colorama', 'dnspython']
extras_require = {
    ":python_version<'3'": ['ipaddr>=2.1.11'],
}

# Fall back to conditional code only for very old version of setuptools
if int(setuptools.__version__.split('.')[0]) < 18:
    extras_require = {}
    if sys.version_info[0] < 3:
        requires.append('ipaddr>=2.1.11')

and have this in the setup() call, replacing line 42:

install_requires=requires,       # Unconditional dependencies
extras_require=extras_require,  # Conditional dependencies

Of course, the fallback code could be omitted if we're sure we're going to build this package with setuptools 18.x or better. In that case we would just have in setup():

install_requires=['colorama', 'dnspython'],
extras_require={
    ":python_version<'3'": ['ipaddr>=2.1.11'],
},

If we're sure that we can use setuptools-36.x or better, then the new syntax can be used directly in the install_requires:

install_requires=[
    'colorama',
    'dnspython',
    "ipaddr>=2.1.11; python_version<'3'",
]

The other code that prepares requires and extras_require can be omitted in this case.

Please, see a similar issue here: https://gitlab.com/pycqa/flake8/issues/341 and the corresponding fix: https://gitlab.com/pycqa/flake8/merge_requests/191/diffs

Can we please get this fixed so that the sdist packages released to PyPI can be used by both Python 2 and 3 correctly?

[zvezdan]

I can make a PR if you want.

[mpenning]

Apologies for the delay... I was short on time when I tried the first time... I think it's nailed down now in 1.3.25... please confirm @zvezdan

[zvezdan]

It seems we worked at the same time on this and I didn't notice until I created a PR and it reported a conflict. Your change is good for the metadata definitely and preserves all the current dependencies in the same form.

I rebased my PR off your change and it just slightly optimized the dependencies + it fixes the syntax of the requirements.txt file for pip use. Let me know if I should delete this PR, or leave some of it's parts only (fix for the requirements.txt).

[mpenning]

This was a great bug; I am grateful for all the work you did documenting improvements to the build process. FYI, I was pulling my hair out to figure out why I couldn't build universal wheels circa Github issue #98... this fixed things so I'm able to build universal wheels again.

Please take a look at version 1.3.27 and ensure I haven't broken anything. I'll close this for now, but it's no problem if we need to re-open if there's something else wrong.