Skip to content
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

feat: add npm sbom command #6801

Merged
merged 1 commit into from
Sep 27, 2023
Merged

feat: add npm sbom command #6801

merged 1 commit into from
Sep 27, 2023

Conversation

bdehamer
Copy link
Contributor

Adds a new sbom command to generate a Software Bill of Materials (SBOM) for the current project.

No new dependencies are introduced, however normalize-package-data and spdx-expression-parse are elevated to top-level dependencies.

References

Original RFC: npm/rfcs#714

@wraithgar
Copy link
Member

This looks good from a code standpoint. Waiting on a rough consensus in the rfc before landing.

We'll land this, give it a few weeks in case there are implementation bugs we missed, update the RFC to reflect those if needed, then land the RFC.

Signed-off-by: Brian DeHamer <bdehamer@github.com>
Copy link

@maxhbr maxhbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for SPDX the directions of the relationships are not consistent (since SPDX provides sometimes both directions, like DEPENDS_ON and DEPENDENCY_OF)

const REL_PREREQ = 'HAS_PREREQUISITE'
const REL_OPTIONAL = 'OPTIONAL_DEPENDENCY_OF'
const REL_DEV = 'DEV_DEPENDENCY_OF'
const REL_DEP = 'DEPENDS_ON'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to double check, but the direction might be wrong:

Suggested change
const REL_DEP = 'DEPENDS_ON'
const REL_DEP = 'DEPENDENCY_OF'

At least the other relationships are in the other direction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maxhbr I was chatting with one of the SPDX contributors about this and the guidance I got was that the "directionality" of the relationship is implied by the spdxElementId/relatedSpdxElement fields not by the relationshipType -- many of the types don't have a supported inverse, so I just picked whichever one best conveys the type of the relationship w/o regard for the direction.

Perhaps, I should switch this one just to be consistent, but it was the one case where there was a supported type that actually matched the direction of the relationship.

My understanding is that this is going to get better in SPDX 3.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the directionality is implied by the spdxElementId/relatedSpdxElement fields, in combination with the type of the relationship.

The following two relationships are equivalent

spdxElementId: PackageA
relatedSpdxElement: PackageB
relationshipType: DEPENDS_ON

and

spdxElementId: PackageB
relatedSpdxElement: PackageA
relationshipType: DEPENDENCY_OF

The direction of the type of relationship is still significant.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your case probably the DEPENDS_ON is the right choice, and the other mappings should be adopted.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to clarify the comment above:

this does not resolve the issue. It is just a statement that, when choosing between DEPENDS_ON and DEPENDENCY_OF the first should probably be preferred (aligning with #6867 (comment)).

But still the issue is valid and the relationshipTypes are inconsistently used here.

@wraithgar wraithgar merged commit 7c459d2 into latest Sep 27, 2023
48 of 54 checks passed
@wraithgar wraithgar deleted the bdehamer/sbom branch September 27, 2023 19:37
@github-actions github-actions bot mentioned this pull request Sep 27, 2023
bdehamer added a commit that referenced this pull request Sep 27, 2023
Signed-off-by: Brian DeHamer <bdehamer@github.com>
bdehamer added a commit that referenced this pull request Sep 27, 2023
Signed-off-by: Brian DeHamer <bdehamer@github.com>
bdehamer added a commit that referenced this pull request Oct 6, 2023
Signed-off-by: Brian DeHamer <bdehamer@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants