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

Un-deprecate source distribution re-installation behaviour #10543

Merged
merged 1 commit into from
Oct 9, 2021

Conversation

pradyunsg
Copy link
Member

This was deprecated in favour of explicitly passing --force-reinstall.

@pradyunsg pradyunsg added the !release blocker Hold a release until this is resolved label Oct 4, 2021
@pradyunsg pradyunsg added this to the 21.3 milestone Oct 4, 2021
issue=8711,
# is a local sdist -- disallow.
raise SamePackageAlreadyInstalled(
refused_to_install=candidate.source_link.file_path
Copy link
Member

Choose a reason for hiding this comment

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

Should that really be an error ? Or just a Requirement already satisfied: ... message ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think an error is better, because otherwise this would be a "silent" behaviour change -- the behaviour changed but the user has no way to know how/why; especially if they don't have checks to ensure that the behaviour doesn't affect them. Given that the deprecation message explicitly advertised a replacement, I think it's a good idea to fail loudly in these cases.

@pradyunsg
Copy link
Member Author

@pypa/pip-committers Re-reading #8711 and #5780 (comment), I do feel a bit confused about this behaviour change.

From the feedback we've gotten since the deprecation was announced, it's clear that using --force-reinstall isn't a good-enough replacement -- that behaviour is not sufficient for the use cases that users have mentioned. It does look like we'll bring in another round of behaviour changes whenever #5780 gets resolved too.

I'm not a 100% sure what to do here TBH. I'm inclined to pull out this deprecation, and then kick the can down the road for this, into #5780 -- since it does look like we want pip install foo.tar.gz to continue reinstalling and pip install https://example.com/foo.tar.gz to reinstall in some situations. There's a bunch of behaviour changes planned in that issue, and I'd much prefer we do them as a coherent effort.

@uranusjr
Copy link
Member

uranusjr commented Oct 8, 2021

since it does look like we want pip install foo.tar.gz to continue reinstalling and pip install https://example.com/foo.tar.gz to reinstall in some situations. There's a bunch of behaviour changes planned in that issue, and I'd much prefer we do them as a coherent effort.

+1 This needs to be done in one shot (which is most of the reason why I haven't touched it one bit)

This will be handled as part of a broader effort to make the reinstall
behaviours more ergonomic and easier to reason about.
@pradyunsg pradyunsg changed the title Drop sdist re-installation behaviour Un-deprecate source distribution re-installation behaviour Oct 8, 2021
@pradyunsg
Copy link
Member Author

Alrighty. Here's a patch removing the deprecation then!

I'm hoping that this doesn't fail CI in some weird way. :)

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Tests failed due network issues. Unrelated.

@pradyunsg pradyunsg merged commit ae2c9ed into pypa:main Oct 9, 2021
@pradyunsg pradyunsg deleted the drop-sdist-reinstall branch October 9, 2021 06:37
@sbidoul
Copy link
Member

sbidoul commented Oct 10, 2021

@pradyunsg @uranusjr sorry to be late to the game but, is it not better to keep this deprecation ?

IMO, "Reinstalling a source distribution despite an installed package having the same name and version as the installed package" is stil a behaviour that is dubious and we will probably want to change in the future. So continuing to warn users that they are doing something unusual seems like a good thing to do ?

@uranusjr
Copy link
Member

I think “reinstalling a (direct URL; the warning did not happen for version-specified requirements) source distribution despite an installed package having the same name and version” in itself isn’t actually dubious on paper—in fact, it’s exactly what I’d want if I pip install foo==1.0 and then pip install foo @ file://.../foo-1.0.tar.gz. The behaviour I wouldn't expect is running pip install foo @ file://.../foo-1.0.tar.gz again still triggers a reinstallation. So to me, the deprecation message is, at best, inaccurate and covers too many false positives, so it’s better to do without it now, and try again in a future version when we can actually figure out what exactly we want to deprecate.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
!release blocker Hold a release until this is resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants