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

reporter: Add a very basic SpdxDocumentReporter #2800

Merged
merged 2 commits into from
Jun 30, 2020
Merged

Conversation

fviernau
Copy link
Member

Add an initial SPDX document reporter which for now only creates the
document header. Logic for serializing additional data will be added
incrementally in following changes.

Signed-off-by: Frank Viernau frank.viernau@here.com

@tsteenbe
Copy link
Member

@fviernau Could you add a debug statement to show how long it took to generate the report? Like the other reporter have, see for example https://github.com/oss-review-toolkit/ort/blob/master/reporter/src/main/kotlin/reporters/EvaluatedModelReporter.kt#L68

@sschuberth
Copy link
Member

sschuberth commented Jun 30, 2020

Could you add a debug statement to show how long it took to generate the report?

That shouldn't be necessary as the generation of any report is timed.

@fviernau fviernau force-pushed the basic-spdx-reporter branch 3 times, most recently from 957b11c to 7df851c Compare June 30, 2020 09:51
To prepare for an upcoming change.

Signed-off-by: Frank Viernau <frank.viernau@here.com>
@fviernau fviernau force-pushed the basic-spdx-reporter branch from 7df851c to b9d0089 Compare June 30, 2020 10:07
@fviernau fviernau force-pushed the basic-spdx-reporter branch 2 times, most recently from c29ade7 to cac7349 Compare June 30, 2020 10:25
@fviernau fviernau requested a review from sschuberth June 30, 2020 10:25
sschuberth
sschuberth previously approved these changes Jun 30, 2020
@fviernau fviernau dismissed mnonnenmacher’s stale review June 30, 2020 10:40

All change requests addressed

Add an initial SPDX document reporter which for now only creates the
document header. Logic for serializing additional data will be added
incrementally in following changes.

Signed-off-by: Frank Viernau <frank.viernau@here.com>
@fviernau fviernau force-pushed the basic-spdx-reporter branch from 8915e7f to 6a285dd Compare June 30, 2020 12:25
Copy link

@swinslow swinslow left a comment

Choose a reason for hiding this comment

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

I happened to see this linked from another issue in spdx/spdx-spec -- added a couple of comments re: the Document Creation Info section in the generated output samples.

@@ -0,0 +1,14 @@
{
"SPDXID" : "SPDXRef-<REPLACE_UUID>",

Choose a reason for hiding this comment

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

The SPDXID for the document itself should always be the literal text "SPDXRef-DOCUMENT" -- see https://spdx.github.io/spdx-spec/v2-draft/document-creation-information/#63-spdx-identifier-field

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @swinslow, greatly appreciated!
I'll merge the PR as is now, and then address your comment(s) in a follow up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@swinslow - I've addressed your findings with #2812 .

Copy link

Choose a reason for hiding this comment

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

LGTM, thanks @fviernau!

@@ -0,0 +1,14 @@
{
"SPDXID" : "SPDXRef-<REPLACE_UUID>",
"spdxVersion" : "SPDX-2.2.1",

Choose a reason for hiding this comment

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

The format for the SPDX version field is SPDX-M.N -- the major and minor versions are included, but not the point releases if there is one -- see https://spdx.github.io/spdx-spec/v2-draft/document-creation-information/#61-spdx-version-field

So this should just be "SPDX-2.2", even after the 2.2.1 release of the spec comes out. The intention is that the point release 2.2.1 is not changing anything substantive in the spec, so SPDX 2.2.1 documents should be identical to 2.2.

@@ -0,0 +1,13 @@
---
SPDXID: "SPDXRef-<REPLACE_UUID>"

Choose a reason for hiding this comment

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

same comment as in JSON file, the value here should be SPDXRef-DOCUMENT

@@ -0,0 +1,13 @@
---
SPDXID: "SPDXRef-<REPLACE_UUID>"
spdxVersion: "SPDX-2.2.1"

Choose a reason for hiding this comment

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

same comment as in JSON file, the value here should be SPDX-2.2

@fviernau fviernau merged commit 0695f55 into master Jun 30, 2020
@fviernau fviernau deleted the basic-spdx-reporter branch June 30, 2020 14:31
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.

5 participants