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

Support projects providing several artifacts #88

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Badya
Copy link
Contributor

@Badya Badya commented Dec 13, 2023

This MR fixes cases when there are several artifacts for different platforms declared by one module (e.g. kotlin-stdlib) using gradle variants

Copy link
Collaborator

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

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

I'll do have to another pass in a bit to fully understand the variant logic

@@ -82,7 +81,7 @@ public class SpdxDocumentBuilder {

private final HashMap<ComponentIdentifier, LinkedHashSet<ComponentIdentifier>> tree =
new LinkedHashMap<>();
private final Map<ComponentIdentifier, File> resolvedExternalArtifacts;
private final Map<ComponentIdentifier, Collection<File>> resolvedExternalArtifacts;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think using List everywhere instead of Collection would be nice. We don't plan on using a non-list anyway

Map<ComponentIdentifier, Collection<File>> results = new HashMap<>();
artifacts.forEach(
(identifier, file) -> {
if (results.containsKey(identifier.getComponentIdentifier())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This section can be compressed down to:

results.getOrDefault(identifier.getComponentIdentifier(), new ArrayList<File>()).add(file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But getOrDefault does not put the defaultValue back into Map

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh right, yeah you have to add it in. oops

Copy link
Collaborator

@loosebazooka loosebazooka Dec 22, 2023

Choose a reason for hiding this comment

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

I think computeIfAbsent is what I actually wanted, it does the insert.

results.computeIfAbsent(identifier.getComponentIdentifier(), l -> new ArrayList<File>()).add(file)

private static Map<ComponentIdentifier, Collection<File>> fromResolvedArtifacts(
Map<ComponentArtifactIdentifier, File> artifacts) {
Map<ComponentIdentifier, Collection<File>> results = new HashMap<>();
artifacts.forEach(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this still needs to filter out project components (for #69)

@@ -289,7 +274,7 @@ private SpdxPackage createProjectPackage(ResolvedComponentResult resolvedCompone
SpdxPackageBuilder builder =
doc.createPackage(
doc.getModelStore().getNextId(IdType.SpdxId, doc.getDocumentUri()),
pi.getName(),
pi.getName(), // TODO: name could be from file in case several files declared
Copy link
Collaborator

Choose a reason for hiding this comment

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

do variants have names? or is it all implicit?


spdxPackages.put(component.getId(), maybePackage.get());
// TODO: support createdPackages list if several packages created
spdxPackages.put(component.getId(), createdPackages.get(0));
Copy link
Collaborator

Choose a reason for hiding this comment

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

is 0 expected to be the "main" variant?

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.

2 participants