New resolver: Use requirement line to populate LinkCandidate._ireq

[uranusjr]

I was looking into a freeze test error (test_freeze_with_requirement_option_multiple), and realised how we built the candidate’s InstallRequirement is broken after merging PEP 610. This is because we were populating the InstallRequirement with the link’s URL as line, which makes PEP 610 records the URL in direct_url.json, and in turn causes pip freeze to use that URL in the output.

The solution here is to use the parent.req as the line instead. This tells the PEP 610 implementation to not treat this as a direct URL, unless parent.req is itself a direct URL.

cc @sbidoul it’d be nice if you could verify whether I’m understanding the PEP 610 implementation correctly.

[sbidoul]

@uranusjr happy to help, but I must admit I have no clue what you are talking about.
How can I reproduce the test failure with test_freeze_with_requirement_option_multiple?
What is a parent requirement in this context?

[uranusjr]

@sbidoul Command to run the test:

PIP_UNSTABLE_FEATURE=resolver tox -e py38 -- tests/functional/test_freeze.py::test_freeze_with_requirement_option_multiple

You’d see the last line in the freeze file doesn’t match expectation (there are other mismatches, but they are not relevant here). The expectation is ...meta==1.0... (the dot dot dot means wild card compare, implemented by doctest.OutputChecker), but the actual output is something like

meta @ file:///[...]/meta-1.0-py2.py3-none-any.whl

You can read the Travis run here as well: https://travis-ci.org/github/pypa/pip/jobs/672815957#L514

In other words, the test expects the requirement to be frozen with a version, but the new resolver freezes it as a direct URL. Does this sound like a change implemented by PEP 610? My assumption to the cause is that the new resolver is building the InstallRequirement incorrectly; it should not supply a URL to the constructor. Does that sound right?

[sbidoul]

@uranusjr I understand better. Thanks for the explanation on how to run the tests with the new resolver.

Yes, if you get an URL out of pip freeze, it comes from direct_url.json. And direct_url.json gets created because of InstallRequirement.original_link:

pip/src/pip/_internal/req/req_install.py

Lines 790 to 796 in c5da021

	direct_url = None
	if self.original_link:
	direct_url = direct_url_from_link(
	self.original_link,
	self.source_dir,
	self.original_link_is_in_wheel_cache,
	)

link is preserved in original_link because it can be replaced by a link to a cache entry during the build process.

So yes, InstallRequirement must only be created with a link if the requirement was an URL, otherwise it is created with req (a Requirement). This is normally taken care of by install_req_from_line via parse_req_from_line.

I don't quite understand yet why the new resolver needs this indirection layer such as make_install_req_from_link(link, parent), nor why _InstallRequirementBackedCandidate needs to know about both a link and an ireq, so I can't say much about the fix in this PR.

[uranusjr]

Thanks for the insight! So the crux to the fix would be to come up with something else to construct InstallRequirement from, but still make it point to a link. And my current try is clearly not working. I’ll try to think of a way.

[uranusjr]

Suepecting at least some errors are caused by #8000.

[pfmoore]

I just merged #8000, so maybe rebase to check?

[uranusjr]

🏎️

[uranusjr]

Things are passing

[sbidoul]

@uranusjr here is a track to explore. Is it correct that what you want to do here is create a new InstallRequirement that is identical to parent except for link? In that case it is important that the new ireq preserves parent's original_link.

[pfmoore]

We did originally have code that cloned the InstallRequirement by creating a new object and copying all(>?) of the state across. It was ugly, but I believe it also had some problems. @uranusjr can you remember what went wrong with that approach? I'm wondering if we should see whether switching back to that approach works any better than what we currently do...

[uranusjr]

The old approach makes the PEP 610 implementation think the cloned InstallRequirement is a direct one, because we would generate the Candidate’s InstallRequirement with a directly URL, i.e. candidates requested by a==1.0.0 and a @ file://.../a-1.0.0-...whl are undistinguishable in the previous approach.

[pfmoore]

Ah yes, that was the problem. I had a suspicion it would be something simple. I need a cup of tea to kick-start my brain :-)