Remove legacy resolver, part 2 #9695
Conversation
MrMino
force-pushed
the
remove_legacy_resolver_pt2
branch
from
c570851 to
e0964eb
Compare
|
Doesn't this PR and #9631 belong to https://github.com/pypa/pip/milestone/47? Is there anything else I can do to help with this milestone? |
|
I’m a bit hesitant to do this since there’s no turning back (except a revert, which can be difficult to do due to conflicts) once the code path is removed. So personally I’m more favouring only removing the option in 21.1 and see what happens first. The code path can be removed after the release so we have time to resolve the remaining issues keeping people on the legacy resolver (they will come complaining). |
|
@uranusjr that's basically why I've split this into two separate PRs :). I'm asking if there's anything else just to make sure that the subject is exhausted on my part (for now). |
|
Ah OK, thanks for the clarification. Yeah I think this is mostly it. There are actually more cleanups available since the code base has several branching logic scattered around to work with both resolvers, but those are basically dead code once this is merged and can be spotted pretty easily when someone sees them. |
Well, I assumed that I've managed to remove all of the branches, so if you see any that are not covered by this PR - please point me to it. I've been rip-grepping the codebase in the hunt for them, so if I missed one, I probably also missed a few others. |
|
IIRC there are some properties/attributes on some classes (especially |
Removing `determine_resolver_variant` from `RequirementCommand` leads logically to the rest of the removals from this commit.
Tests that no longer work on the new resolver: - test_no_reuse_existing_build_dir - doesn't raise anything where PreviousBuildDirError was expected - test_unsupported_hashes - only the first unsupported hash is shown, should show all of them - test_unpinned_hash_checking - only the first unpinned hash is shown, should show all of them
MrMino
force-pushed
the
remove_legacy_resolver_pt2
branch
from
ff8596f to
3daf678
Compare
|
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 |
|
Closing since this has bitrotten. |
|
@pradyunsg I honestly forgotten about this at the time @BrownTruck added the label. Is this still considered useful? I'd be happy to redo this and remove the conflicts if it is. |
This is a superset of #9631.
#9631 is only about disallowing
--use-deprecated=legacy-resolver. This PR is about fully removing the legacy resolver's code, as well as conditional branches based on it - in bothsrc/andtests/.I realize that interdependent PRs might not be the best practice, but since the other one has much tighter scope, and is sufficient to fix the release schedule situation, I decided on splitting the work into two parts, so that the much smaller part 1 can be merged first.
Previously, tests in
tests/unit/test_req.pywere performed using legacy resolver. After switching them to the new one, the following tests started failing, and so were marked withxfail:test_no_reuse_existing_build_dir: the new resolver does not raisePreviousBuildDirErrorwhere old resolver did.test_unsupported_hashes: only the first unsupported hash is listed in the error message, whereas the test expects both of them there.test_unpinned_hash_checking: similarly to the one above, only the first erroneous requirement is shown in the message, but the test explicitly states# Make sure all failing requirements are listed:.@pradyunsg should I update docs from #9656 in this PR?