From 701d010c0298f3ef2cc003d5f0957c4924c2e5ae Mon Sep 17 00:00:00 2001 From: Chris Jerdonek Date: Sun, 8 Jul 2018 04:21:00 -0700 Subject: [PATCH 1/2] Implement VersionControl.obtain() in the base class. --- ...450A0C-9B6C-4AAB-8171-AB92935AE2CA.trivial | 0 src/pip/_internal/vcs/__init__.py | 56 +++++++++++-------- src/pip/_internal/vcs/bazaar.py | 6 -- src/pip/_internal/vcs/git.py | 6 -- src/pip/_internal/vcs/mercurial.py | 6 -- src/pip/_internal/vcs/subversion.py | 24 ++++---- tests/unit/test_vcs.py | 18 ++++++ 7 files changed, 63 insertions(+), 53 deletions(-) create mode 100644 news/03450A0C-9B6C-4AAB-8171-AB92935AE2CA.trivial diff --git a/news/03450A0C-9B6C-4AAB-8171-AB92935AE2CA.trivial b/news/03450A0C-9B6C-4AAB-8171-AB92935AE2CA.trivial new file mode 100644 index 000000000..e69de29bb diff --git a/src/pip/_internal/vcs/__init__.py b/src/pip/_internal/vcs/__init__.py index 97d32859b..08bd71fa8 100644 --- a/src/pip/_internal/vcs/__init__.py +++ b/src/pip/_internal/vcs/__init__.py @@ -232,6 +232,24 @@ class VersionControl(object): url = urllib_parse.urlunsplit((scheme, netloc, path, query, '')) return url, rev + def get_url_rev_args(self, url): + """ + Return the URL and RevOptions "extra arguments" to use in obtain(), + as a tuple (url, extra_args). + """ + return url, [] + + def get_url_rev_options(self): + """ + Return the URL and RevOptions object to use in obtain(), as a tuple + (url, rev_options). + """ + url, rev = self.get_url_rev() + url, extra_args = self.get_url_rev_args(url) + rev_options = self.make_rev_options(rev, extra_args=extra_args) + + return url, rev_options + def get_info(self, location): """ Returns (url, revision), where both are strings @@ -253,13 +271,6 @@ class VersionControl(object): """ return (self.normalize_url(url1) == self.normalize_url(url2)) - def obtain(self, dest): - """ - Called when installing or updating an editable package, takes the - source path of the checkout. - """ - raise NotImplementedError - def fetch_new(self, dest, url, rev_options): """ Fetch a revision from a repository, in the case that this is the @@ -299,18 +310,19 @@ class VersionControl(object): """ raise NotImplementedError - def check_destination(self, dest, url, rev_options): + def obtain(self, dest): """ - Prepare a location to receive a checkout/clone. - - Return True if the location is ready for (and requires) a - checkout/clone, False otherwise. + Install or update in editable mode the package represented by this + VersionControl object. Args: - rev_options: a RevOptions object. + dest: the repository directory in which to install or update. """ + url, rev_options = self.get_url_rev_options() + if not os.path.exists(dest): - return True + self.fetch_new(dest, url, rev_options) + return rev_display = rev_options.to_display() if os.path.exists(os.path.join(dest, self.dirname)): @@ -332,7 +344,7 @@ class VersionControl(object): self.update(dest, rev_options) else: logger.info('Skipping because already up-to-date.') - return False + return logger.warning( '%s %s in %s exists with URL %s', @@ -365,7 +377,8 @@ class VersionControl(object): if response == 'w': logger.warning('Deleting %s', display_path(dest)) rmtree(dest) - return True + self.fetch_new(dest, url, rev_options) + return if response == 'b': dest_dir = backup_dir(dest) @@ -373,8 +386,10 @@ class VersionControl(object): 'Backing up %s to %s', display_path(dest), dest_dir, ) shutil.move(dest, dest_dir) - return True + self.fetch_new(dest, url, rev_options) + return + # Do nothing if the response is "i". if response == 's': logger.info( 'Switching %s %s to %s%s', @@ -385,10 +400,6 @@ class VersionControl(object): ) self.switch(dest, url, rev_options) - # Do nothing if the response is "i". - - return False - def unpack(self, location): """ Clean up current location and download the url repository @@ -410,7 +421,8 @@ class VersionControl(object): def get_url(self, location): """ Return the url used at location - Used in get_info or check_destination + + This is used in get_info() and obtain(). """ raise NotImplementedError diff --git a/src/pip/_internal/vcs/bazaar.py b/src/pip/_internal/vcs/bazaar.py index 71553533e..80c14903f 100644 --- a/src/pip/_internal/vcs/bazaar.py +++ b/src/pip/_internal/vcs/bazaar.py @@ -66,12 +66,6 @@ class Bazaar(VersionControl): cmd_args = ['pull', '-q'] + rev_options.to_args() self.run_command(cmd_args, cwd=dest) - def obtain(self, dest): - url, rev = self.get_url_rev() - rev_options = self.make_rev_options(rev) - if self.check_destination(dest, url, rev_options): - self.fetch_new(dest, url, rev_options) - def get_url_rev(self): # hotfix the URL scheme after removing bzr+ from bzr+ssh:// readd it url, rev = super(Bazaar, self).get_url_rev() diff --git a/src/pip/_internal/vcs/git.py b/src/pip/_internal/vcs/git.py index b2d6e1439..6bda129a8 100644 --- a/src/pip/_internal/vcs/git.py +++ b/src/pip/_internal/vcs/git.py @@ -203,12 +203,6 @@ class Git(VersionControl): #: update submodules self.update_submodules(dest) - def obtain(self, dest): - url, rev = self.get_url_rev() - rev_options = self.make_rev_options(rev) - if self.check_destination(dest, url, rev_options): - self.fetch_new(dest, url, rev_options) - def get_url(self, location): """Return URL of the first remote encountered.""" remotes = self.run_command( diff --git a/src/pip/_internal/vcs/mercurial.py b/src/pip/_internal/vcs/mercurial.py index 2ab325a5e..2d0750cbb 100644 --- a/src/pip/_internal/vcs/mercurial.py +++ b/src/pip/_internal/vcs/mercurial.py @@ -64,12 +64,6 @@ class Mercurial(VersionControl): cmd_args = ['update', '-q'] + rev_options.to_args() self.run_command(cmd_args, cwd=dest) - def obtain(self, dest): - url, rev = self.get_url_rev() - rev_options = self.make_rev_options(rev) - if self.check_destination(dest, url, rev_options): - self.fetch_new(dest, url, rev_options) - def get_url(self, location): url = self.run_command( ['showconfig', 'paths.default'], diff --git a/src/pip/_internal/vcs/subversion.py b/src/pip/_internal/vcs/subversion.py index 3a56fe6a5..f42aa2f43 100644 --- a/src/pip/_internal/vcs/subversion.py +++ b/src/pip/_internal/vcs/subversion.py @@ -61,9 +61,8 @@ class Subversion(VersionControl): def export(self, location): """Export the svn repository at the url to the destination location""" - url, rev = self.get_url_rev() - rev_options = get_rev_options(self, url, rev) - url = remove_auth_from_url(url) + url, rev_options = self.get_url_rev_options() + logger.info('Exporting svn repository %s to %s', url, location) with indent_log(): if os.path.exists(location): @@ -92,13 +91,6 @@ class Subversion(VersionControl): cmd_args = ['update'] + rev_options.to_args() + [dest] self.run_command(cmd_args) - def obtain(self, dest): - url, rev = self.get_url_rev() - rev_options = get_rev_options(self, url, rev) - url = remove_auth_from_url(url) - if self.check_destination(dest, url, rev_options): - self.fetch_new(dest, url, rev_options) - def get_location(self, dist, dependency_links): for url in dependency_links: egg_fragment = Link(url).egg_fragment @@ -147,6 +139,12 @@ class Subversion(VersionControl): url = 'svn+' + url return url, rev + def get_url_rev_args(self, url): + extra_args = get_rev_options_args(url) + url = remove_auth_from_url(url) + + return url, extra_args + def get_url(self, location): # In cases where the source is in a subdirectory, not alongside # setup.py we have to look up in the location until we find a real @@ -225,9 +223,9 @@ class Subversion(VersionControl): return False -def get_rev_options(vcs, url, rev): +def get_rev_options_args(url): """ - Return a RevOptions object. + Return the extra arguments to pass to RevOptions. """ r = urllib_parse.urlsplit(url) if hasattr(r, 'username'): @@ -250,7 +248,7 @@ def get_rev_options(vcs, url, rev): if password: extra_args += ['--password', password] - return vcs.make_rev_options(rev, extra_args=extra_args) + return extra_args vcs.register(Subversion) diff --git a/tests/unit/test_vcs.py b/tests/unit/test_vcs.py index 1cfd4c409..1c7836bab 100644 --- a/tests/unit/test_vcs.py +++ b/tests/unit/test_vcs.py @@ -177,3 +177,21 @@ def test_bazaar_simple_urls(): def test_get_git_version(): git_version = Git().get_git_version() assert git_version >= parse_version('1.0.0') + + +@pytest.mark.parametrize('url, expected', [ + # Test a basic case. + ('svn+https://svn.myproject.org/MyProject#egg=MyProject', + ('svn+https://svn.myproject.org/MyProject#egg=MyProject', [])), + # Test with username and pass. + ('svn+https://user:pass@svn.myproject.org/MyProject#egg=MyProject', + ('svn+https://svn.myproject.org/MyProject#egg=MyProject', + ['--username', 'user', '--password', 'pass'])), +]) +def test_subversion__get_url_rev_args(url, expected): + """ + Test Subversion.get_url_rev_args(). + """ + vcs = Subversion() + actual = vcs.get_url_rev_args(url) + assert actual == expected From d096c6b3407335929b418fc34b79382126f5822b Mon Sep 17 00:00:00 2001 From: Chris Jerdonek Date: Mon, 9 Jul 2018 23:08:27 -0700 Subject: [PATCH 2/2] Add test_git__get_url_rev_args() to test a non-SVN backend. This addresses a review comment of @pradyunsg. --- tests/unit/test_vcs.py | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/tests/unit/test_vcs.py b/tests/unit/test_vcs.py index 1c7836bab..46e0d66ef 100644 --- a/tests/unit/test_vcs.py +++ b/tests/unit/test_vcs.py @@ -179,19 +179,36 @@ def test_get_git_version(): assert git_version >= parse_version('1.0.0') +# The non-SVN backends all have the same get_url_rev_args() implementation, +# so test with Git as a representative. @pytest.mark.parametrize('url, expected', [ # Test a basic case. - ('svn+https://svn.myproject.org/MyProject#egg=MyProject', - ('svn+https://svn.myproject.org/MyProject#egg=MyProject', [])), - # Test with username and pass. - ('svn+https://user:pass@svn.myproject.org/MyProject#egg=MyProject', - ('svn+https://svn.myproject.org/MyProject#egg=MyProject', + ('git+https://git.example.com/MyProject#egg=MyProject', + ('git+https://git.example.com/MyProject#egg=MyProject', [])), + # Test with username and password. + ('git+https://user:pass@git.example.com/MyProject#egg=MyProject', + ('git+https://user:pass@git.example.com/MyProject#egg=MyProject', [])), +]) +def test_git__get_url_rev_args(url, expected): + """ + Test Git.get_url_rev_args(). + """ + actual = Git().get_url_rev_args(url) + assert actual == expected + + +@pytest.mark.parametrize('url, expected', [ + # Test a basic case. + ('svn+https://svn.example.com/MyProject#egg=MyProject', + ('svn+https://svn.example.com/MyProject#egg=MyProject', [])), + # Test with username and password. + ('svn+https://user:pass@svn.example.com/MyProject#egg=MyProject', + ('svn+https://svn.example.com/MyProject#egg=MyProject', ['--username', 'user', '--password', 'pass'])), ]) def test_subversion__get_url_rev_args(url, expected): """ Test Subversion.get_url_rev_args(). """ - vcs = Subversion() - actual = vcs.get_url_rev_args(url) + actual = Subversion().get_url_rev_args(url) assert actual == expected