Fix auto-instrumentation dependency conflict detection #530
Conversation
owais
changed the title
owais
force-pushed
the
instrumentation-check-dep-conflicts-without-markers
branch
3 times, most recently
from
f488345 to
6d3e4c5
Compare
owais
requested review from
a team,
codeboten and
srikanthccv
and removed request for
a team
|
|
||
|
|
||
| def get_dependency_conflicts( | ||
| deps: Collection[str], | ||
| ) -> Optional[DependencyConflict]: | ||
| for dep in deps: | ||
| try: | ||
| get_distribution(str(dep)) | ||
| get_distribution(dep) |
There was a problem hiding this comment.
Is it worth adding a catch all here to prevent any other exceptions from causing the script to exit unexpectedly?
There was a problem hiding this comment.
We could but I don't think there is anything too special about this and I expect it to work always unless in truly exceptional cases like the filesystem misbehaving or something like that.
There was a problem hiding this comment.
Added an additional RequirementParseError that should catch most issues with invalid requirement strings being passed to this func.
| return get_dependency_conflicts(deps) | ||
| main_deps = dist.requires() | ||
| instrumentation_deps = [] | ||
| for dep in dist.requires(("instruments",)): |
There was a problem hiding this comment.
Just to clarify, this would generate the Requirements for whatever is in extra_requires["instruments"] IN ADDITION TO the regular install_requires correct? In this case, the logic underneath will only parse through the ones that are not in the regualr install_requires?
There was a problem hiding this comment.
Correct. pkg_resource does not have an API to fetch only a specific extra require group. We could have used something like dist.requires(only_extra=('instruments',)) but it doesn't exist :)
|
|
||
|
|
||
| def get_dependency_conflicts( | ||
| deps: Collection[str], | ||
| ) -> Optional[DependencyConflict]: | ||
| for dep in deps: | ||
| try: | ||
| get_distribution(str(dep)) | ||
| get_distribution(dep) |
There was a problem hiding this comment.
What was the original issue behind this? get_distribution behavior if the dep was not installed?
There was a problem hiding this comment.
It cannot parse requirement strings that might also contain markers (extra requires section name).
get_distribution("lib == 1.0") works but get_distribution('lib == 1.0; extra = "instruments"') does not.
owais
force-pushed
the
instrumentation-check-dep-conflicts-without-markers
branch
6 times, most recently
from
b189239 to
eef7a13
Compare
owais
force-pushed
the
instrumentation-check-dep-conflicts-without-markers
branch
from
eef7a13 to
efc735d
Compare
Description
Fixes #529
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.