Upgrade ResolveLib to 0.4.0 and implement the new Provider.find_matches() interface

[uranusjr]

I think I get the logic right (or at least mostly), but the implementation probably looks a bit messy. Recommendations most definitely welcomed :)

Note that ResolveLib 0.4.0 requires the candidates to be returned in reverse order, so I added reverse=True to Provider._sort_matches().

I’ll probably need to add a test based on #8249.

[uranusjr]

Alright, I think this is ready. I’m leaving out the lru_cache() for now because I’m still afraid it might break something. It probably belongs to a follow-up PR…

[pfmoore]

I can't see anything wrong with the implementation, and the tests pass, so let's go for it. I'm not a fan of get_candidate_lookup but to be honest, I don't know how to write it differently (at least, not without checking for explicit types, or something else that is even uglier than get_candidate_lookup). Let's worry about this in a follow-up PR (why do I think that we won't get round to that follow-up...? 🙄)

[pfmoore]

I suspect @pradyunsg may be too busy to review this, but I'll leave it a day or so to give him a chance before merging.

[uranusjr]

I'm not a fan of get_candidate_lookup but to be honest, I don't know how to write it differently (at least, not without checking for explicit types […])

I probably wrote too much Rust and was wishing for pattern-matching in Python, it would have made the implementation much easier. Maybe I should go read some Martin Fowler books to learn more OOP tricks. (I won’t)

[pfmoore]

I probably wrote too much Rust and was wishing for pattern-matching in Python

lol, yeah, I'm trying to use more Rust and the pattern matching is awesome. What worries me about the current code is that some bright spark will decide that returning both a candidate and an InstallRequirement is a good idea. And no, I don't think an assert helps much, it just papers over the real problem (lack of Rust's Enum types 🙂).

[pradyunsg]

I'll probably take a look later today, after I get tired of explaining every possible phrase in writing, like "package manager" and "dependency". :)

Feel free to merge without me taking a look though -- I'd done a quick-skim yesterday, and didn't really have much to say TBH.

[BrownTruck]

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 be eligible for code review and hopefully merging!

[uranusjr]

Note: Delaying this since #8066 (wheel cache) will cause a conflict. Both PRs are approved, but the other one can be merged as-is right now, so I’ll rebase this one later.

[uranusjr]

Note for self: Since #8319 makes the result of find_matches() depend on the finder’s ordering, the current approach of sorted(matches, key=sort_key, reverse=True) will not work.

The easiest solution would be sorted(reversed(matches), key=sort_key), but I think it would be easier to just pass the eligible_for_upgradee flag into the factory with this implementation, since this implementation calls the factory directly anyway.

[uranusjr]

Gosh, I’ll need to rethink the candidate ordering. This will take a while.

[uranusjr]

I hope so much this will work.

[pfmoore]

Hey Travis, report your flipping status when you complete!

[uranusjr]

Looks like we have a regression.

test_install_editable_with_wrong_egg_name

  File "/tmp/pytest-of-travis/pytest-0/popen-gw1/pip0/pip/src/pip/_internal/resolution/resolvelib/candidates.py", line 221, in iter_dependencies
    for r in self.dist.requires():
  File "/tmp/pytest-of-travis/pytest-0/popen-gw1/pip0/pip/src/pip/_internal/resolution/resolvelib/candidates.py", line 202, in dist
    self._prepare()
  File "/tmp/pytest-of-travis/pytest-0/popen-gw1/pip0/pip/src/pip/_internal/resolution/resolvelib/candidates.py", line 186, in _prepare
    assert (
AssertionError: Name mismatch: 'pkgb' vs 'pkga'

[uranusjr]

Uh, wait, that was passing…? It’s literally testing that pip should install a package that’s specified with the wrong project name:

def test_install_editable_with_wrong_egg_name(script):
    script.scratch_path.joinpath("pkga").mkdir()
    pkga_path = script.scratch_path / 'pkga'
    pkga_path.joinpath("setup.py").write_text(textwrap.dedent("""
        from setuptools import setup
        setup(name='pkga',
              version='0.1')
    """))
    result = script.pip(
        'install', '--editable',
        'file://{pkga_path}#egg=pkgb'.format(**locals()),
    )
    assert ("Generating metadata for package pkgb produced metadata "
            "for project name pkga. Fix your #egg=pkgb "
            "fragments.") in result.stderr
    assert "Successfully installed pkga" in str(result), str(result)

[uranusjr]

I think we should get this in first and figure out the test config mess later, to not block this any longer 🙁

[pradyunsg]

Hey Travis, report your flipping status when you complete!

This, is why it was demoted to no-longer being a required status.

[pradyunsg]

Uh, wait, that was passing…? It’s literally testing that pip should install a package that’s specified with the wrong project name:

Yea, we can start making that error out, with a faster-than-usual deprecation cycle (deprecate in 20.2, drop in 20.3). :)

I'm pretty sure it exists because people make mistakes and we didn't want to break workflows for users back then.

[pradyunsg]

I think we should get this in first and figure out the test config mess later, to not block this any longer 🙁

I'm happy with that.

[pfmoore]

I'm pretty sure it exists because people make mistakes

Yeah, it's basically saying that we ignore every bit of metadata we could use because it might be wrong 🙁 That's a philosophy that we need to stop - unless we start saying that people need to not lie to pip (intentionally or accidentally), we're never going to be able to get anywhere.

[pfmoore]

OK, pushing the button. Let the merge conflicts begin!