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

Assorted bug fixes #263

Closed
wants to merge 15 commits into from
Closed

Conversation

nicoweidner
Copy link
Collaborator

@nicoweidner nicoweidner commented Nov 2, 2022

Taken over from #207.

There were some conflicts when rebasing on main. I tried to make minimal changes, only fixing code that was obviously wrong after resolving conflicts. I will add additional changes in separate commits.

NOTE: Since signoffs of original authors are missing, I will redo this PR from scratch.

@nicoweidner nicoweidner mentioned this pull request Nov 2, 2022
@nicoweidner nicoweidner self-assigned this Nov 2, 2022
tamari-oz and others added 13 commits November 7, 2022 11:59
1. yaml creation missing encoding.
2. NoAssert cannot be written to a file (not serializable) as it does not have default repr.
3. Problem creating relationships

Signed-off-by: OzTamari <[email protected]>
1. yaml creation missing encoding.
2. NoAssert cannot be written to a file (not serializable) as it does not have default repr.
3. Problem creating relationships

Signed-off-by: OzTamari <[email protected]>
…ue is not available at WhiteSource

Updating setup.py version to '0.7.0a3.post6'

Signed-off-by: rammatzkvosky <[email protected]>
Signed-off-by: Nicolaus Weidner <[email protected]>
Copy link
Collaborator

@meretp meretp left a comment

Choose a reason for hiding this comment

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

Thanks for continuing this PR!
I have a few minor comments, but I'm not sure if the rebase will take care of some of it. I haven't looked at the changes to the example files yet, because I think that will change more with the rebase.

@@ -112,3 +118,5 @@ def validate_relationship(self, messages):
"Relationship type must be one of the constants defined in "
"class spdx.relationship.Relationship"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we use these two functions if the validate method only calls the validate_relationship method? IMHO we should only use the validate_relationship method.

package_objects.append({"Package": package_info_object})
# create a list of the packages' ids for the 'documentDescribes' field
package_spdx_id = package_info_object.get("SPDXID")
document_describes_objects.append(package_spdx_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this we will add all packages at document level to the list of documentDescribes SPDXIDs. As far as I understood only the packages which are part of a DESCRIBES-Relationship should be listed here. I am not sure if the rebase will tackle this already.

Comment on lines +1019 to +1025
# Add relationship
for relate_node in self.relationships():
relate_triple = (doc_node, self.spdx_namespace.relationship, relate_node)
self.graph.add(relate_triple)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did the same as in your comment here and still got the same result. I think this should be fixed before merging this PR.

Signed-off-by: Nicolaus Weidner <[email protected]>
Comment on lines +65 to +67
"ns1:relationship": {"ns1:Relationship": {"@rdf:about": "SPDXRef-DOCUMENT",
"ns1:relatedSpdxElement": "SPDXRef-Package",
"ns1:relationshipType": "DESCRIBES"}},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A comment about these additions:
This structure looks reasonable to me and is apparently what our rdf writer produces at the moment. It may not be what the spec intends, though:

The structure in e.g. this example is different: The related element is usually expanded (making the whole thing completely unreadable), and there doesn't seem to be a reference to the current element (maybe relationships are put on the originating object instead of the top-level document?).

Considering that there are other oddities in this test file as well (e.g. referencesFile and describesPackage doesn't exist in example files from the spec or java tools), and the documentation on the desired state is unclear, I'd leave it at this for now. I will ask on the ML what's going on with rdf...

Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I know, elements in rdf files are normally expanded at the first place they appear (which does get quite muddy, yeah).

@meretp meretp mentioned this pull request Nov 9, 2022
@meretp
Copy link
Collaborator

meretp commented Nov 9, 2022

Continued in #270.

@meretp meretp closed this Nov 9, 2022
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.

7 participants