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.

Sign up for GitHub

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

Jump to bottom

Always reinstall local distributions provided by the user #9147

Merged
merged 8 commits into from
Nov 26, 2020
Merged

Always reinstall local distributions provided by the user #9147

merged 8 commits into from
Nov 26, 2020

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Nov 19, 2020 •
edited
Loading

Toward #8711

@pradyunsg pradyunsg added type: enhancement Improvements to functionality !release blocker Hold a release until this is resolved skip news Does not need a NEWS file entry (eg: trivial changes) type: bugfix labels Nov 19, 2020
@pradyunsg pradyunsg added this to the 20.3 milestone Nov 19, 2020
pfmoore
pfmoore previously requested changes Nov 19, 2020
View reviewed changes
Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to think that we should explicitly not reinstall wheels even if they are local files. I'd like to see some justification for that.

I'm fine with the change as it applies to installs from source.

tests/functional/test_new_resolver.py Outdated Show resolved Hide resolved
@pradyunsg pradyunsg force-pushed the reinstall-local-distributions branch from c6a9490 to cdbb2ba Compare November 19, 2020 18:39
@pradyunsg pradyunsg dismissed pfmoore’s stale review November 20, 2020 13:50

Addressed these concerns, I think. :)

@pradyunsg pradyunsg requested a review from pfmoore November 20, 2020 13:50
@pradyunsg pradyunsg mentioned this pull request Nov 20, 2020
Closed
uranusjr
uranusjr reviewed Nov 20, 2020
View reviewed changes
src/pip/_internal/resolution/resolvelib/resolver.py
@@ -147,6 +152,29 @@ def resolve(self, root_reqs, check_supported_wheels):
ireq.should_reinstall = True
elif dist_is_editable(installed_dist) != candidate.is_editable:
ireq.should_reinstall = True
Copy link
Member

@uranusjr uranusjr Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related, but is this a bug? I thought editables are always reinstalled. (i.e. this should be true if dist_is_editable(installed_dist) == candidate.is_editable == True)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one's complained yet. And, we'd only hit this if someone tries to install an editable-over-already-installed-editable.

I'm fine with deferring this until someone complains.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd hate to be the one seen as complaining but reinstalling an already installed editable is a very common workflow, to make sure entrypoints are up-to-date. The fix looks easy enough. I'll look at writing a test for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#9116 seems to be along the same lines as well.

pfmoore
pfmoore approved these changes Nov 20, 2020
View reviewed changes
Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One comment in the code that needs correcting as it's the opposite of what's meant, and a couple of notes for clarification, but nothing that's a showstopper.

src/pip/_internal/resolution/resolvelib/resolver.py Outdated Show resolved Hide resolved
src/pip/_internal/resolution/resolvelib/resolver.py Show resolved Hide resolved
tests/functional/test_new_resolver.py Outdated Show resolved Hide resolved
@uranusjr uranusjr mentioned this pull request Nov 23, 2020
Open
@pradyunsg
Re-install local candidates unconditionally
ce46a5e 
Signed-off-by: Pradyun Gedam <[email protected]>
@pradyunsg
Ensure we're not mishandling local indexes
bb7fce7 
Signed-off-by: Pradyun Gedam <[email protected]>
@pradyunsg
Print a message and don't reinstall wheels
d6e3643 
Also, adds a test for source distributions being reinstalled.

Signed-off-by: Pradyun Gedam <[email protected]>
@pradyunsg
Factor out is_archive_file
cd15a85 
Signed-off-by: Pradyun Gedam <[email protected]>
@pradyunsg
Add deprecation warning when reinstalling sdists
9add1c1 
Signed-off-by: Pradyun Gedam <[email protected]>
@pradyunsg
Don't deprecate reinstalling from .zip files
5753884 
Signed-off-by: Pradyun Gedam <[email protected]>
@pradyunsg
Remove test for not reinstalling wheels, as requested
258242f 
Signed-off-by: Pradyun Gedam <[email protected]>
@pradyunsg
Drop incorrect line from comment
b2785d8 
This was a copy-paste error that I didn't catch earlier.

Signed-off-by: Pradyun Gedam <[email protected]>
@pradyunsg pradyunsg force-pushed the reinstall-local-distributions branch from b8c1752 to b2785d8 Compare November 25, 2020 14:21
@pradyunsg
Copy link
Member Author

Turns out... Our tests are not robust to us-east-1 going down. ;)

@pradyunsg
Copy link
Member Author

I'll make a separate PR for editables, with a test. Merging this! :)

@pradyunsg pradyunsg merged commit 254414b into pypa:master Nov 26, 2020
@uranusjr uranusjr mentioned this pull request Nov 28, 2020
Closed
@pradyunsg pradyunsg deleted the reinstall-local-distributions branch November 28, 2020 13:48
@pradyunsg pradyunsg mentioned this pull request Nov 28, 2020
Merged
@uranusjr uranusjr mentioned this pull request Jul 24, 2021
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pfmoore pfmoore pfmoore approved these changes

@sbidoul sbidoul sbidoul left review comments

@uranusjr uranusjr uranusjr left review comments

Assignees
No one assigned
Labels
!release blocker Hold a release until this is resolved skip news Does not need a NEWS file entry (eg: trivial changes) type: enhancement Improvements to functionality
Projects
None yet
Milestone
20.3
Development

Successfully merging this pull request may close these issues.
4 participants
@pradyunsg @uranusjr @sbidoul @pfmoore