agronholm / pythonfutures

Backport of the concurrent.futures package to Python 2.6 and 2.7
Other
232 stars 51 forks source link

Issue: __get_result incorrectly overwrites the correct version when installed on python 3. #78

Closed mpacer closed 6 years ago

mpacer commented 6 years ago

https://github.com/agronholm/pythonfutures/blob/5edbc65401fd578e195c2f0e2e8d3a6b5404f6bc/concurrent/futures/_base.py#L405-L416

I'm happy to make a PR to fix this, but I think the real solution is to release a new version that does not include the python_requires='>=2.6, <3', and raises an error instead of a warning when attempted to be installed on python 3.

For example, the monkey patching will break pytest if installed on python3 with version 3.1.1. That means if someone does pip3 install -U futures, that python3 executable would install a futures==3.1.1 because it doesn't have a python_requires='>=2.6, <3', which is what stops pip3 from even seeing that futures 3.2.0 exists.

If we were to raise an error in a way analogous to what we do in IPython: https://github.com/ipython/ipython/blob/01bd59ec7c184171df0cb0d933c5672e8c20b67e/setup.py#L25-L58:

if sys.version_info < (3, 4):
    pip_message = 'This may be due to an out of date pip. Make sure you have pip >= 9.0.1.'
    try:
        import pip
        pip_version = tuple([int(x) for x in pip.__version__.split('.')[:3]])
        if pip_version < (9, 0, 1) :
            pip_message = 'Your pip version is out of date, please install pip >= 9.0.1. '\
            'pip {} detected.'.format(pip.__version__)
        else:
            # pip is new enough - it must be something else
            pip_message = ''
    except Exception:
        pass

    error = """
IPython 7.0+ supports Python 3.4 and above.
When using Python 2.7, please install IPython 5.x LTS Long Term Support version.
Python 3.3 was supported up to IPython 6.x.
See IPython `README.rst` file for more information:
    https://github.com/ipython/ipython/blob/master/README.rst
Python {py} detected.
{pip}
""".format(py=sys.version_info, pip=pip_message )

    print(error, file=sys.stderr)
    sys.exit(1)

then pip install -U futures would successfully avoid breaking peoples' python3 executables by not allowing itself to be installed. I'll happiliy make a PR for this.

agronholm commented 6 years ago

I don't think it's reasonable for anyone to do pip3 install -U futures. Things will then go wrong and a reasonable user will learn then that they did not need to install futures via pip in the first place. What I could possibly do is delete all the previous releases (now that it's possible again).

mpacer commented 6 years ago

People will effectively be doing that in the context of building a container from scratch from a requirements file that happens to have had futures in it. I was just suggesting a new version because then non-pypi package indices will be more likely to be checking for new packages than they are likely to delete them. Especially in a production environment.

I was trying to give a simpler example that would cause the same problematic code path.

agronholm commented 6 years ago

This should not usually be a problem because pip installing futures will either result directly in a failure, or it will be ignored by Python as the standard library takes priority on import. I don't recall ever being able to reproduce a situation where I could install and import the backport on Python 3.

agronholm commented 6 years ago

At any rate, dropping the Python version requirement from setup.py is definitely the wrong approach.

mpacer commented 6 years ago

No I think you misunderstand me. I understand why that needs to be there in the long run — I was part of the IPython team that helped get the python_requires infrastructure in place. What I’m talking about is covered in my talk with @Carreau about this at Pycon 2017: https://youtu.be/2DkfPzWWC2Q

I intend it to continue to exist for one version to have the cancellation. The next version would have python_requires again. We explain in that talk why you need to do these other things too. I need to add one more commit to the __init__.py for the top level package. But in order to distribute this you have to have at least one version >3.1.1 that uses this erroring approach. If you’d prefer, we could do it as a back port version to 3.1.2 instead of 3.2.1. That way from a version perspective the python_requires will always be there for >=3.2. Would you be more comfortable with that?

agronholm commented 6 years ago

Yes, releasing a 3.1.2 would be perfect. That is something I was thinking about too. Would you mind creating a new issue for that? I'm closing this one as wontfix.