Pip freeze of editable git repos reports repo path #4760

cboylan · 2017-10-03T00:58:43Z

Rather than report an error for git repositories with no remotes in
pip freeze output report the filesystem location with a file:// url.

Edit by @pradyunsg: Fixes #4759

cboylan · 2017-10-03T01:00:24Z

Fixes issue #4759

pradyunsg · 2017-10-04T13:40:51Z

tests/functional/test_install_vcs.py

@@ -241,21 +241,6 @@ def test_git_with_ambiguous_revs(script):


 @pytest.mark.network
-def test_git_works_with_editable_non_origin_repo(script):


Why has this test been removed?

If there's a behaviour change, it would be wiser to update this test.

Because this test is asserting the (IMO) broken behavior that an error is raised if using an editable install against git repo with no remotes. I updated the test that asserts proper behavior to include the no git remote situation.

This shouldn't be an error (IMO) and therefore we shouldn't have a test asserting that there is an error.

pradyunsg · 2017-10-25T07:38:14Z

tests/functional/test_freeze.py

+        """
+    ).format(remote='file://' + repo_dir).strip()
+    _check_output(result.stdout, expected)
+    # check when there are no remotes. Should report local file path.


I'd like it if you removed this line. :)

pradyunsg · 2017-10-25T07:39:40Z

news/4759.bugfix

@@ -0,0 +1,2 @@
+Rather than report an error for git repositories with no remotes in


pip freeze now reports URL based on file system location for git repositories with no remotes.

pradyunsg

As noted above.

cjerdonek · 2017-10-25T08:36:52Z

src/pip/_internal/vcs/git.py

+                    found_remote = remote
+                    break
+            url = found_remote.split(' ')[1]
+        except InstallationError:


It seems like this try-except should be more tightly bound -- like just around run_command().

Also, one thing I'm wondering about is if this PR can possibly mask other issues, since the absence of remotes isn't the only thing that can cause an InstallationError (non-zero exit status). I'm wondering if there might be a tighter condition than catching all non-zero exit statuses.

Ya we can more tightly scope the error checking. As for masking errors, I'm not sure if git config will return a variety of error codes we can check. But even if it does would it be invalid to report a local path in that case?

cjerdonek · 2017-10-25T08:43:45Z

src/pip/_internal/vcs/git.py

+        except InstallationError:
+            # If the above command fails we have no git remotes. Just use the
+            # local repo path in that case instead.
+            url = 'file://' + location


Does this need to be escaped? I would check elsewhere in the code for how this is done. There seem to be helper functions around this.

We pass location to cwd as is without any escaping. If its good enough for the subprocess execution I'm hoping it is good enough for this string, but I may be missing something important.

xavfernandez · 2017-11-17T22:40:18Z

@cboylan do you plan to keep working on this one ?

cboylan · 2017-11-17T23:46:44Z

Yes, I will pick this back up again. Sorry have been mostly away from my computer due to travel and things. Will try to address some of these comments in the near future.

pradyunsg · 2017-11-20T19:30:30Z

tests/functional/test_freeze.py

+            ...-e git+{remote}@...#egg=version_pkg
+            ...
+        """
+    ).format(remote='file://' + repo_dir).strip()


You'd want to wrap repo_dir with os.path.normcase here.

pradyunsg · 2017-11-20T19:32:15Z

news/4759.bugfix

@@ -0,0 +1,2 @@
+Pip freeze now reports URL based on file system location for git


nit: pip is a noun. Use lowercase here. :P

I believe changes have been made.

JohnVillalovos · 2017-12-14T23:57:40Z

It would be great to get this fixed as we see message like this in our build log:

full installed: ----------------------------------------,Error when trying to get requirement for VCS system Command "git config --get-regexp remote..*.url" failed with error code 1 in /opt/stack/new/tempest, falling back to uneditable format,Could not determine repository location of /opt/stack/new/tempest,asn1crypto==0.22.0,Babel==2.3.4,bcrypt==3.1.3,cffi==1.10.0,cliff==2.8.0,cmd2==0.7.5,cryptography==2.0.2,debtcollector==1.17.1,enum34==1.1.6,extras==1.0.0,fasteners==0.14.1,fixtures==3.0.0,funcsigs==1.0.2,functools32==3.2.3.post2,future==0.16.0,idna==2.5,ipaddress==1.0.18,iso8601==0.1.11,jsonschema==2.6.0,linecache2==1.0.0,monotonic==1.3,msgpack-python==0.4.8,netaddr==0.7.19,netifaces==0.10.6,os-testr==0.8.2,oslo.concurrency==3.21.1,oslo.config==4.11.1,oslo.context==2.17.1,oslo.i18n==3.17.1,oslo.log==3.30.1,oslo.serialization==2.20.1,oslo.utils==3.28.1,paramiko==2.2.1,pbr==3.1.1,positional==1.1.2,prettytable==0.7.2,pyasn1==0.3.1,pycparser==2.18,pyinotify==0.9.6,PyNaCl==1.1.2,pyparsing==2.2.0,pyperclip==1.5.27,python-dateutil==2.6.1,python-mimeparse==1.6.0,python-subunit==1.2.0,pytz==2017.2,PyYAML==3.12,rfc3986==1.1.0,six==1.10.0,stestr==1.1.0,stevedore==1.25.1,## !! Could not determine repository location,tempest==17.2.1.dev58,testrepository==0.0.20,testtools==2.3.0,traceback2==1.4.0,unicodecsv==0.14.1,unittest2==1.1.0,urllib3==1.22,wrapt==1.10.10

BrownTruck · 2017-12-25T20:15:01Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

Rather than report an error for git repositories with no remotes in pip freeze output report the filesystem location with a file:// url. Fixes issue 4759

Reduce scope of try/except when determining git remotes, reword bugfix information, and clean up an unnecessary comment.

This was caught by linting, remove unused exception as e variable in exception handler.

Handle case insensitive filesystems when checking the local path used when git repo has no remotes during testing. Correct case of 'pip' from 'Pip' in the news update.

cjerdonek · 2018-07-03T21:59:05Z

I made a comment about escaping the URL that doesn't seem to have been addressed:
#4760 (comment)
Don't special characters in the path need to be quoted if included as part of a URL?

cjerdonek · 2018-07-04T10:31:16Z

To elaborate on my previous comment, I referenced a comment from 8 months ago in which I said the following in reference to escaping paths used in URLs:

I would check elsewhere in the code for how this is done. There seem to be helper functions around this.

I searched the code, and here is an example of one of the helper functions I was referring to--

pip/src/pip/_internal/download.py

Lines 474 to 481 in 4ba6769

    
           def path_to_url(path): 
        
               """ 
        
               Convert a path to a file: URL.  The path will be made absolute and have 
        
               quoted path parts. 
        
               """ 
        
               path = os.path.normpath(os.path.abspath(path)) 
        
               url = urllib_parse.urljoin('file:', urllib_request.pathname2url(path)) 
        
               return url

cjerdonek · 2018-07-05T06:10:04Z

src/pip/_internal/vcs/git.py

@@ -203,17 +203,24 @@ def obtain(self, dest):

    def get_url(self, location):
        """Return URL of the first remote encountered."""


This docstring should be updated to reflect the new behavior.

cjerdonek · 2018-07-05T06:18:40Z

src/pip/_internal/vcs/git.py

+                ['config', '--get-regexp', r'remote\..*\.url'],
+                show_stdout=False, cwd=location,
+            )
+        except InstallationError:


In a previous review comment here, I made the suggestion that the "file" case not be handled simply by catching any command error. The reason is that this can result in masking various errors and bugs. It would be better if a more targeted approach can be used. Is there a different Git command that can be used that can list the remotes (even if there are 0) without erroring out with a non-zero exit status?

For example, why can't the output of git remote -v be used? That command still has exit status 0 (and so won't result in an InstallationError) even in the case of no remotes.

cjerdonek · 2018-07-05T06:19:36Z

src/pip/_internal/vcs/git.py

+            # If the above command fails we have no git remotes or cannot
+            # determine what they are. Just use the local repo path in that
+            # case instead.
+            url = 'file://' + location


See my comment here for another previous review comment, regarding escaping characters.

BrownTruck · 2018-07-29T21:00:04Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

Alexander-Shukaev · 2018-09-16T00:29:35Z

When merge?

pradyunsg · 2018-09-17T08:56:11Z

When all reviews are addressed.

Alexander-Shukaev · 2018-09-17T13:20:47Z

I see there are only minor changes required and it could be that the person has already abandoned this pull request/moved on or is simply too busy to work on it. Since the changes are minor, why not simply apply them on your own and just merge it?

cjerdonek · 2018-09-17T15:41:16Z

Speaking for myself, when I made review comments, I didn't mean to suggest that if those were addressed, I would be okay with the patch. The comments I made were just the biggest things that popped out at me. There are other things I see, too, now. But if the first things aren't addressed, then I'm not sure it makes sense to add even more things.

pradyunsg · 2018-09-17T15:50:31Z

Indeed, that is an option and someone probably will do that if they find the time to do so.

ssbarnea · 2018-12-13T20:11:56Z

@pradyunsg @Alexander-Shukaev Can we try to me more effective and do our best to fix this bug?

I am not sure if @cboylan still have the energy (and hope) to see this PR merged.

On GitHub project members can edit PRs and fix them regardless whom raised the PR in the first place.

cjerdonek · 2018-12-13T22:30:27Z

@ssbarnea I have been working up to this. PR #5905 (merged) and PR #5912 (for a helper argument it introduces) were precursors.

cjerdonek · 2018-12-21T08:55:49Z

FYI, I posted another PR to address this issue: #6095. It addresses some issues I raised here and takes advantage of some more recent changes to the code (e.g. the new extra_ok_returncodes argument to VersionControl.run_command()).

cjerdonek · 2019-01-06T22:22:21Z

Closing as this was addressed by PR #6095 referenced above (now merged).

pradyunsg · 2019-01-08T16:47:41Z

Thank @cboylan for this PR! :)

lock · 2019-05-31T04:20:49Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

pradyunsg reviewed Oct 4, 2017

View reviewed changes

pradyunsg added type: enhancement Improvements to functionality C: freeze 'pip freeze' related labels Oct 4, 2017

pradyunsg reviewed Oct 25, 2017

View reviewed changes

pradyunsg requested a review from a team October 25, 2017 07:39

pradyunsg previously requested changes Oct 25, 2017

View reviewed changes

cjerdonek reviewed Oct 25, 2017

View reviewed changes

pradyunsg reviewed Nov 20, 2017

View reviewed changes

pradyunsg requested a review from a team November 20, 2017 19:42

BrownTruck added the needs rebase or merge PR has conflicts with current master label Dec 25, 2017

cboylan force-pushed the no-git-remote branch from 57c8b28 to ccf97c6 Compare January 5, 2018 19:42

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jan 5, 2018

cboylan added 4 commits March 13, 2018 08:42

Pip freeze of editable git repos reports repo path

23a311b

Rather than report an error for git repositories with no remotes in pip freeze output report the filesystem location with a file:// url. Fixes issue 4759

Cleanup based on PR review comments

feb8d80

Reduce scope of try/except when determining git remotes, reword bugfix information, and clean up an unnecessary comment.

Remove unused local variable

1edc109

This was caught by linting, remove unused exception as e variable in exception handler.

Address more review comments

6e9d961

Handle case insensitive filesystems when checking the local path used when git repo has no remotes during testing. Correct case of 'pip' from 'Pip' in the news update.

cboylan force-pushed the no-git-remote branch from ccf97c6 to 6e9d961 Compare March 13, 2018 16:09

xavfernandez approved these changes Jul 3, 2018

View reviewed changes

pradyunsg added the C: vcs pip's interaction with version control systems like git, svn and bzr label Jul 4, 2018

cjerdonek suggested changes Jul 5, 2018

View reviewed changes

BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 29, 2018

This was referenced Dec 21, 2018

Address #4759: change freeze to support editable Git repos with no remote #6095

Merged

pip freeze errors when packages are pip install -e'd against git repos with no remote #4759

Closed

cjerdonek closed this Jan 6, 2019

lock bot added the auto-locked Outdated issues that have been locked by automation label May 31, 2019

lock bot locked as resolved and limited conversation to collaborators May 31, 2019

		@@ -241,21 +241,6 @@ def test_git_with_ambiguous_revs(script):


		@pytest.mark.network
		def test_git_works_with_editable_non_origin_repo(script):

		@@ -0,0 +1,2 @@
		Rather than report an error for git repositories with no remotes in

		@@ -0,0 +1,2 @@
		Pip freeze now reports URL based on file system location for git

		@@ -203,17 +203,24 @@ def obtain(self, dest):

		def get_url(self, location):
		"""Return URL of the first remote encountered."""

Pip freeze of editable git repos reports repo path #4760

Pip freeze of editable git repos reports repo path #4760

Conversation

cboylan commented Oct 3, 2017 • edited by pradyunsg Loading

cboylan commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavfernandez commented Nov 17, 2017

cboylan commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnVillalovos commented Dec 14, 2017 • edited Loading

BrownTruck commented Dec 25, 2017

cjerdonek commented Jul 3, 2018

cjerdonek commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrownTruck commented Jul 29, 2018

Alexander-Shukaev commented Sep 16, 2018

pradyunsg commented Sep 17, 2018

Alexander-Shukaev commented Sep 17, 2018

cjerdonek commented Sep 17, 2018

pradyunsg commented Sep 17, 2018

ssbarnea commented Dec 13, 2018

cjerdonek commented Dec 13, 2018

cjerdonek commented Dec 21, 2018

cjerdonek commented Jan 6, 2019

pradyunsg commented Jan 8, 2019

lock bot commented May 31, 2019

cboylan commented Oct 3, 2017 •

edited by pradyunsg

Loading

pradyunsg left a comment •

edited

Loading

JohnVillalovos commented Dec 14, 2017 •

edited

Loading