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

Duplicate distributions with distinct extras #3198

Merged
merged 1 commit into from Nov 23, 2015

Conversation

Projects
None yet
5 participants
@s-t-e-v-e-n-k
Contributor

s-t-e-v-e-n-k commented Oct 21, 2015

Any duplicate distributions will currently raise an error, even if
their extras are different: for example, 'pip install bar bar[foo]'.
Detect this situation, and union the extras together.

Closes #3189

Review on Reviewable

@nakato

This comment has been minimized.

Show comment
Hide comment
@nakato
Contributor

nakato commented Oct 29, 2015

@rbtcollins

This comment has been minimized.

Show comment
Hide comment
@rbtcollins

rbtcollins Nov 4, 2015

Contributor

I think this also needs the tests from #3105

Contributor

rbtcollins commented Nov 4, 2015

I think this also needs the tests from #3105

@rbtcollins

This comment has been minimized.

Show comment
Hide comment
@rbtcollins

rbtcollins Nov 4, 2015

Contributor

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


tests/data/packages/LocalExtras/setup.py, line 27 [r1] (raw file):
Test review please ignore.


Comments from the review on Reviewable.io

Contributor

rbtcollins commented Nov 4, 2015

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


tests/data/packages/LocalExtras/setup.py, line 27 [r1] (raw file):
Test review please ignore.


Comments from the review on Reviewable.io

@rbtcollins

This comment has been minimized.

Show comment
Hide comment
@rbtcollins

rbtcollins Nov 20, 2015

Contributor

Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


pip/req/req_set.py, line 241 [r1] (raw file):
This if says 'if user supplied and we have it already and it is not a constraint' - and then we do the check for differing extras. I think perhaps it would be better to just enhance the guard here rather than having duplicate extra handling code:
if (parent_req_name is None # user supplied
and existing_req # we already have it
and not existing_req.constraint # but not a constraint
and existing_req.extras == install_req.extras): # and the extras are the same
... error


pip/req/req_set.py, line 251 [r2] (raw file):
We should perhaps sort() that rather than taking unordered output.


pip/req/req_set.py, line 281 [r2] (raw file):
And here.


tests/functional/test_install_reqs.py, line 356 [r2] (raw file):
So this tests by-location, extras in the constraints. The full matrix is...

findstyle in (location, name)
extrastyle in (nothing, [a], [b], [c])
find-style-in-constraints x find-style in install command x extras-style-in-constraints x extras-style-in-install x extras-style-in-deplist

2 x 2 x 4 x 4 x 4 combinations. Some can be trimmed - (e.g. we only need one case where all three places extras can turn up with them set to the same value), so the extras-style really is more like (none, unique-to-position, same-as-another position).

A bit of though should get a sane set of nested loops to express this, or perhaps you can use py.tests's built in scenarios support (I'm not sure if its up to this or not - but if a loop can express it it probably can.

I'm not too worried about editable here, since editable doesn't change process handling, its location vs index lookups that can do that because of the late binding of metadata.

This suggestion may be overkill of course; it all feeds through add_requirement, so lets look at it from that angle: the more conditionals it has, we need to feed into each one.

The first conditional is

if not name:
            # url or path requirement w/o an egg fragment
            self.unnamed_requirements.append(install_req)
            return [install_req]

Thats clearly going to drop stuff on pip install . .[foo]

Then we've got the bit already reviewed you added, and Sachi's, respectively, adding extras to a non-extra existing dep and adding extras to a constraint existing dep.

I think thats sufficient, and since I know you have tests that cover both those cases, I'm happy to ignore the unnamed case since thats already broken in myriad ways; we can overhaul it separately.

I don't see the expected failure that @xavfernandez asked we add though:

given package p with where adding in extras makes the install unsatisfiable, have an error occur.

e.g.conflictingextras depends on simple==1.0, but conflictextras[bad] depends on simple==2.0

pip install conflictingextras conflictextras[bad]
today with your patch will install simple==1.0, in future with the resolver it should error.


Comments from the review on Reviewable.io

Contributor

rbtcollins commented Nov 20, 2015

Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


pip/req/req_set.py, line 241 [r1] (raw file):
This if says 'if user supplied and we have it already and it is not a constraint' - and then we do the check for differing extras. I think perhaps it would be better to just enhance the guard here rather than having duplicate extra handling code:
if (parent_req_name is None # user supplied
and existing_req # we already have it
and not existing_req.constraint # but not a constraint
and existing_req.extras == install_req.extras): # and the extras are the same
... error


pip/req/req_set.py, line 251 [r2] (raw file):
We should perhaps sort() that rather than taking unordered output.


pip/req/req_set.py, line 281 [r2] (raw file):
And here.


tests/functional/test_install_reqs.py, line 356 [r2] (raw file):
So this tests by-location, extras in the constraints. The full matrix is...

findstyle in (location, name)
extrastyle in (nothing, [a], [b], [c])
find-style-in-constraints x find-style in install command x extras-style-in-constraints x extras-style-in-install x extras-style-in-deplist

2 x 2 x 4 x 4 x 4 combinations. Some can be trimmed - (e.g. we only need one case where all three places extras can turn up with them set to the same value), so the extras-style really is more like (none, unique-to-position, same-as-another position).

A bit of though should get a sane set of nested loops to express this, or perhaps you can use py.tests's built in scenarios support (I'm not sure if its up to this or not - but if a loop can express it it probably can.

I'm not too worried about editable here, since editable doesn't change process handling, its location vs index lookups that can do that because of the late binding of metadata.

This suggestion may be overkill of course; it all feeds through add_requirement, so lets look at it from that angle: the more conditionals it has, we need to feed into each one.

The first conditional is

if not name:
            # url or path requirement w/o an egg fragment
            self.unnamed_requirements.append(install_req)
            return [install_req]

Thats clearly going to drop stuff on pip install . .[foo]

Then we've got the bit already reviewed you added, and Sachi's, respectively, adding extras to a non-extra existing dep and adding extras to a constraint existing dep.

I think thats sufficient, and since I know you have tests that cover both those cases, I'm happy to ignore the unnamed case since thats already broken in myriad ways; we can overhaul it separately.

I don't see the expected failure that @xavfernandez asked we add though:

given package p with where adding in extras makes the install unsatisfiable, have an error occur.

e.g.conflictingextras depends on simple==1.0, but conflictextras[bad] depends on simple==2.0

pip install conflictingextras conflictextras[bad]
today with your patch will install simple==1.0, in future with the resolver it should error.


Comments from the review on Reviewable.io

@rbtcollins

This comment has been minimized.

Show comment
Hide comment
@rbtcollins

rbtcollins Nov 20, 2015

Contributor

Review status: 3 of 6 files reviewed at latest revision, 6 unresolved discussions.


tests/functional/test_install_reqs.py, line 431 [r3] (raw file):
I think this needs a comment explaining why success on a conflict is an error :)


Comments from the review on Reviewable.io

Contributor

rbtcollins commented Nov 20, 2015

Review status: 3 of 6 files reviewed at latest revision, 6 unresolved discussions.


tests/functional/test_install_reqs.py, line 431 [r3] (raw file):
I think this needs a comment explaining why success on a conflict is an error :)


Comments from the review on Reviewable.io

Join constraints and requested extras
Compare extras when checking if a requirement has already been
specified, and take a union of the extras before installation.

Co-Authored-By: Sachi King <nakato@nakato.io>
Closes #3046, #3189
@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Nov 20, 2015

Contributor

@rbtcollins: it depends whether parent_req_name is None or not.

With top-level requirement (directly provided by the user or from a requirement file) pip always honors them or crashes in case of double-requirement.
With this change one of them could be silently overridden due to the presence of an extra...

pip install requests==2.7 requests[security]==2.8 would work while it previously crashed.

Contributor

xavfernandez commented Nov 20, 2015

@rbtcollins: it depends whether parent_req_name is None or not.

With top-level requirement (directly provided by the user or from a requirement file) pip always honors them or crashes in case of double-requirement.
With this change one of them could be silently overridden due to the presence of an extra...

pip install requests==2.7 requests[security]==2.8 would work while it previously crashed.

@rbtcollins

This comment has been minimized.

Show comment
Hide comment
@rbtcollins

rbtcollins Nov 20, 2015

Contributor

@xavfernandez pip is inconsistent on that today - consider "pip install . ."

I think this is a sufficiently useful thing that the risk is worth taking. Without it you cannot have e.g. test dependencies in an extra and then install_require on foo, and also have e.g. in dev-requirements.txt foo[testsupport].

The resolver is just around the corner where we'll stop giving duplicate requirement errors altogether...

Contributor

rbtcollins commented Nov 20, 2015

@xavfernandez pip is inconsistent on that today - consider "pip install . ."

I think this is a sufficiently useful thing that the risk is worth taking. Without it you cannot have e.g. test dependencies in an extra and then install_require on foo, and also have e.g. in dev-requirements.txt foo[testsupport].

The resolver is just around the corner where we'll stop giving duplicate requirement errors altogether...

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Nov 20, 2015

Contributor

@rbtcollins point taken ☺
Maybe an update of the debug message 'logger.debug("Setting %s extras to: %s")' to better explain the merge would be a good idea then...

Contributor

xavfernandez commented Nov 20, 2015

@rbtcollins point taken ☺
Maybe an update of the debug message 'logger.debug("Setting %s extras to: %s")' to better explain the merge would be a good idea then...

@rbtcollins

This comment has been minimized.

Show comment
Hide comment
@rbtcollins

rbtcollins Nov 22, 2015

Contributor

I think perhaps a comment, rather than the message changing?

Contributor

rbtcollins commented Nov 22, 2015

I think perhaps a comment, rather than the message changing?

@rbtcollins

This comment has been minimized.

Show comment
Hide comment
@rbtcollins

rbtcollins Nov 23, 2015

Contributor

So, looks good to me.


Reviewed 2 of 3 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


Comments from the review on Reviewable.io

Contributor

rbtcollins commented Nov 23, 2015

So, looks good to me.


Reviewed 2 of 3 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


Comments from the review on Reviewable.io

dstufft added a commit that referenced this pull request Nov 23, 2015

Merge pull request #3198 from s-t-e-v-e-n-k/issue3189
Duplicate distributions with distinct extras

@dstufft dstufft merged commit ef73f43 into pypa:develop Nov 23, 2015

1 of 2 checks passed

code-review/reviewable Review in progress: all files reviewed, 5 unresolved discussions
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment