Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

scanner: Add Nomos plugin #10631

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

Open
Prakash-Mishra-9ghz wants to merge 1 commit into oss-review-toolkit:main
base: main
Choose a base branch
Loading
from Prakash-Mishra-9ghz:nomos

Conversation

Copy link

@Prakash-Mishra-9ghz Prakash-Mishra-9ghz commented Jul 20, 2025

This pull request introduces a new external scanner plugin for integrating FOSSology's Nomos license scanner into the OSS Review Toolkit (ORT).

🔹 What's implemented

  • Nomos is implemented as an ExternalScanner.
  • Supports command-line execution of Nomos and parses its output.
  • Converts results into ORT's ScanResult model.

🔹 File breakdown

  • Nomossa.kt: Main scanner class implementing the Scanner interface.
  • NomossaConfig.kt: Loads configuration for invoking Nomos CLI.
  • NomossaResultParser.kt: Parses Nomos output to extract license info.
  • NomossaResultExtension.kt: Converts parsed data into ScanResult.
  • build.gradle.kts: Sets up plugin dependencies and build configuration.

This plugin allows users to plug FOSSology’s Nomos agent into ORT workflows to enhance license detection coverage and flexibility.

Copy link
Member

Commit message title: According to conventional commits, the title should start with feat(scanner):.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link

codecov bot commented Jul 20, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.56%. Comparing base (bf85ad1) to head (9dbf5d9).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files 
@@ Coverage Diff @@
## main #10631 +/- ##
============================================
- Coverage 57.58% 57.56% -0.02% 
- Complexity 1704 1709 +5 
============================================
 Files 346 346 
 Lines 12832 12850 +18 
 Branches 1212 1215 +3 
============================================
+ Hits 7389 7397 +8 
- Misses 4978 4988 +10 
 Partials 465 465
Flag Coverage Δ
test-ubuntu-24.04 42.33% <ø> (+<0.01%) ⬆️
test-windows-2025 42.31% <ø> (+<0.01%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@Prakash-Mishra-9ghz, please have a look at these issues and make all automated checks pass before we start with the actual code review.

Copy link
Author

detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@Prakash-Mishra-9ghz, please have a look at these issues and make all automated checks pass before we start with the actual code review.

Resolved all Detekt issues

Copy link
Member

@Prakash-Mishra-9ghz we have a policy to fix any issues identified during review / by automated checks in the actual commit that introduces the issue as long as the PR is still under review.

As your PR originally only had a single commit, this means to just amend that original commit with any updates. So please squash the two commits that you have now into just one again.

Copy link
Author

Prakash-Mishra-9ghz commented Jul 22, 2025 •
edited
Loading

@sschuberth I added the following copyright header: Copyright (C) 2025 Prakash Mishra. The CI failed due to this not being in the NOTICE file. Should I add myself there, or just remove the header?

Copy link
Member

The CI failed due to this not being in the NOTICE file. Should I add myself there, or just remove the header?

You should add yourself to the NOTICE file (grouped by year, then sorted alphabetically) and keep the generic Copyright header in the source file.

Copy link
Author

hi @sschuberth could you help me to figure out the funtest-docker, test(unbuntu and windows) fail.
I think it might be due to missing tests or some configuration issue, but I’m not exactly sure.
Could you help me understand what’s going wrong?

Copy link
Member

hi @sschuberth could you help me to figure out the funtest-docker, test(unbuntu and windows) fail.

Those are temporary failures due to foojayio/discoapi#124, which had been fixed, but now is back again... we simply need to wait for that external service to recover. Just rebase your PR onto latest main in a few hours to check.
Prakash-Mishra-9ghz reacted with thumbs up emoji

Copy link
Member

Just rebase your PR onto latest main in a few hours to check.

Will you do that rebase, @Prakash-Mishra-9ghz?

Copy link
Author

@sschuberth Currently working on the Docker implementation for the plugin.

Copy link
Author

@sschuberth CI is failing due to an sbt-assembly version mismatch—could you confirm if it is related to PR?

Copy link
Member

@sschuberth CI is failing due to an sbt-assembly version mismatch—could you confirm if it is related to PR?

No, it should not be related, I've retriggered the test.

*/

plugins {
id("org.jetbrains.kotlin.plugin.serialization") version "1.8.22"
Copy link
Member

@sschuberth sschuberth Jul 31, 2025

Choose a reason for hiding this comment

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

Please change the syntax to make use of Gradle's version catalog alias like here:

alias(libs.plugins.kotlinSerialization)

Copy link
Member

@sschuberth sschuberth Jul 31, 2025

Choose a reason for hiding this comment

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

While at it, also add our conventional the grouping comments here, so it looks exactly like:

plugins {
 // Apply precompiled plugins.
 id("ort-plugin-conventions")
 // Apply third-party plugins.
 alias(libs.plugins.kotlinSerialization)
}

api(projects.scanner)

implementation(projects.utils.commonUtils)

Copy link
Member

@sschuberth sschuberth Jul 31, 2025

Choose a reason for hiding this comment

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

No separation for grouping needed here, so just remove this blank line.


ksp(projects.scanner)

implementation("org.jetbrains.kotlinx:kotlinx-serialization-json:1.8.1")
Copy link
Member

@sschuberth sschuberth Jul 31, 2025

Choose a reason for hiding this comment

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

Again, align with code that makes use of the version catalog, so that we do not hard-code versions here:

implementation(libs.kotlinx.serialization.core)
implementation(libs.kotlinx.serialization.json)

Comment on lines 39 to 41
override fun command(workingDir: File?): String {
return listOfNotNull(workingDir, "nomossa").joinToString(File.separator)
}
Copy link
Member

@sschuberth sschuberth Jul 31, 2025

Choose a reason for hiding this comment

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

This is basically a one-liner that should be a function expression with =, similar to

override fun command(workingDir: File?) =
listOfNotNull(workingDir, if (Os.isWindows) "askalono.exe" else "askalono").joinToString(File.separator)

}

override fun transformVersion(output: String): String {
// Example output: nomossasa build version: 4.5.1.1 r(ff4fa7)
Copy link
Member

@sschuberth sschuberth Jul 31, 2025

Choose a reason for hiding this comment

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

Nit: Maybe put the "nomossasa build version: 4.5.1.1 r(ff4fa7)" part to its own line, similar to

// The version string can be something like:
// askalono 0.2.0-beta.1

to avoid potential confusion where the example begins / ends due to the multiple colons in the line-

internal fun NomossaResult.toScanSummary(startTime: Instant, endTime: Instant): ScanSummary {
val licenseFindings = results.flatMap { fileResult ->
fileResult.licenses.map { rawLicense ->

Copy link
Member

@sschuberth sschuberth Jul 31, 2025

Choose a reason for hiding this comment

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

Nit: Omit this superfluous blank line.

Comment on lines 47 to 53
}.toSortedSet(
compareBy(
{ it.license.toString() },
{ it.location.path },
{ it.location.startLine }
)
)
Copy link
Member

@sschuberth sschuberth Jul 31, 2025

Choose a reason for hiding this comment

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

We shouldn't do any sorting here for the in-memory representation. Sorting is only meaningful for deserialization, and there we do it centrally. So just drop this code.

}

LicenseFinding(
license = safeLicense, // use raw string
Copy link
Member

@sschuberth sschuberth Jul 31, 2025

Choose a reason for hiding this comment

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

I think the comment is misleading, as the "raw string" has been converted to a "safe string". I'd just drop the comment.

Comment on lines 42 to 43
startLine = 1,
endLine = 1
Copy link
Member

@sschuberth sschuberth Jul 31, 2025

Choose a reason for hiding this comment

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

So, does Nomossa not provide the line range where a license matches was found at all? I fear that makes it quite unattractive as a scanner. In any case, if the line is unknown, the TextLocation.UNKNOWN_LINE constant should be used.

Copy link
Author

@Prakash-Mishra-9ghz Prakash-Mishra-9ghz Aug 4, 2025

Choose a reason for hiding this comment

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

Nomossa does provide line numbers. Some updates are needed in the implementation — will take care of that soon.

Comment on lines 32 to 42
val safeLicense = if (rawLicense.matches(Regex("^[A-Za-z0-9.\\-+]+$"))) {
rawLicense
} else {
"LicenseRef-Nomossa-${rawLicense.replace(Regex("[^A-Za-z0-9.+-]"), "-")}" // License not found
}
Copy link
Member

@sschuberth sschuberth Jul 31, 2025

Choose a reason for hiding this comment

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

I guess instead of Regex-based checking we could just try to parse the license, similar to here:

val licenseExpression = runCatching { SpdxExpression.parse(license.name) }.getOrNull()
val validatedLicense = when {
licenseExpression == null -> SpdxConstants.NOASSERTION
licenseExpression.isValid() -> license.name
else -> "${SpdxConstants.LICENSE_REF_PREFIX}scanoss-${license.name}"
}

Copy link
Member

Please see my comment over here: This implementation lacks functional tests that make use of the scanner to prove that the implementation is working.

 

- Implement Nomos as an ExternalScanner in ort
- add FOSSology Nomos binary to the docker image
- add funtest for FOSSology Nomos
Signed-off-by: Prakash Mishra <prakashmishra9921@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sschuberth sschuberth sschuberth requested changes

@fviernau fviernau fviernau left review comments

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /