Fix #5624: installing a Git ref for installs other than the first #5623
Conversation
cjerdonek
changed the title
cjerdonek
changed the title
cjerdonek
force-pushed
the
vcs-fix-install-git-ref
branch
from
e1b1605 to
24e7546
Compare
cjerdonek
added
C: vcs
cjerdonek
force-pushed
the
vcs-fix-install-git-ref
branch
from
24e7546 to
b1ba837
Compare
|
Hi @di, would you mind taking a quick look at this PR, given that you were the one to first start adding support for Git refs? Thanks! |
cjerdonek
force-pushed
the
vcs-fix-install-git-ref
branch
from
b1ba837 to
d5af777
Compare
| return url | ||
|
|
||
|
|
||
| def test_git_install_ref(script): |
There was a problem hiding this comment.
Seems like these tests lost the @pytest.mark.network decorator. I'm not 100% sure what the mark means in the context of this test suite, but all the other tests in this file have it, perhaps these two should as well?
There was a problem hiding this comment.
The decorator is essentially for marking all tests that reach out to the network.
There was a problem hiding this comment.
Yes, I removed it because _create_test_package() creates the source package on the file system, and the package is installed with the 'git+file:// scheme (see make_version_pkg_url() a few lines above).
I thought the network decorator was only meant for cases where the test was reaching out to an external network, like test_install_noneditable_git, which installs from GitHub (with url git+https://github.com/pypa/pip-test-package.git). It looked to me like some other tests in the file might also have the decorator unnecessarily.
Technically, I guess the Git docs do say that file:// "fires up the processes that it normally uses to transfer data over a network, which is generally much less efficient," but it seems like there is a big difference between using the same "processes" for network activity and actually reaching out to an external network location like, say, GitHub. What do you both think? I'm guessing the decorator is used to help partition tests for CI purposes (e.g. which tests are slower or depend on external networks, etc).
There was a problem hiding this comment.
The partitioning is so that you can run tests while not connected to the internet.
If this test runs with no internet connectivity, then it doesn't need this decorator and vice versa.
There was a problem hiding this comment.
Okay, I'll leave the decorator off then because as near as I can tell it doesn't require internet connectivity.
| """ | ||
| package_path = _create_test_package(script) | ||
| _add_ref(script, package_path, 'refs/foo/bar') | ||
| _change_test_package_version(script, package_path) |
There was a problem hiding this comment.
Yes, I think so because otherwise version.stdout for the ref will be the same as version.stdout for master (namely both 0.1), in which case we won't know if the test is succeeding because it's actually installing the ref or just because it happens to match master. By running _change_test_package_version(), it makes it so that version.stdout for the ref is 0.1 and version.stdout for master is some different version, so the test has to install the ref to succeed.
news/5624.bugfix
Outdated
| @@ -0,0 +1 @@ | |||
| Fix installing a Git ref for installs after the first. | |||
There was a problem hiding this comment.
I think this could be more clear, it was not obvious to me what this change was about until I actually looked at the code. Perhaps "Allow a git ref to be installed over an existing non-ref install"
There was a problem hiding this comment.
Okay, I'll try rephrasing. The meaning was meant to be a bit more general actually -- allowing a Git ref to be installed over an existing install of any kind. Currently (without this PR), if it's the very first install (a "fresh" install), installing a Git ref works. But if it's the second or later install (i.e. an "update" to an install), then it doesn't work, no matter what the previous installs were. So the PR makes the latter case work, too.
There was a problem hiding this comment.
Right, so maybe "Allow a git ref to be installed over an existing installation" then?
That said I think most casual users are not going to understand this change regardless so perhaps it's fine as is. 🙂
|
Thanks for reviewing, both of you! |
This addresses a review comment by @di.
|
Okay, I pushed a new commit with the better NEWS entry (using @di's suggested wording). Thanks again for the review and comments! |
|
Hi @pradyunsg, would you be able to merge this now that it has been approved and review comments addressed? Thanks! |
|
Yep. Can do. |
|
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. |
lock
bot
added
the
auto-locked
This gets installing a Git "ref" working for pip installs other than the first.
This change also has some independent benefits. For example, it simplifies the implementation of
Git.fetch_new()by pulling some of the code out into a new (renamed) method calledGit.resolve_revision()-- responsible for resolving all types of revision names (branch, tags, and refs, etc). Because this method is also called inGit.update()(just likeGit.fetch_new()), it fixes the issue.