resolvelib: Stop resolving requirements one-by-one #488
Conversation
|
Still need to fix tests + address a few things. |
|
This is what it looks like when I audit the same requirements file from earlier: (env) tetsuo@Alexs-MacBook-Pro pip-audit % pip-audit -v -r dependency-resolution-test.txt
DEBUG:pip_audit._cli:parsed arguments: Namespace(local=False, requirements=[<_io.TextIOWrapper name='dependency-resolution-test.txt' mode='r' encoding='UTF-8'>], project_path=None, format=<OutputFormatChoice.Columns: 'columns'>, vulnerability_service=<VulnerabilityServiceChoice.Pypi: 'pypi'>, dry_run=False, strict=False, desc=<VulnerabilityDescriptionChoice.Auto: 'auto'>, cache_dir=None, progress_spinner=<ProgressSpinnerChoice.On: 'on'>, timeout=15, paths=[], verbose=1, fix=False, require_hashes=False, index_url='https://pypi.org/simple/', extra_index_urls=[], skip_editable=False, no_deps=False, output=PosixPath('stdout'), ignore_vulns=[])
DEBUG:pip_audit._cli:Auditing flask (1.0)
DEBUG:pip_audit._cli:Auditing jinja2 (2.10)
DEBUG:pip_audit._cli:Auditing itsdangerous (2.1.2)
DEBUG:pip_audit._cli:Auditing click (8.1.3)
DEBUG:pip_audit._cli:Auditing werkzeug (2.2.2)
DEBUG:pip_audit._cli:Auditing markupsafe (2.1.2)
Found 2 known vulnerabilities in 1 package
Name Version ID Fix Versions
------ ------- -------------- ------------
jinja2 2.10 PYSEC-2021-66 2.11.3
jinja2 2.10 PYSEC-2019-217 2.10.1So it audits Jinja2 once and uses the correct version. |
tetsuo-cpp
changed the base branch from
main
to
alex/pip-requirements-parser-fix
|
This change requires #499. |
| skip_reason = str(e) | ||
| logger.debug(skip_reason) | ||
| self.skip_deps.append(SkippedDependency(name=identifier, skip_reason=skip_reason)) | ||
| return |
There was a problem hiding this comment.
@woodruffw I just found a serious problem here as I was fixing unit tests. The fact that we record the skipped candidate is fine, but then we proceed to return 0 candidates for a given requirement. This causes the resolver to raise a ResolutionImpossible.
In the past, we just skipped the top-level requirement and then try resolving the next one (since it was doing one at a time). But the resolver is "all or nothing" so we can't really get partial results from it. And once we get into find_matches, I don't think there's a way to backtrack and say "nevermind, just forget about that requirement I said I needed earlier". I'm at a bit of a loss figuring out how to fix this.
Do you have any ideas? I was considering doing something like creating a dummy candidate without any dependencies just to get past the resolver.
There was a problem hiding this comment.
Yeah, this is reminding me why I thought "one at a time" made sense despite not accurately reflecting the actual dependency resolution process 😅
IMO a dummy candidate here is fine, but we should probably tread carefully -- it'd be good to have it be a separate type entirely similarly to how we handle SkippedDependency (SkippedCandidate maybe?).
|
Oops, not quite ready yet. I'm missing a bit of coverage. I'll try to do that later tonight. One caveat here. I'm thinking this might not be 100% correct yet because it still processes requirements files one-by-one. I'm not sure how This patch gets us most of the way there but there still needs some work to fix up the caching to not work on a file basis. This fixes most cases so I'm proposing to get this in first and cut a release before fixing the multiple file case. |
Yeah, I believe it gathers them all and resolves them in a single shot (for exactly the reason you said.)
Strong agree! Multiple requirements inputs are probably only used by a small minority of users, so we shouldn't block a fix for them on a larger fix like this one 🙂 |
| if isinstance(candidate, SkippedCandidate): | ||
| return True |
There was a problem hiding this comment.
Nit: IMO we should have a small comment here and in get_dependencies explaining why we do this (SkippedCandidate already explains this, so maybe just a ref telling the reader to look at the docstring there?)
## [2.5.1] ### Fixed * Fixed a crash on Windows caused by multiple open file handles to input requirements ([#551](pypa/pip-audit#551)) ## [2.5.0] ### Changed * Improved error messaging when a requirements input or indirect dependency has an invalid (non-PEP 440) requirements specifier ([#507](pypa/pip-audit#507)) * `pip-audit`'s handling of dependency resolution has been significantly refactored and simplified ([#523](pypa/pip-audit#523)) ### Fixed * Fixed a potential crash on invalid unicode in subprocess streams ([#536](pypa/pip-audit#536)) ## [2.4.15] **YANKED** ### Fixed * Fixed an issue where hash checking would fail when using third-party indices ([#462](pypa/pip-audit#462)) * Fixed the behavior of the `--skip-editable` flag, which had regressed with an internal API change ([#499](pypa/pip-audit#499)) * Fixed a dependency resolution bug that can potentially be triggered when multiple packages have the same subdependency ([#488](pypa/pip-audit#488))
Closes #463