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

Explicit support and tests for hg+file scheme for pip install. #5955

Closed
wants to merge 3 commits into from

Conversation

atse
Copy link
Contributor

@atse atse commented Oct 28, 2018

Reworking #5935 to be more complete for #4358

The hg+file scheme was already functioning implicitly (with test coverage) but inconsistently documented. This change is to make the scheme support explicit, including tests and docs.

@pradyunsg
Copy link
Member

The linting jobs don't seem to be happy with these changes. :)

news/4358.bugfix Outdated Show resolved Hide resolved
@cjerdonek
Copy link
Member

The hg+file scheme was already functioning implicitly (with test coverage)

How could hg+file have been working if it wasn't listed explicitly in the supported schemes? That seems like something important to understand and track down first, as it could signal a bug or reveal other things..

@cjerdonek cjerdonek added type: enhancement Improvements to functionality C: vcs pip's interaction with version control systems like git, svn and bzr labels Oct 30, 2018
@chrahunt
Copy link
Member

chrahunt commented Jul 8, 2019

I only see us using these schemes in a handful of places and it looks like, for editable installs, we split base on the + and then leave everything else to the backend (in InstallRequirement.update_editable).

To confirm, I ran pip under hunter like

PYTHONHASHSEED=0 PYTHONHUNTER="stdlib=False, action=CallPrinter(stream=open(f'$PWD/hunter-old-{os.getpid()}.log', 'w'))" pip install -e hg+file://$PWD/hg_repo#egg=foo

before and after the change in this PR and can see virtually no difference in behavior between the two runs.

pkg_path = _create_test_package(script, name='testpackage', vcs='hg')
args = ['install', '-e', 'hg+%s#egg=testpackage' % path_to_url(pkg_path)]
result = script.pip(*args, **{"expect_error": True})
result.assert_installed('testpackage', with_files=['.hg'])
assert path_to_url(pkg_path).startswith("file://")
Copy link
Member

Choose a reason for hiding this comment

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

I would save this into a variable and use it in the construction of args and in the assert. It may also make more sense to move the assert above the args declaration. Normally we assert as close as possible to the place where something could've failed.

@@ -17,7 +17,9 @@ class Mercurial(VersionControl):
name = 'hg'
dirname = '.hg'
repo_name = 'clone'
schemes = ('hg', 'hg+http', 'hg+https', 'hg+ssh', 'hg+static-http')
schemes = (
'hg', 'hg+http', 'hg+https', 'hg+ssh', 'hg+static-http', 'hg+file'
Copy link
Member

Choose a reason for hiding this comment

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

It looks like these where alphabetized before.

@cjerdonek
Copy link
Member

I still don't see an answer to the question I asked before. If this was already working before (without hg+file in the schemes attribute), why do we need to add it to the list of schemes -- what behavior changes by doing that? If there is no change in behavior, it seems like it shouldn't be added because it gives a false impression that it's doing something. And if there is a change, it seems like that's one of the things that should be tested (e.g. to prevent a regression).

@chrahunt
Copy link
Member

This should be added so that Link.is_vcs returns the expected value on hg+file URLs. I was about to use it as part of some refactoring and was reminded of this PR. We should add a test covering that case here.

It currently works because we are pretty flexible when it comes to input URL parsing. In several places we just check if "+" is in the scheme to determine if something is a VCS URL, or split on + to get the VCS name and non-VCS scheme.

@chrahunt
Copy link
Member

chrahunt commented Oct 6, 2019

Thanks again for your work here @atse! I will close this since it has been superseded by #7065.

@chrahunt chrahunt closed this Oct 6, 2019
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Nov 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: vcs pip's interaction with version control systems like git, svn and bzr type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants