Conversation
spiffcs
left a comment
There was a problem hiding this comment.
Glad to see the relationship index being useful over in other ecosystems!
I don't have any comments blocking this to merge. I tried it on a couple of anchore projects written in python to get an idea of the before and after for what new relationships show up and it all LGTM as far as what was expected to be added appeared with this branch.
The schema bump is 🟢 so no comments there.
This comment was covered in the summary. There was a new processExecutor private interface added with a private function.
So resolvingProcessorWrapper implements processorWrapper which embeds the public ResolvingProcessor
I agree that it's complex and took me a little while to get a mental model of how everything behaved. I like how it works now and can get on board with the tradeoffs made here.
There was a dependency_test.go stubbed out syft/pkg/cataloger/python/dependency_test.go did you mean to go back and fill this out or delete it?
I tried a couple different configurations for virtual env and found syft/pkg/cataloger/python/virtual_env.go worked, but suspect if there is anything I missed it's in this file as I don't have a great mental model of all python virtual env configurations or if there is something missing here.
indeed, I was going to add a test here but then realized the code in question was already covered... however, I have a follow up PR #2906 that will add a test in this file, so I'll leave this for now. |
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
b05f536 to
27eee8c
Compare
|
note: force pushed to rebased on main (resolving conflicting schema changes) |
Partially implements #572
Adds dependency relationships between python packages installed within site-package directories. This only associates packages that are either:
This adds indications of
Requires-Dist,Provides-Extra, andRequires-Pythonto the existingpkg.PythonPackagemetadata struct.The
extrasfield is only partially supported, in that, if packageAhas an extra declared that will install packageBconditionally, and packageBis also found to be installed, then there is a relationship explicitly created that linksB dependency-of A, even ifAwas not installed with any extras. There are a couple of reasons to do this:What is not supported in this PR is validating any version constraints from any
Requires-Distdeclaration... if the package is installed, then it is assumed that the virtual environment is valid and consistent... sopipor other tooling has already performed to correct version constraint processing during installation.Regarding changes made to
cataloger/generic: In this particular instance we could not use exclusively material that was already present on packages, we needed to gather additional material via thefile.Resolverto do so. Additionally, this materialpyenv.cfgis not evidence for any single package, but instead configuration for an entire virtual environment, so didn't belong as additional data attached to package metadata. For this reason a new pattern was added next togeneric.Processor, calledgeneric.ResolvingProcessoras to not make any breaking changes.I did decide that users should be able to specify both
Processors andResolvingProcessors in the same slice of post-cataloging processor functions, so that callers can closely control the order between these two kinds of processors. This lead to additional non-exported interfaces to facilitate this (and added more complexity than I wanted) but seemed like the best way to achieve the stated goals.Lastly, this rearranges some
internalfunctionality for necessary re-use (specifically using therelationship.Indexcapability). This caused additional utility functions to be moved because of this (otherwise it would be calledbinary.RelationshipIndexwhich makes no sense).