Keep second-level requirements for editable reqs after round one #476
Conversation
| session=self.session) | ||
| dependencies = reqset._prepare_file(self.finder, ireq) | ||
| return set(dependencies) | ||
| if ireq not in self._dependencies_cache: |
There was a problem hiding this comment.
It might be better to key this cache off some derived key of ireq rather than using ireq itself, but I was unclear what would be the optimal key to use. Open to suggestions!
There was a problem hiding this comment.
I don't see a problem for this right now, but I am not the authority on pip-tools code either.
|
I like the relatively simple approach of caching the dependency, and it fixes in an unobtrusive manner the dependencies of an editable. Can you put up a test case so we can catch this in the future (preferably in a commit of the failing situation and then the commit fix)? For the test case, you could probably use -e on the fake_package in tests/fixtures. |
|
@mattlong For your info, I would like to make a 1.8.2 release to fix this regression. The only thing missing is tests. :-) |
|
@davidovich Great! Will try to get a test case in today! |
| @@ -93,16 +93,7 @@ def cli(verbose, dry_run, pre, rebuild, find_links, index_url, extra_index_url, | |||
| # Setup | |||
| ### | |||
|
|
|||
| # Use pip's parser for pip.conf management and defaults. | |||
There was a problem hiding this comment.
Extracted this bit of code into a separate function to facilitate testing
To illustrate Issue 446
Addresses Issue jazzband#446 Besides generally speeding up repetitive requests for secondary dependencies, this avoids an edge case where getting dependencies for editable requirements will return an empty list after the first invocation due to the internals of pip.req.req_set.RequirementSet._prepare_file. In particular, it marks InstallRequirements already visited by the method by setting ireq.parsed = True and short-circuiting on repeated calls with the same input.
|
@davidovich Added some tests! It wasn't as straightforward as I think either of us thought it might be since |
|
Awesome! Thanks from me also @mattlong, started working on adding the tests myself and was getting very confused. |
This is a straw-man proposal to address #466 based on my comments there.