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

Remove svn handling in unpacking.unpack_file #7037

Merged
merged 4 commits into from Sep 19, 2019

Conversation

chrahunt
Copy link
Member

@chrahunt chrahunt commented Sep 19, 2019

There are two code paths that lead to unpacking.unpack_file:

  1. download.unpack_url -> {download.unpack_http_url, download.unpack_file_url} -> unpacking.unpack_file
  2. wheel.build -> download.unpack_file_url -> unpacking.unpack_file

In 1, we first check whether the provided link in a VCS URL and if so we call vcs_backend.unpack directly.

In 2, we only pass in the wheel file itself - no chance the removed code is called.

@chrahunt chrahunt added type: refactor skip news labels Sep 19, 2019
@chrahunt chrahunt force-pushed the refactor/clean-up-unpack branch from 70ed188 to 236bfac Compare Sep 19, 2019
@chrahunt
Copy link
Member Author

@chrahunt chrahunt commented Sep 19, 2019

Actually, I think this would be called when a URL mapped to an SVN repository, but an explicit svn+ was not provided. This is not documented anywhere, so I propose to remove the behavior and direct users to use an explicit prefix.

chrahunt added 4 commits Sep 19, 2019
`download.unpack_url` already calls `unpack_vcs_link`, it looks like
this was leftover from before that change.
@chrahunt chrahunt force-pushed the refactor/clean-up-unpack branch from ceffb0e to b6f7470 Compare Sep 19, 2019
@chrahunt chrahunt removed the skip news label Sep 19, 2019
Copy link
Member

@pradyunsg pradyunsg left a comment

Yay! Dead code.

@chrahunt chrahunt marked this pull request as ready for review Sep 19, 2019
@chrahunt chrahunt merged commit 79ec3de into pypa:master Sep 19, 2019
27 checks passed
@chrahunt chrahunt deleted the refactor/clean-up-unpack branch Sep 19, 2019
@xavfernandez
Copy link
Member

@xavfernandez xavfernandez commented Sep 22, 2019

Hmm, even if it wasn't documented, given the wide usage of pip, it's quite credible that some user somewhere was actually relying on this ^^
I would have advocated to go through the usual deprecation cycle (even for a short 3 months period).

Since it's already merged and the impacted use case is certainly quite rare and the workaround quite simple (adding the svn+ prefix), a revert seems overkill but we might want to document this workaround in the removal changelog ?

@chrahunt
Copy link
Member Author

@chrahunt chrahunt commented Sep 22, 2019

Yes that makes sense. Currently the removal wording is:

Remove undocumented support for http:// requirements pointing to SVN repositories.

How does this sound?

Remove undocumented support for un-prefixed URL requirements pointing to SVN repositories. Users relying on this can get the original behavior by prefixing their URL with svn+ (which is backwards-compatible).

@xavfernandez
Copy link
Member

@xavfernandez xavfernandez commented Sep 22, 2019

How does this sound?

Remove undocumented support for un-prefixed URL requirements pointing to SVN repositories. Users relying on this can get the original behavior by prefixing their URL with svn+ (which is backwards-compatible).

Pretty good 👍

@pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Sep 22, 2019

a revert seems overkill

Reverts are cheap. I'd say we shouldn't care whether a feature is in master but rather whether it's been released, when deciding about deprecation stuff.


This is super edge-casey but yea, I'm on board to do a single-release deprecation then removal. I don't think it's needed but I'm not gonna crib or anything, if we do have it. :)

@xavfernandez
Copy link
Member

@xavfernandez xavfernandez commented Sep 23, 2019

Reverts are cheap. I'd say we shouldn't care whether a feature is in master but rather whether it's been released, when deciding about deprecation stuff.

I completely agree but since other PRs have been merged that touch the same code area, it might not be (completely) trivial.

And since we all agree that the use case is pretty niche, is undocumented and has an easy and clean workaround, I don't think it is worth it so let's move forward :)

@lock lock bot added the auto-locked label Oct 23, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked type: refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants