-
-
Notifications
You must be signed in to change notification settings - Fork 289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix corner case in cached dependency resolution #362
Fix corner case in cached dependency resolution #362
Conversation
Ref: pex-tool#178 The error is as follows: 1. Two resolvables, one pinned, the other not. 2. First pex run with clean cache works. 3. Change the pinned version. 4. Second pex run fails with Unsatisfiable. The problem is that the non-pinned resolvable locates the cached dep and only returns it. Since that version doesn't match the pinned version, we fail. This fixes that behavior by returning both the cached version and all uncached pypi packages for unpinned resolvables. We change the pinned version to test the use case where you have a cached version of a dep and then you transitively pull in another requirement that either has a tighter bound or a pin on a version not found in the cache.
15ccd05
to
f18543b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm w/ one minor thought. thanks for fixing this bug!
pex/resolver.py
Outdated
# packages because the cached version might not match another resolvable which has a tighter | ||
# bound or an exact version. | ||
return packages + super(CachingResolver, self).package_iterator(resolvable, | ||
existing=existing) | ||
|
||
return super(CachingResolver, self).package_iterator(resolvable, existing=existing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assuming packages == []
in the non-cached case, how would you feel about killing the slight bit of super().package_iterator()
call duplication here (lines 255 + 258) by always returning a combined iterable that includes packages
- whether empty or not - in the main return statement?
e.g.
if self.__cache_ttl:
packages = ...
return itertools.chain(packages, super(...).package_iterator(...))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So something like this?
def package_iterator(self, resolvable, existing=None):
iterator = Iterator(fetchers=[Fetcher([self.__cache])])
packages = self.filter_packages_by_interpreter(
resolvable.compatible(iterator), self._interpreter, self._platform)
if packages:
if resolvable.exact:
return packages
if self.__cache_ttl:
packages = self.filter_packages_by_ttl(packages, self.__cache_ttl)
else:
packages = []
return itertools.chain(packages, super(CachingResolver, self).package_iterator(resolvable, existing=existing))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, you could go a step further and guard the population of packages
altogether by checking for the conditions in which it'd be used - was sort of imagining something along the lines of:
def package_iterator(self, resolvable, existing=None):
packages = []
if self.__cache_ttl or resolvable.exact:
iterator = Iterator(fetchers=[Fetcher([self.__cache])])
packages = self.filter_packages_by_interpreter(
resolvable.compatible(iterator),
self._interpreter,
self._platform
)
if packages:
if resolvable.exact:
return packages
if self.__cache_ttl:
packages = self.filter_packages_by_ttl(packages, self.__cache_ttl)
return itertools.chain(
packages,
super(CachingResolver, self).package_iterator(resolvable, existing=existing)
)
but I'm probably just splitting hairs at this point - fine with any of the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, yah that looks good to me. I'll push up the change. For now I'll do a separate autosquashable commit unless you'd prefer it be a separate commit. Local tests run fine with that change btw :)
merged - thanks for the PR @butlern ! |
Fixes pex-tool#178 The error is as follows: Two resolvables, one pinned, the other not. First pex run with clean cache works. Change the pinned version. Second pex run fails with Unsatisfiable. The problem is that the non-pinned resolvable locates the cached dep and only returns it. Since that version doesn't match the pinned version, we fail. This fixes that behavior by returning both the cached version and all uncached pypi packages for unpinned resolvables. We change the pinned version to test the use case where you have a cached version of a dep and then you transitively pull in another requirement that either has a tighter bound or a pin on a version not found in the cache.
Ref: #178
The error is as follows:
The problem is that the non-pinned resolvable locates the cached dep
and only returns it. Since that version doesn't match the pinned
version, we fail. This fixes that behavior by returning both the
cached version and all uncached pypi packages for unpinned
resolvables.
We change the pinned version to test the use case where you have a
cached version of a dep and then you transitively pull in another
requirement that either has a tighter bound or a pin on a version not
found in the cache.