Pip10 update

[techalchemy]

Update pip-tools for pip10 compatibility (and backwards compatibility with pip9)

Contributor checklist

• Provided the tests for the changes
• Requested (or received) a review from another contributor
• Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md afterwards).

/cc @vphilippon

I didn't add tests, since I just rebuilt existing functionality using pip10 as well. This should work with both versions of pip and is all green locally

[suutari]

Why is this changed?

IIRC changing this makes installed packages affect how the requirements are compiled, which is not a good thing.

[suutari]

I think this is a good change. IMHO being compatible with Pip 10 is better than vendoring old Pip, since vendoring always has its problems. (E.g. we would have to backport all security and bug fixes from upstream Pip, etc. The PyPI URL change being a good example.)

It seems that the build matrix doesn't include Pip 9 anymore though. Can you add it back with Pip 8 and 10 so we could see if all tests pass on other Pip version than 10 too?

[vphilippon]

The default index URL changes with the pip version, so I would make this match the one provided by pip itself if we stop vendoring pip.

[techalchemy]

Yeah this is just totally non-functional if you don't point at the new pypi, but I am not sure if that's an issue of testing in tox environments and not in proper environments.

[vphilippon]

Thanks for putting this together @techalchemy !
As @suutari said, if we're un-vendoring, we'll want to put back the pip "dimension" in the test matrix.
Here's the commit that remove it: 61163e0

Ideally, we'd keep pip 8 to 10 compatibility, but if this gets messy, we can look to drop pip 8. I'll try to find some usage stats on pip 8 first though.

Edit:
Here's some pip download stats, over the last 30 days:

| version  | download_count |
| -------- | -------------- |
| 9.0.3    |      8,023,066 |
| 6.1.1    |      4,364,196 |
| 9.0.1    |      3,030,700 |
| 10.0.1   |      2,826,028 |
| 10.0.0   |      2,594,810 |
| 10.0.0b2 |      1,247,348 |
| 10.0.0b1 |        337,727 |
| 7.1.2    |        325,324 |
| 8.1.1    |        163,670 |
| 8.1.2    |         49,028 |

The data doesn't take into account people with pip 8 installed and not downloading it, but it gives an overview.

[techalchemy]

OK I hit most of the major items -- I have no idea if this will work with the matrix in question becuase of the way it grabs the index url -- we may need to keep this pointed at the current pypi.

[techalchemy]

Alrighty so I was wrong, but now this is mostly working except a tiny pip8 error and I would appreciate any guidance at all on what to do with that one

[techalchemy]

scratch that, it was just a new style marker test

[vphilippon]

@techalchemy
Ok first off, once again, thanks a million time for your work.
Second, sorry for the delays in reply, its hard to get the time to give a whole proper check/reply at this.

By unvendoring, we fully match the pip the end-user chooses to use, default PyPI URL and everything else. I agree with @suutari and you on this.

But by unvendoring, we also put ourselves (and all pip-tools users) back at risk of breakage on any pip update. The stuff we use was explicitly refactored in a package named _internal for crying out loud.

I hate everything about this situation.
The least-horrible thing I can think of is to go ahead and unvendor pip, and if we get to regret this, just vendor it back.

Thoughts?

[rpkilby]

While not ideal, would a better solution an acceptable alternative be to vendor the most recent version of pip? This would require a bit of upkeep, but would avoid the cost of maintaining compatibility across three major versions of pip.

[techalchemy]

I don't have particularly strong feelings except to say that we vendor pip over in pipenv (and have been doing so) in order to maintain compatibility with piptools specifically and that also can cause issues related to certificates, but gives us some flexibility to make changes to the dependency resolver directly. In all likelihood we will end up vendoring pip 10 anyway so I can see an argument for that, but if you're not modifying the resolver it may not make sense.

Overall they moved some APIs around primarily to drive some discussion about whether the internal apis really ought to be available externally, and we are in touch about that (I believe pipenv is probably the worst offender but also is part of the PyPA and works collaboratively with the pip team). Overall I don't see much risk of any sweeping API changes, but I guess you never know. Either approach is going to come with maintenance overhead. I think that's just the cost of maintainership.

In the end no decision is going to protect pip-tools from having to handle adjusting the codebase in the future, I'm kind of split on this one. IMO unvendoring is easiest

[techalchemy]

Also sincere apologies I know this is nearly impossible to review properly, here is a near-complete set of the changes (sans the first commit) without the removal of pip 9:

https://github.com/jazzband/pip-tools/pull/657/files/4c7fce7897e4dbbba524b59e0a52ee38e37a7aa6..3ba2956232fad316b8c07daf65c7a23d4aa96033

[suutari]

Thanks @techalchemy.

The changes look good and the tests pass. 👍

Though I would squash this into one commit or maybe two commits where first one does everything else but the removing of vendored Pip and the second one removes the vendored Pip. That's entirely up to you, if you want to do such git rebase activity...

Good work!

[suutari]

@vphilippon I see you as the lead maintainer of pip-tools currently, so IMHO it's your call if the Pip should be vendored or not. I'd vote for not vendoring, if anyone cares.

[tysonclugg]

I vote for un-vendoring pip, along with using stricter version requirements in our install_requires.

The release of pip-tools that un-vendors pip should depend on pip>=8.0,<10.1. Patch releases of pip should be allowed. When a new major/minor version of pip is released, nothing will break. People can submit issues/pull requests to enable new versions of pip when they become available.

[techalchemy]

@suutari no problem I agree with that rebase / squash. @tysonclugg I think this approach makes the most sense for now as well except that I believe pip is switching to calver which will complicate this.

[tysonclugg]

@techalchemy You're right that CalVer will complicate things, especially given the new pip deprecation policy. But I'm not aware of a better alternative, aside from helping the pypa team to make the necessary pip internal APIs public if at all possible.

The pip release cadence docs state that master should always be in a releasable state. We should include a pipmaster factor in our tox.ini (and .travis.yml) in order to catch issues before a breaking version of pip is released.

[techalchemy]

we should include a pipmaster factor

Best suggestion yet

[vphilippon]

Thanks for everyone input.
Lets go with un-vendoring then, I think it makes sense.

@tysonclugg's suggestion to put pip in the install_requires worries me for Windows user though. Doing pip install pip-tools will cause issue as pip will to uninstall/modify itself if it doesn't match, leaving ugly errors like:

Exception:
Traceback (most recent call last):
  File "f:\virtualenvs\tmp-abfbe3982928ec9c\lib\site-packages\pip\basecommand.py", line 215, in main
    status = self.run(options, args)
  File "f:\virtualenvs\tmp-abfbe3982928ec9c\lib\site-packages\pip\commands\install.py", line 342, in run
    'Successfully downloaded %s', downloaded
  File "f:\virtualenvs\tmp-abfbe3982928ec9c\lib\site-packages\pip\req\req_set.py", line 795, in install
  File "f:\virtualenvs\tmp-abfbe3982928ec9c\lib\site-packages\pip\req\req_install.py", line 767, in commit_uninstall
    response = ask_path_exists(
  File "f:\virtualenvs\tmp-abfbe3982928ec9c\lib\site-packages\pip\req\req_uninstall.py", line 142, in commit
    rmtree(self.save_dir)
  File "f:\virtualenvs\tmp-abfbe3982928ec9c\lib\site-packages\pip\_vendor\retrying.py", line 49, in wrapped_f
    return Retrying(*dargs, **dkw).call(f, *args, **kw)
  File "f:\virtualenvs\tmp-abfbe3982928ec9c\lib\site-packages\pip\_vendor\retrying.py", line 212, in call
    raise attempt.get()
  File "f:\virtualenvs\tmp-abfbe3982928ec9c\lib\site-packages\pip\_vendor\retrying.py", line 247, in get
    six.reraise(self.value[0], self.value[1], self.value[2])
  File "f:\virtualenvs\tmp-abfbe3982928ec9c\lib\site-packages\pip\_vendor\retrying.py", line 200, in call
    attempt = Attempt(fn(*args, **kwargs), attempt_number, False)
  File "f:\virtualenvs\tmp-abfbe3982928ec9c\lib\site-packages\pip\utils\__init__.py", line 102, in rmtree
    onerror=rmtree_errorhandler)
  File "c:\python27\Lib\shutil.py", line 247, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "c:\python27\Lib\shutil.py", line 247, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "c:\python27\Lib\shutil.py", line 247, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "c:\python27\Lib\shutil.py", line 252, in rmtree
    onerror(os.remove, fullname, sys.exc_info())
  File "f:\virtualenvs\tmp-abfbe3982928ec9c\lib\site-packages\pip\utils\__init__.py", line 114, in rmtree_errorhandler
    func(path)
WindowsError: [Error 5] Access is denied: 'f:\\tmp\\pip-o9ongj-uninstall\\virtualenvs\\tmp-abfbe3982928ec9c\\scripts\\pip.exe'

which will end up as recurring Github a few minutes later.

This would force Windows users to use python -m pip install pip-tools, and that's going to break stuff for a lot of users and automated CI system right now. As a dev working on Windows... Please don't 😄 .

We can add a check early in pip-compile/pip-sync to check pip version and act on that. The check will occurs on pip-tools's use rather than install, but I think it's the best we can do right now. IIRC there was a check like that prior to vendoring pip.

[techalchemy]

I honestly think building against master to stay ahead of issues is the best bet. But I don’t think there is much risk of huge sweeping changes in the near future

[tysonclugg]

@vphilippon You're right that having an install_requires line for pip is a bad idea. Having the pipmaster factor in CI should be enough, without the install_requires changes I had proposed.

[tysonclugg]

@vphilippon Would you like me to pick this up and add the pipmaster CI factor?

[techalchemy]

Sorry for the delay on this I’ve been trying to manage a pipenv release. Feel free to push a pipmaster factor here to speed things up, no issues with that from my end

[tysonclugg]

@techalchemy We're not using appveyor.yml, and I don't have access to change settings on the pip-tools Appveyor project. Either I create an appveyor.yml, somebody gives me access, or someone else adds the pipmaster factor there. Which will it be?

[suutari]

@tysonclugg: It's .appveyor.yml here: https://github.com/jazzband/pip-tools/blob/master/.appveyor.yml

[tysonclugg]

@techalchemy I'm trying to understand why we should use an environment variable to determine the pip version rather than inspecting the pip.__version__ attribute in the test suite.

Can you please enlighten me?

[techalchemy]

@tysonclugg this is because we are configuring the environment to tell the tests what we are expecting to see, i.e. in this case we expect pip 9 and above to pass the test and we indicate which pip version we expect via the environment. What we actually get in the code may be different, so to avoid having to build sanity checks everywhere it's just easier to check the thing we are setting everything based off of.

[tysonclugg]

@techalchemy May I change the decorator to wrap unittest.expectedFailure instead of unittest.skipIf, so that we can see an unexpected pass/fail when the environment doesn't match reality, rather than skipping the test?

diff --git a/tests/test_cli.py b/tests/test_cli.py
index dfea0cf..6d6c820 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -16,10 +16,11 @@ from pip import __version__ as pip_version
 
 PIP_VERSION = parse_version(os.environ.get('PIP', pip_version))
 
-skip_unless_pip9 = pytest.mark.skipif(
-    PIP_VERSION < parse_version('9'),
-    reason='needs pip 9 or greater'
-)
+def expected_failure_unless_pip9(test_method):
+    if PIP_VERSION < parse_version('9'):
+        return unittest.expectedFailure(test_method)
+    else:
+        return test_method
 
 
 @pytest.yield_fixture
@@ -359,7 +360,7 @@ def test_generate_hashes_with_editable():
     assert expected in out.output
 
 
-@skip_unless_pip9
+@expected_failure_unless_pip9
 def test_filter_pip_markes():
     """
     Check that pip-compile works with pip environment markers (PEP496)

[techalchemy]

@tysonclugg sure but it's a bit different for pytest, just a sec and I'll add it

[tysonclugg]

@techalchemy I don't see the point of shunning unittest in favour of pytest in this case, since it's no more verbose and functionality is identical.

[techalchemy]

@tysonclugg because that's the framework we're using, why would we use two frameworks when we can use one which we use for the rest of the testing? I'm not 'shunning' anything.

[tysonclugg]

@techalchemy My mistake, carry on!

[techalchemy]

OK all, squashed down to 2 commits, building against pip@master, hopefully this is good to go

[tysonclugg]

Small aside: I understand the desire to squash changes down to the minimal required, but I'm a little dismayed that contributions are incorrectly attributed to the person who performed the squash. Obviously this is more of a social issue than a technical one.

It bummed me out when I finally had a change accepted into @django, only to have a core member gain attribution (in terms of git stats) for the work I did. Not so much the case here, but you get the gist of what I'm saying.

Don't worry about undoing the squash in this particular instance - just be mindful of the social implications when squashing and/or rebasing commits.

I wonder if git could be improved to maintain attribution for squashed commits?

[techalchemy]

@tysonclugg if your intention in asking to add the pip master branch here was to get accolades by jumping onto this PR then I apologize, but we discussed the squash much earlier and it was always going to happen with or without your contributions, which will still be attributed to you even if the specific commit isn’t. I am pretty careful to always include credit on the lines by the author in question, which fit handles just fine.

Basically, I am mindful of it. I changed some things, I appreciate your willingness to help and I kept credit where possible, but this was always getting squashed. It was discussed and decided before you ever asked if you could make your change.

You should consider the reason for your offer here— are you here to help out the project, or are you merely interest in attribution?

I realize those are not mutually exclusive, but this is how it works. Sometimes your stuff gets reworked. If you make it easy to collaborate with you, it’s a lot more likely that projects will cooperate on bigger changes

[tysonclugg]

You should consider the reason for your offer here— are you here to help out the project, or are you merely interest in attribution?

I already said not to worry about undoing the squash - I'm here to help. My offer to help isn't dependent upon attribution. I'm sorry I mentioned it, I'm ready to move on.

[tysonclugg]

@techalchemy I've just become aware that GitHub recently added support for “Co-authored-by” trailers on commit messages. This answers my question regarding if git could be improved to maintain attribution for squashed commits.

Could you please amend the commit message(s) to include the relevant "Co-authored-by" trailer lines to include myself and the others who have contributed in various forms (ie: code, ideas, etc) to this PR?

[techalchemy]

Please stop derailing this PR over a line of testing instructions that is already attributed to you.

[vphilippon]

With the 2 commits, this was way easier to review, thanks a lot!

I spotted 2 minor things, then we should good for merging, thanks again for all your work! I'll keep an eye out to not let this on hold.

Btw, this will be introduced in a major (3.x.x) version for consistency with the previous version bump.

[vphilippon]

We have a duplicate PIP=master here

[vphilippon]

Could we have a comment pointing which section is pre-pip 10 and post-pip 10 please, that will help future contributors (and the future "us"!)

[vphilippon]

Oops, Last review was not meant to be "approve" as there where some minor fix to do.

[techalchemy]

All set on the changes

[vphilippon]

Thanks everyone!

[jezdez]

Hi all, a Jazzband roadie here.

@tysonclugg has asked me to weigh in here with regard to a missing attribution of contribution for the comment above and for a commit @tysonclugg has allegedly contributed as well (in his own words: "to add the pipmaster factor to tox.ini and travis.yml"). I can't verify the latter as it no longer is part of the pull request or the set of commits that landed in master due to the way the commits of this pull request were squashed.

It's a particular unfortunate combination of circumstances that requires reminding everyone of the existence of the Jazzband code of conduct which governs the conduct in Jazzband projects. One aspect of it is the ethical and professional manner in which we must collaborate, including respecting the right for attribution of contributions by fellow members.

Reading through the comments of this PR I see there were multiple opportunities to acknowledge @tysonclugg's contribution (e.g. keep the commit intact, adding a “Co-authored-by” line to the commit message, updating an AUTHORS file, leaving a code comment etc). As none of these steps were taken and multiple requests for attribution (e.g. #657 (comment)) have been ignored this is a violation of the Jazzband Code of Conduct.

Additionally I've found the response to the requests for attribution lacking the appropriate respect towards @tysonclugg as it specifically boasted to take great care to attributing contributions (and failing to do so) and called @tysonclugg's motivation to contribute into question at the same time. I want to be clear that this is not acceptable behavior towards fellow Jazzband members as it does not cater to a welcoming community.

Here are the actions to follow:

• @techalchemy Please add an attribution for @tysonclugg's contribution somewhere along the lines of other contributions to this project.

• @techalchemy Consider offering an apology to @tysonclugg for the misconduct in professional and ethical aspects of this collaboration.

• @tysonclugg Please accept my apology for the experience you've had and my thanks for working on resolving it by reaching out to me.

This pull request will be locked as it's not an appropriate avenue for feedback. Please write to roadies@jazzband.co instead.

[jezdez]

Update on the actions taken:

• @vphilippon agreed to add an attribution of @tysonclugg's contribution to the CHANGELOG upon the next release.

• @techalchemy has been freed from the action to add an attribution of @tysonclugg's contribution as stated in the previous comment.

I consider the matter resolved now.