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

fix(Scanner): Apply detectedLicenseMapping to FossId findings #7537

Conversation

MarcelBochtler
Copy link
Member

@MarcelBochtler MarcelBochtler commented Sep 19, 2023

While the detectedLicenseMappings are globally applied for the other
scanners, these mappings are not applied for FossId. This is because
FossId works different from other scanners as a "remote" scanner.

Apply the mappings to FossId results as well. This fixes a regression
introduced in cdecb9b.

@MarcelBochtler MarcelBochtler force-pushed the fossid-detected-license-mapping branch from a97eefb to 662757e Compare September 19, 2023 11:47
@MarcelBochtler MarcelBochtler force-pushed the fossid-detected-license-mapping branch 2 times, most recently from 96cdd5c to c5db772 Compare September 19, 2023 11:52
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (2a7d3f5) 68.02% compared to head (c35212f) 68.02%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7537   +/-   ##
=========================================
  Coverage     68.02%   68.02%           
  Complexity     2023     2023           
=========================================
  Files           343      343           
  Lines         16723    16723           
  Branches       2371     2371           
=========================================
  Hits          11375    11375           
  Misses         4364     4364           
  Partials        984      984           
Flag Coverage Δ
test 35.51% <ø> (ø)

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.

@MarcelBochtler MarcelBochtler force-pushed the fossid-detected-license-mapping branch 2 times, most recently from 92cd8af to fb82e67 Compare September 19, 2023 13:28
@MarcelBochtler MarcelBochtler changed the title fix(Scanner): Apply detectedLicenseMapping to package scanners fix(Scanner): Apply detectedLicenseMapping to FossId findings Sep 19, 2023
@MarcelBochtler MarcelBochtler force-pushed the fossid-detected-license-mapping branch from fb82e67 to 1385d27 Compare September 19, 2023 13:29
@MarcelBochtler MarcelBochtler marked this pull request as ready for review September 19, 2023 14:19
@MarcelBochtler MarcelBochtler requested a review from a team as a code owner September 19, 2023 14:19
@sschuberth
Copy link
Member

This fixes a regression introduced in cdecb9b.

Sorry for having broken this. The intention of my commit was to "centralize" the mapping on the scanner-level, I just accidentally only applied it to path scanners. With FossId being a package scanner, can't we do the same centralization for package scanners (or even more ideally, for any scanner) instead of only applying it to FossId?

@MarcelBochtler
Copy link
Member Author

With FossId being a package scanner, can't we do the same centralization for package scanners (or even more ideally, for any scanner) instead of only applying it to FossId?

I tried that. See: https://github.com/oss-review-toolkit/ort/compare/fb82e67c4972fc1e707a3c0735336206bd2e1b74..1385d27ac7ce6d1c22e4dec19e1bea26a704b88d (the removed code).

This was not working in my tests.
I don't see if or which central method FossID is using. That's why I implemented it in FossID directly.

@mnonnenmacher
Copy link
Member

With FossId being a package scanner, can't we do the same centralization for package scanners (or even more ideally, for any scanner) instead of only applying it to FossId?

I tried that. See: https://github.com/oss-review-toolkit/ort/compare/fb82e67c4972fc1e707a3c0735336206bd2e1b74..1385d27ac7ce6d1c22e4dec19e1bea26a704b88d (the removed code).

This was not working in my tests. I don't see if or which central method FossID is using. That's why I implemented it in FossID directly.

I looked at the diff and am really surprised that it didn't work, I don't understand why it shouldn't. I would be fine with merging this as a quick fix, but could you please add a TODO comment that the mapping of licenses should be moved to a central location?

@MarcelBochtler MarcelBochtler force-pushed the fossid-detected-license-mapping branch from 1385d27 to a97d4ea Compare September 21, 2023 10:56
@MarcelBochtler MarcelBochtler enabled auto-merge (rebase) September 21, 2023 14:56
While the `detectedLicenseMapping`s are globally applied for the other
scanners, these mappings are not applied for FossId. This is because
FossId works different from other scanners as a "remote" scanner.

Apply the mappings to FossId results as well. This fixes a regression
introduced in cdecb9b.

Signed-off-by: Marcel Bochtler <[email protected]>
@MarcelBochtler MarcelBochtler force-pushed the fossid-detected-license-mapping branch from a97d4ea to c35212f Compare September 21, 2023 14:59
@MarcelBochtler MarcelBochtler merged commit 0b04df0 into oss-review-toolkit:main Sep 21, 2023
37 checks passed
@MarcelBochtler MarcelBochtler deleted the fossid-detected-license-mapping branch September 21, 2023 15:44
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.

3 participants