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

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

Closed
mpacer opened this issue Apr 27, 2018 · 6 comments
Labels

Comments

@mpacer
Copy link

mpacer commented Apr 27, 2018

def __get_result(self):
if self._exception:
if isinstance(self._exception, types.InstanceType):
# The exception is an instance of an old-style class, which
# means type(self._exception) returns types.ClassType instead
# of the exception's actual class type.
exception_type = self._exception.__class__
else:
exception_type = type(self._exception)
raise exception_type, self._exception, self._traceback
else:
return self._result

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
Copy link
Owner

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
Copy link
Author

mpacer commented Apr 27, 2018

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
Copy link
Owner

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
Copy link
Owner

agronholm commented Apr 27, 2018

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

@mpacer
Copy link
Author

mpacer commented Apr 27, 2018

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants