Map unmatched node_modules files - #796
Conversation
65dd14b to
f5bf95d
Compare
|
@keshav-space is this PR complete and ready to be reviewed? |
@tdruez PR is complete and ready for review. |
pombredanne
left a comment
There was a problem hiding this comment.
See some nits for your review.
|
Also can you elaborate in the body of the PR what this is doing? |
Signed-off-by: Keshav Priyadarshi <git@keshav.space>
Signed-off-by: Keshav Priyadarshi <git@keshav.space>
Signed-off-by: Keshav Priyadarshi <git@keshav.space>
293ddc5 to
9bd1fc7
Compare
Signed-off-by: Keshav Priyadarshi <git@keshav.space>
9bd1fc7 to
adfe55a
Compare
| if package: | ||
| package.add_resources(matched) | ||
| else: | ||
| if results := purldb.fetch_package(purl=str(purl)): |
There was a problem hiding this comment.
I'm not sure about depending on a purldb call deep within a d2d pipe.
@pombredanne what's your take on this architecture? We've only implemented pipes and steps dedicated to purldb so far.
There was a problem hiding this comment.
How is this different? Is this not mostly a purldb step?
There was a problem hiding this comment.
Or would you want to have a separate step?
@tdruez what alternative approach would you see?
There was a problem hiding this comment.
Ok I get the problem.... @keshav-space @tdruez here my suggestion:
- We have directory and we get an npm PURL from this. If we are reaching here, the package instance for this directory does not exist IMHO. We should not reuse randomly the first package.
- We should then create a new "skinny" package based on the PURL and assign the resources to it, with extra care for possible nested "node_modules".
- If somehow, we have package.json data available nearby on disk, we should use this to enhance the package data
- As a completely and new separate step, we could lookup in the PURL for some packages that are missing some details, like license and similar, BUT not in this step. This should be designed carefully in a new issue. Do we want to "enhance" all the packages with PURLDB data? Or is this only for npms? Which field do we update? etc... See Enhance Discovered Packages with PurlDB data #869 to track this
Signed-off-by: Keshav Priyadarshi <git@keshav.space>
|
|
||
|
|
||
| def get_purl_from_node_module(node_module_directory): | ||
| """Return PURL for given a `node_modules` package directory.""" |
There was a problem hiding this comment.
Can we have some examples of the paths in the docstring?
pombredanne
left a comment
There was a problem hiding this comment.
Here is some extre feedback.
| project, | ||
| ): | ||
| """Map unmatched ``node_modules`` files.""" | ||
| purl = js.get_purl_from_node_module(to_directory.path) |
There was a problem hiding this comment.
This line should come after "if not matched" below at https://github.com/nexB/scancode.io/pull/796/files#diff-5b5fd8c312dacbb59edb7017b5e11bfb2809d756ae5632d7f48a4e3488c6b0d8R1063
| namespace="" if not purl.namespace else purl.namespace, | ||
| name=purl.name, | ||
| version=purl.version, | ||
| ).first() |
There was a problem hiding this comment.
this first() call may be problematic.... this may be a package that is for completely unrelated resources, and we may end-up assigning the wrong resources to a package or the resources to the wrong package
There was a problem hiding this comment.
Also since we are doing something to create a package, would we ever be in a case where we have an existing package for the same resources that exists? IMHO it would never exist, otherwise why would be doing this work in the first place?
| ): | ||
| """Map unmatched ``node_modules`` files.""" | ||
| purl = js.get_purl_from_node_module(to_directory.path) | ||
| matched = to_resources.filter(path__startswith=to_directory.path) |
There was a problem hiding this comment.
What if we have nested node_modules directories> the nested sub-dirs are for different packages
| if package: | ||
| package.add_resources(matched) | ||
| else: | ||
| if results := purldb.fetch_package(purl=str(purl)): |
There was a problem hiding this comment.
Ok I get the problem.... @keshav-space @tdruez here my suggestion:
- We have directory and we get an npm PURL from this. If we are reaching here, the package instance for this directory does not exist IMHO. We should not reuse randomly the first package.
- We should then create a new "skinny" package based on the PURL and assign the resources to it, with extra care for possible nested "node_modules".
- If somehow, we have package.json data available nearby on disk, we should use this to enhance the package data
- As a completely and new separate step, we could lookup in the PURL for some packages that are missing some details, like license and similar, BUT not in this step. This should be designed carefully in a new issue. Do we want to "enhance" all the packages with PURLDB data? Or is this only for npms? Which field do we update? etc... See Enhance Discovered Packages with PurlDB data #869 to track this
|
closing this in favour of #881 |
Map the 3rd-party files inside
node_modulesthat couldn't be matched to PurlDB due to slight modifications.