feat(spdx): Expose the validation of license text files' existence #9523
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9523 +/- ##
=========================================
Coverage 67.97% 67.97%
Complexity 1290 1290
=========================================
Files 249 249
Lines 8813 8813
Branches 916 916
=========================================
Hits 5991 5991
Misses 2433 2433
Partials 389 389
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| return getLicenseTextReader(id, handleExceptions, licenseTextDirectories) { dir, filename -> | ||
| dir.resolve(filename).takeIf { it.isFile } | ||
| } | ||
| } |
There was a problem hiding this comment.
Have you considered refactoring the function to stop returning a lambda, in favour of the actual text / String?
There was a problem hiding this comment.
No, what would be the motivation to do it ? Just for code cleanup ?
BTW, do you know why it was decided to return a lambda in the first place ?
EDIT: I think @mnonnenmacher wrote the implementation. Could you please share your opinion on this ?
There was a problem hiding this comment.
BTW, do you know why it was decided to return a lambda in the first place ?
Git blame is your friend, the rationale is in the commit message of 45a3087.
There was a problem hiding this comment.
No, what would be the motivation to do it ?
Motivation IMO would be to have a cleaner / simpler (public) API.
The approach to do this lazily could still be kept, but by an ORT internal function.
(To me this API looks a bit more complicated than necessary / not clean enough, which I believe it should be fixed before made public).
I don't understand why exposing something like the following does not suffice (which I believe we already have).
interface LicenseTextReader {
fun getLicenseText(licenseId: String, handleExceptions: Boolean)
}
fun getLicenseTextReader(textDirectories: List<File>)
There was a problem hiding this comment.
@fviernau I think this is already the case. The public function hides the lambda return type https://github.com/boschglobal/oss-review-toolkit/blob/ed2f67ed59a0d7970ab7d4efb0767622cef05659/utils/spdx/src/main/kotlin/Utils.kt#L113.
There was a problem hiding this comment.
This PR exposes return type (() -> String)? of getLicenseTextReader(), doesn't it ?
There was a problem hiding this comment.
Yes, but this is because the LicenseTextProvider.getLicenseTextReader also exposes this return type:
getLicenseText -> String?
getLicenseTextReader -> (() -> String)?
Therefore this is consistent with the current state of the implementation.
ORT has a complicate logic to search for a file containing the full license text of a given license: several search paths are tried. Unfortunately, the latter are not exposed, making it hard for an alternative `LicenseTextProvider` to have the same logic. Additionally, the current code assumes that the license files are on a local storage and the I/Os are therefore cheap. To allow the current utility function to be used with a `LicenseTextProvider` that works with license text files stored on a network storage, this commit exposes the validation function. Signed-off-by: Nicolas Nobelis <[email protected]>
nnobelis
force-pushed
the
nnobelis/license_text_loading
branch
from
4537bc8 to
ed2f67e
Compare
I totally agree to the described limitation, and that the current code with top-level functions is really ugly and hard to maintain. That's why I started a while ago to think about a "license text provider API" where implementations should be done as plugins. Now that we have the KSP-based plugin architecture, it's probably a good time to revive that effort and implement it before making further changes to this code area. |
|
@sschuberth Even if the current License Test Providers is migrated to a plugin architecture, you would still need to decouple the
and in parallel you would have a core engine that processes the paths according to their priority, and call the processing functions. However, we should discuss that with the Core developers in tech meeting. The question is: How to move forward now ? |
|
This PR is not needed by the ORT Server anymore: See also #9620 for the continuation of the discussion. |
ORT has a complicate logic to search for a file containing the full license text of a given license: several search paths are tried. Unfortunately, the latter are not exposed, making it hard for an alternative
LicenseTextProviderto have the same logic. Additionally, the current code assumes that the license files are on a local storage and the I/Os are therefore cheap. To allow the current utility function to be used with aLicenseTextProviderthat works with license text files stored on a network storage, this commit exposes the validation function.