Properly support interactive Subversion client

[johnthagen]

Fixes #6386

cc @cjerdonek

[johnthagen]

@cjerdonek Do you know of any public Internet-facing SVN repos that contain Python packages we could use to test with? I have some private ones I can test against, but it would also be helpful if there were some that others (like you) could test as part of the review. Even if they aren't private (e.g. needing a password), I think it would be a helpful smoke test.

[cjerdonek]

Do you know of any public Internet-facing SVN repos that contain Python packages we could use to test with?

I don't know offhand, but I agree it would be good to find some examples.

On your PR, for caching reasons, the methods needing get_remote_call_options() are going to have to become instance methods again. Can you tell if that will be a manageable change?

[johnthagen]

@cjerdonek Some initial good news! I tested the following configurations with this PR:

• Ubuntu 18.04 LTS: svn 1.9.7
• Ubuntu 16.04 LTS: svn 1.9.3
• CentOS 7: svn 1.7.14

And pip successfully prompted the user for the password for the private Subversion repo in all situations! I saw pip log the use of --force-interactive for svn 1.9.x as expected and not for svn 1.7.14 (again as expected).

So it would seem the logic is at least sound and actually solves the issue. (Very cool!)

[cjerdonek]

Great to hear! Great work!

[johnthagen]

On your PR, for caching reasons, the methods needing get_remote_call_options() are going to have to become instance methods again.

Yeah, I noticed that too. It seems like this will bubble up into most of the Subversion methods, and also (more intrusively) into VersionControl base class (for example get_requirement_revision).

So, it looks pretty invasive (I'm also not very familiar with where this cacheing would happen. Does pip keep around a global Subversion/Git/etc object where this will be referenced?)

[johnthagen]

The macOS failures look spurious and unrelated to this PR?

[cjerdonek]

(I'm also not very familiar with where this cacheing would happen. Does pip keep around a global Subversion/Git/etc object where this will be referenced?)

I was referring to the self._vcs_version caching that you did. But the caching you're asking about happens with VcsSupport._registry. That's a dictionary that stores an instance for each VersionControl base class once, globally. I made that change recently (to store instances rather than types) in the registry precisely to take advantage of the self._vcs_version caching you added. (Otherwise, self._vcs_version would have to be re-obtained each time the Subversion object is re-instantiated, which would defeat the purpose of the _vcs_version cache.)

For the class methods, if that's something you'd feel more comfortable me doing, let me know and I could do it. It should be an easy, more or less mechanical change. Also, depending on the size of the change, it should maybe go in a separate refactoring PR, too. A lot of those methods were made class methods before this use case came up.

[cjerdonek]

The macOS failures look spurious and unrelated to this PR?

Yes, those are flaky tests (I filed an issue about them not too long ago). And glad you like the "scone" idiom. :) I learned about that recently so want to use it more..

[johnthagen]

I made that change recently (to store instances rather than types) in the registry precisely to take advantage of the self._vcs_version caching you added.

Ah cool, that makes sense.

For the class methods, if that's something you'd feel more comfortable me doing, let me know and I could do it.

Yeah, if you're willing I think it'd be more efficient for you to the the refactor since I'm less familiar with anything outside of the Subversion class.

Also, depending on the size of the change, it should maybe go in a separate refactoring PR, too

Yeah, I feel it would be bets for that refactor to go into another PR. That way, this PR can be minimal for future posterity.

[johnthagen]

I see you reported the flaky test in #6235 (referencing to cross-post another example of failure).

[cjerdonek]

Almost all of the class method changes would be needed only for _get_svn_url_rev(), which is where svn info is called (and only for SVN >= 1.7 in that method). Are you sure that svn info needs to be called interactively, or can _get_svn_url_rev() be implemented in a way that doesn't require calling svn info? One reason I ask is that _get_svn_url_rev() is only needed to get (1) the current integer revision number, and (2) the remote url, which both seem like they should require access only to local information. (And for SVN < 1.7, svn info isn't needed to get the same information.)

[johnthagen]

are you sure that svn info needs to be called interactively

Oh, good catch. Looking at this more closely you're right, we don't need to call interactively since it's not running svn info against a URL (I mis-interpreted the meaning of _get_svn_url_rev).

Since we're running it against a local directory, the interactive password issue shouldn't be relevant to this use of svn info.

I'll update the PR accordingly.

[cjerdonek]

Okay, cool. That means the class method changes will be minimal.

[johnthagen]

I looked over the remaining usages of get_remote_call_options() and they all seem necessary (involve interaction with the server, which may require authentication).

[cjerdonek]

You might as well add a code comment above the svn info invocation saying why it’s not needed in that case (especially because it was initially unclear).

[cjerdonek]

FYI, I posted PR #6519 for the fetch_new() change.

[johnthagen]

You might as well add a code comment above the svn info invocation saying why it’s not needed in that case (especially because it was initially unclear).

Good idea. Done.

[cjerdonek]

A couple last comments.

[johnthagen]

Build failure appears to be the same spurious macOS failure.

[johnthagen]

@cjerdonek I believe I have addressed your comments.

Note that I switched away from using the script fixture as I couldn't find a way to get a normal pytest fixture to work with a TestCase. In this instance, since we're mocking call_subprocess, I don't think it matters, and actually makes the tests overall a little simpler.

[cjerdonek]

Some additional comments after the addition of the test case class.

[johnthagen]

@cjerdonek I believe I have addressed all of your comments.

[cjerdonek]

Looks great! Again, good job on the PR. 😀

[cjerdonek]

Oops, I meant to click "squash" on that. Oh well, not a big deal.

[johnthagen]

Thanks for all of your help getting this through @cjerdonek!

[johnthagen]

Oops, I meant to click "squash" on that. Oh well, not a big deal.

FYI, you probably already know this, but it is possible to set up the GitHub project to require squashed PRs, etc. if that would help.

[pfmoore]

It's no big deal - pip doesn't require squashed PRs. I assume the only reason @cjerdonek intended to squash was because this PR has had quite a lot of revisions.

[cjerdonek]

I assume the only reason @cjerdonek intended to squash was because this PR has had quite a lot of revisions.

Yeah, basically.

but it is possible to set up the GitHub project to require squashed PRs, etc. if that would help.

This occurred to me, too, but there are also many times we don't want to squash -- like when each commit is made separately meaningful and doesn't contain things getting corrected, etc.

As @pfmoore said, it's not a big deal. But if some kind of policy were possible, I think it'd be useful if the UI could be configured to prompt with an "are you sure?" message if you're not squashing and the PR has over N commits (with N perhaps 5 or so). But I doubt GitHub would have anything so fine-grained as that.

[cjerdonek]

By the way, if you want to do a follow-up cleanup, you could move the tests/functional/test_install_vcs_svn.py tests into unit/test_vcs.py (since the former aren't actually functional but unit tests), and also move export(), fetch_new() to the end of the class (after the methods they depend on) since they're currently mixed in with the class methods.

[cjerdonek]

Hi @johnthagen, I just wanted to check again to see if you'd maybe be willing to do the two minor clean-ups I mentioned in my comment above. It would be appreciated!

[johnthagen]

@cjerdonek Sure, I'll work on some PRs.