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

[Py3] Remove package containing entry points names with colons #3434

Merged
merged 6 commits into from Feb 2, 2016

Conversation

Projects
None yet
2 participants
@montefra
Contributor

montefra commented Jan 29, 2016

Entry points names containing colons are legit, or at least pkg_resources can deal with them without any issue.

Unfortunately, in python 3 pip uninstall choke on them when removing a package.
This is because ConfigParser supports by defaults also the "INI" format, in which the delimiter is a colon. This is not a problem for python 2.

This PR force the delimiter to be only = to fix this issue.

I haven't add a test as now. I should have to dig into the test suite (which is huge).
In theory would be enough to add an entry point somewhere called e.g.:

entry:point = path.to.package:function

If someone would be so kind as to point me to the correct place to add it, I would be very happy to add it and check if everything is fine.

Review on Reviewable

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Jan 31, 2016

Contributor

@montefra good catch.

For the test, maybe the simplest would be to write a functional test like this one:

def test_simple_uninstall_distutils(script):
"""
Test simple install and uninstall.
"""
script.scratch_path.join("distutils_install").mkdir()
pkg_path = script.scratch_path / 'distutils_install'
pkg_path.join("setup.py").write(textwrap.dedent("""
from distutils.core import setup
setup(
name='distutils-install',
version='0.1',
)
"""))
result = script.run('python', pkg_path / 'setup.py', 'install')
result = script.pip('list')
assert "distutils-install (0.1)" in result.stdout
script.pip('uninstall', 'distutils_install', '-y', expect_stderr=True)
result2 = script.pip('list')
assert "distutils-install (0.1)" not in result2.stdout

Where you can define the setup.py of your project with the problematic entry_point.

Contributor

xavfernandez commented Jan 31, 2016

@montefra good catch.

For the test, maybe the simplest would be to write a functional test like this one:

def test_simple_uninstall_distutils(script):
"""
Test simple install and uninstall.
"""
script.scratch_path.join("distutils_install").mkdir()
pkg_path = script.scratch_path / 'distutils_install'
pkg_path.join("setup.py").write(textwrap.dedent("""
from distutils.core import setup
setup(
name='distutils-install',
version='0.1',
)
"""))
result = script.run('python', pkg_path / 'setup.py', 'install')
result = script.pip('list')
assert "distutils-install (0.1)" in result.stdout
script.pip('uninstall', 'distutils_install', '-y', expect_stderr=True)
result2 = script.pip('list')
assert "distutils-install (0.1)" not in result2.stdout

Where you can define the setup.py of your project with the problematic entry_point.

@montefra

This comment has been minimized.

Show comment
Hide comment
@montefra

montefra Feb 1, 2016

Contributor

@xavfernandez: thank you for the pointer. I should commit the test soon.

Contributor

montefra commented Feb 1, 2016

@xavfernandez: thank you for the pointer. I should commit the test soon.

@montefra

This comment has been minimized.

Show comment
Hide comment
@montefra

montefra Feb 1, 2016

Contributor

I have added a test.
I've added pretend to the test_requires list to be able to run py.test.
Tox run through, except for py35:

  docs: commands succeeded
  packaging: commands succeeded
  pep8: commands succeeded
ERROR:   py3pep8: InterpreterNotFound: python3.3
ERROR:   py26: InterpreterNotFound: python2.6
  py27: commands succeeded
ERROR:   py33: InterpreterNotFound: python3.3
  py34: commands succeeded
ERROR:   py35: commands failed
ERROR:   pypy: InterpreterNotFound: pypy

Python 3.5 failed with an error which is probably fault of pytest e.g.:

ERROR collecting tests/functional/test_completion.py

.tox/py35/lib/python3.5/site-packages/py/_path/local.py:650: in pyimport
    __import__(modname)
<frozen importlib._bootstrap>:969: in _find_and_load
    ???
<frozen importlib._bootstrap>:954: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:892: in _find_spec
    ???
<frozen importlib._bootstrap>:873: in _find_spec_legacy
    ???
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:137: in find_module
    source_stat, co = _rewrite_test(state, fn_pypath)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:278: in _rewrite_test
    rewrite_asserts(tree)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:336: in rewrite_asserts
    AssertionRewriter().run(mod)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:557: in run
    new.extend(self.visit(child))
/usr/lib/python3.5/ast.py:245: in visit
    return visitor(node)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:665: in visit_Assert
    top_condition, explanation = self.visit(assert_.test)
/usr/lib/python3.5/ast.py:245: in visit
    return visitor(node)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:796: in visit_Compare
    left_res, left_expl = self.visit(comp.left)
/usr/lib/python3.5/ast.py:245: in visit
    return visitor(node)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:647: in generic_visit
    return res, self.explanation_param(self.display(res))
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:584: in display
    return self.helper("saferepr", expr)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:590: in helper
    return ast.Call(attr, list(args), [], None, None)
E   TypeError: Call constructor takes either 0 or 3 positional arguments
Contributor

montefra commented Feb 1, 2016

I have added a test.
I've added pretend to the test_requires list to be able to run py.test.
Tox run through, except for py35:

  docs: commands succeeded
  packaging: commands succeeded
  pep8: commands succeeded
ERROR:   py3pep8: InterpreterNotFound: python3.3
ERROR:   py26: InterpreterNotFound: python2.6
  py27: commands succeeded
ERROR:   py33: InterpreterNotFound: python3.3
  py34: commands succeeded
ERROR:   py35: commands failed
ERROR:   pypy: InterpreterNotFound: pypy

Python 3.5 failed with an error which is probably fault of pytest e.g.:

ERROR collecting tests/functional/test_completion.py

.tox/py35/lib/python3.5/site-packages/py/_path/local.py:650: in pyimport
    __import__(modname)
<frozen importlib._bootstrap>:969: in _find_and_load
    ???
<frozen importlib._bootstrap>:954: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:892: in _find_spec
    ???
<frozen importlib._bootstrap>:873: in _find_spec_legacy
    ???
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:137: in find_module
    source_stat, co = _rewrite_test(state, fn_pypath)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:278: in _rewrite_test
    rewrite_asserts(tree)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:336: in rewrite_asserts
    AssertionRewriter().run(mod)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:557: in run
    new.extend(self.visit(child))
/usr/lib/python3.5/ast.py:245: in visit
    return visitor(node)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:665: in visit_Assert
    top_condition, explanation = self.visit(assert_.test)
/usr/lib/python3.5/ast.py:245: in visit
    return visitor(node)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:796: in visit_Compare
    left_res, left_expl = self.visit(comp.left)
/usr/lib/python3.5/ast.py:245: in visit
    return visitor(node)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:647: in generic_visit
    return res, self.explanation_param(self.display(res))
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:584: in display
    return self.helper("saferepr", expr)
.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py:590: in helper
    return ast.Call(attr, list(args), [], None, None)
E   TypeError: Call constructor takes either 0 or 3 positional arguments
@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Feb 1, 2016

Contributor

Yes the pytest issue is known: it is pinned to avoid this issue pytest-dev/pytest#1083 but would need to be bumped to work on python 3.5 ...

Contributor

xavfernandez commented Feb 1, 2016

Yes the pytest issue is known: it is pinned to avoid this issue pytest-dev/pytest#1083 but would need to be bumped to work on python 3.5 ...

@montefra

This comment has been minimized.

Show comment
Hide comment
@montefra

montefra Feb 1, 2016

Contributor

Ok. Thank you for the pytest pointer.

Contributor

montefra commented Feb 1, 2016

Ok. Thank you for the pytest pointer.

@montefra

This comment has been minimized.

Show comment
Hide comment
@montefra

montefra Feb 1, 2016

Contributor

@xavfernandez : I've removed it. The test goes fine on my system.

If you accept the PR, do I have to do something else (changelog, other file, ...)?
Do you want me to rebase the branch?

Contributor

montefra commented Feb 1, 2016

@xavfernandez : I've removed it. The test goes fine on my system.

If you accept the PR, do I have to do something else (changelog, other file, ...)?
Do you want me to rebase the branch?

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Feb 1, 2016

Contributor

Well a changelog would make my life easier ^^
cf https://github.com/pypa/pip/blob/develop/CHANGES.txt

The rebase is optional since there is no conflict.

Contributor

xavfernandez commented Feb 1, 2016

Well a changelog would make my life easier ^^
cf https://github.com/pypa/pip/blob/develop/CHANGES.txt

The rebase is optional since there is no conflict.

@montefra

This comment has been minimized.

Show comment
Hide comment
@montefra

montefra Feb 1, 2016

Contributor

@xavfernandez : I'll add an entry under **8.1.0 (unreleased)** tomorrow.

If I see that tomorrow there are conflicts, I'll rebase.

Good night

Contributor

montefra commented Feb 1, 2016

@xavfernandez : I'll add an entry under **8.1.0 (unreleased)** tomorrow.

If I see that tomorrow there are conflicts, I'll rebase.

Good night

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Feb 1, 2016

Contributor

@montefra thanks :)

Contributor

xavfernandez commented Feb 1, 2016

@montefra thanks :)

@montefra

This comment has been minimized.

Show comment
Hide comment
@montefra

montefra Feb 2, 2016

Contributor

@xavfernandez : done.

I hope that everything is fine.

Contributor

montefra commented Feb 2, 2016

@xavfernandez : done.

I hope that everything is fine.

Show outdated Hide outdated pip/req/req_install.py Outdated
@montefra

This comment has been minimized.

Show comment
Hide comment
@montefra

montefra Feb 2, 2016

Contributor

@xavfernandez I'm about to rebase and push the commit

Contributor

montefra commented Feb 2, 2016

@xavfernandez I'm about to rebase and push the commit

@montefra

This comment has been minimized.

Show comment
Hide comment
@montefra

montefra Feb 2, 2016

Contributor

@xavfernandez : rebase done

Contributor

montefra commented Feb 2, 2016

@xavfernandez : rebase done

xavfernandez added a commit that referenced this pull request Feb 2, 2016

Merge pull request #3434 from montefra/fix_uninstall_colon
[Py3] Remove package containing entry points names with colons

@xavfernandez xavfernandez merged commit 3455016 into pypa:develop Feb 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Feb 2, 2016

Contributor

Thanks for your patience :)

Contributor

xavfernandez commented Feb 2, 2016

Thanks for your patience :)

@montefra

This comment has been minimized.

Show comment
Hide comment
@montefra

montefra Feb 2, 2016

Contributor

thank for accepting the PR and mostly thankd for pip.

Contributor

montefra commented Feb 2, 2016

thank for accepting the PR and mostly thankd for pip.

@montefra montefra deleted the montefra:fix_uninstall_colon branch Feb 2, 2016

@xavfernandez xavfernandez removed the needstest label Feb 8, 2016

@xavfernandez xavfernandez added this to the 8.0.3 milestone Feb 8, 2016

xavfernandez added a commit to xavfernandez/pip that referenced this pull request Feb 24, 2016

Merge pull request pypa#3434 from montefra/fix_uninstall_colon
[Py3] Remove package containing entry points names with colons
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment