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

Improvements to make AOSD reports better comparable #9615

Merged
merged 3 commits into from
Dec 16, 2024
Merged

Conversation

sschuberth
Copy link
Member

Please have a look at the individual commit messages for the details.

@sschuberth sschuberth requested a review from a team as a code owner December 16, 2024 07:44
@sschuberth sschuberth enabled auto-merge (rebase) December 16, 2024 07:44
@sschuberth sschuberth changed the title Improvmenets to make AOSD reports better comparable Improvements to make AOSD reports better comparable Dec 16, 2024
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.02%. Comparing base (6db8eae) to head (b4dd81d).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #9615   +/-   ##
=========================================
  Coverage     68.02%   68.02%           
  Complexity     1285     1285           
=========================================
  Files           249      249           
  Lines          8828     8828           
  Branches        921      921           
=========================================
  Hits           6005     6005           
  Misses         2433     2433           
  Partials        390      390           
Flag Coverage Δ
funTest-docker 64.96% <ø> (ø)
funTest-non-docker 33.31% <ø> (ø)
test-ubuntu-24.04 35.80% <ø> (ø)
test-windows-2022 35.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…thing

Omit the `selectedLicense` if it is the same as `spdxId` and might still
offer a choice. Also, an absent `selectedLicense` should be encoded as
the empty string, not as SPDX `NOASSERTION`.

Signed-off-by: Sebastian Schuberth <[email protected]>
@@ -62,6 +62,7 @@ class Aosd21Reporter(override val descriptor: PluginDescriptor = Aosd21ReporterF
indexedPackages[it]?.index?.toLong()
},
components = indexedPackages.toComponents(input, project)
.toSortedSet(compareBy { it.componentName })
Copy link
Member

Choose a reason for hiding this comment

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

There was this larger effort to eliminate the use of sorted collections in favor of using annotation to do the sorting for the serialization only. I believe we should annotate the components property instead.

Copy link
Member Author

@sschuberth sschuberth Dec 16, 2024

Choose a reason for hiding this comment

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

Please note that the model still only uses a Set, not a SortedSet. I.e. users of the data model are not forced to use a sorted set, esp. when only using the data model in memory. And with my change even users that want to serialize are not forced to use a sorted set. In that sense my change is even less strict than enforcing sorting on any serialization.

Copy link
Member

Choose a reason for hiding this comment

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

Please note that the model still only uses a Set, not a SortedSet.

Was aware of that.

And with my change even users that want to serialize are not forced to use a sorted set.

I suppose with users your speaking of callers from within the same component. It's not obvious to me
whether there is need for using different orderings within the reporter.

...anyhow, I thought the annotation approach would be favorable by default, because it yields a more deterministic serialization.

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway, I'm now sorting on serialization so that files can always be easily compared, even if the data model would be serialized by another caller.

@@ -11,8 +11,8 @@
],
"components": [
{
"id": 0,
"componentName": "no-license-file",
"id": 3,
Copy link
Member

Choose a reason for hiding this comment

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

commit message: Why is it favorable to align with [1] ?

Copy link
Member Author

@sschuberth sschuberth Dec 16, 2024

Choose a reason for hiding this comment

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

As said in the title: To more easily compare the JSON output files from both tools with a diff tool, while focusing on real differences.

Copy link
Member

Choose a reason for hiding this comment

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

I had read that title of course. To me it wasn't clear the motivation behind being able to compare to that tool?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've also added to the commit message that spdx2aosd actually is a tool that's officially used by Audi.

@sschuberth sschuberth requested review from fviernau and a team December 16, 2024 11:47
@sschuberth sschuberth disabled auto-merge December 16, 2024 11:55
@sschuberth sschuberth enabled auto-merge (rebase) December 16, 2024 12:09
fviernau
fviernau previously approved these changes Dec 16, 2024
This eases comparison with the files generated by [1], which is a tool
officially used by Audi.

[1]: https://gitlab.com/digitalwerk/tools/spdx2aosd

Signed-off-by: Sebastian Schuberth <[email protected]>
This eases file comparison. Do not trim leading whitespace as some
license texts (like for `Apache-2.0`) actually start with indentation.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth merged commit a100dcb into main Dec 16, 2024
26 checks passed
@sschuberth sschuberth deleted the aosd-comparison branch December 16, 2024 14:55
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