-
Notifications
You must be signed in to change notification settings - Fork 133
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
Fix up a few filesAnalyzed issues #195
Conversation
Looks like circleci stuff on mac is broken. I'm not quite sure how to fix it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent catch!
Thank you ++
Could I interest you in adding a unit test too to avoid regression?
And sorry if I did update your branch by mistake with the latest. Let's pretend it was because of fat fingers. |
Yes, of course. I'll add some tests. |
@lhh Thank you! you rock. |
I am chasing possibly a bug in the RDF writer not handling multiple packages properly (particularly when one is filesAnalyzed = false), which is why that and the tag/value writer tests are not present yet. My apologies for the delay. I'm out for a few days. |
Should |
@dholth this code is a source of troubles more often than not. We should make it optional in this library IMHO. |
@lhh, do you still plan on updating this PR? |
Ah, yes - my apologies. I'll address the above issues shortly. |
d4a1415
to
03788d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pombredanne you still have a blocker on this PR. As I don't want to dismiss your review, would you care to have a short look at it again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left two minor questions. Thanks for the contribution!
When filesAnalyzed is False, several sections should be omitted when writing rather than being presented as empty sets or "None". SPDX 2.2, sec: 3.8.3 - files analyzed specification 3.9.3 - verification code 3.14.3 - licenses from files Without doing this, invalid SPDX documents are written and cannot be parse by the parser. Signed-off-by: Lon Hohberger <lhh@redhat.com>
xmltodict is used to generate a Python dictionary from XML tags. Unfortunately, it does not transmogrify CDATA "true" or "false" to their Python bool representation, so we must do it ourselves. Add this to the XML example for testing purposes. Signed-off-by: Lon Hohberger <lhh@redhat.com>
Reading in SPDX BOMs with multiple packages produces a list of dictionaries, which cannot be sorted (throws a TypeError) Signed-off-by: Lon Hohberger <lhh@redhat.com>
When multiple packages are present, some may have filesAnalyzed set to True, others set to False. Make sure we test both cases. Signed-off-by: Lon Hohberger <lhh@redhat.com>
The SPDX specification holds that the package verification code is mandatory whenever files_analyzed is True or none (omitted), however, for the purposes of this library, we make it optional here. Signed-off-by: Lon Hohberger <lhh@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my questions! I didn't look at the remaining code in detail, but two other reviewes already approved, so this should be good to merge
No description provided.