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

Analyzer reports issues and returns exit code 2 but no issues are visible #3324

Closed
pilvikala opened this issue Nov 11, 2020 · 19 comments · Fixed by #4246
Closed

Analyzer reports issues and returns exit code 2 but no issues are visible #3324

pilvikala opened this issue Nov 11, 2020 · 19 comments · Fixed by #4246

Comments

@pilvikala
Copy link

I am running the analyzer against the the master branch of Flasky (https://github.com/miguelgrinberg/flasky) and the analyzer reports that there are issues in the output file. Indeed, the analyzer-result.yml contains the has_issues: true but there are no additional details. The debug log does not provide any information either, or at least I don't see it.

analyzer-result.zip

log.log

Can we make the analyzer to produce more information about the actual issues either in the logs or the results file?

@woznik
Copy link

woznik commented Nov 16, 2020

Hello
I have a similar case: has_issues: true
There are only warnings in the analyzer log (4 artifacts are unreachable) and finally Analyzer throws an error at the end

Project: https://github.com/seata/seata

I have applied curations.yml file to ommit missing sources for the raised warnings but the results is the same

I enclose the analyzer logs

ort_artifacts_from_seata_on_master.zip

@mnonnenmacher
Copy link
Member

mnonnenmacher commented Nov 17, 2020

The hasIssues property is only set to true if there is at least one OrtIssue object in the result (no matter what severity), so the result file should contain more information. There are three places where an analyzer result can contain issues:

  • In the dependency tree, if one of the dependencies could not be resolved correctly. For example:
    issues:
    - timestamp: "1970-01-01T00:00:00Z"
    source: "Gradle"
    message: "Unresolved: ModuleVersionNotFoundException: Cannot resolve external\
    \ dependency org.apache.commons:commons-text:1.1 because no repositories\
    \ are defined."
    severity: "ERROR"
  • In AnalyzerResult.issues. This is only used if there is an issue with one of the projects, for example if there was an exception while analyzing the project.
  • ORT issues are dynamically created if there is a declared license that could not be mapped to an SPDX expression, for example:
    declared_licenses_processed:
    unmapped:
    - "http://json.codeplex.com/license"
    . I have checked the results you posted, in both cases the reason for has_issues: true is an unmapped declared license.

The easiest way to see those issues is to generate the Static HTML or WebApp reports, but for this you have to run the scanner on the analyzer result first.

@sschuberth
Copy link
Member

Thanks @mnonnenmacher for that nice summary.

if there is at least one OrtIssue object in the result

Actually, that's what I thought, too, but strictly speaking an unmapped license does not create an OrtIssue object in the result that would be visible on its own, and I would agree if some one would call this confusing.

So, should we maybe not consider unmapped licenses to be issues? Or maybe better, create an AnalyzerResult.issues entry for unmapped licenses?

@mnonnenmacher
Copy link
Member

It's important that unmapped licenses get reported, because they can contain valid licenses. The easist way was to use OrtIssues for this. An alternative would be to make an evaluator rule to check for unmapped licenses, but that would mean that every user has to setup that rule and that it is reported only in the evaluator when the issue is already known in the analyzer. I'd prefer if just the description of hasIssues was updated instead of changing how it currently works.

Also exit code 2 probably added some confusion, the commit message of c00d33a should be added to the docs somewhere.

@sschuberth
Copy link
Member

To me, the root cause of the confusion is that the causes of has_issues are meanwhile scattered across the analyzer result. Usually, if I myself want to find the actual issue, I just search for "issues:" in the analyzer-result.yml file, which does not help currently in the case of unmapped licenses.

Hence my proposal to list unmapped licenses issues also in the top-level issues. In fact, I believe we should consider to generally only have a single list of issues in analyzer-result.yml, and a way how issues could point to where they occurred. I believe that would make reading the analyzer-result.yml file much clearer.

sschuberth added a commit that referenced this issue Nov 18, 2020
The homepage lists being liensed under "MIT License" as a feature.

Also see the discussion at [1].

[1] #3324 (comment)

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth
Copy link
Member

BTW, I'm adding a mapping for "http://json.codeplex.com/license" in #3346.

@pilvikala
Copy link
Author

Thanks for the explanation. I would definitely like the 'issues' listed in the analyzer result file.

sschuberth added a commit that referenced this issue Nov 18, 2020
The homepage lists being liensed under "MIT License" as a feature.

Also see the discussion at [1].

[1] #3324 (comment)

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth
Copy link
Member

@pilvikala I'm proposing a mapping that should fix your case, see #3347.

@sschuberth
Copy link
Member

@woznik for your case #3349 should fix the issue.

sschuberth added a commit that referenced this issue Nov 19, 2020
The homepage lists being licensed under "MIT License" as a feature.

Also see the discussion at [1].

[1] #3324 (comment)

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Nov 19, 2020
The homepage lists being licensed under "MIT License" as a feature.

Also see the discussion at [1].

[1] #3324 (comment)

Signed-off-by: Sebastian Schuberth <[email protected]>
@woznik
Copy link

woznik commented Nov 26, 2020

@sschuberth, @mnonnenmacher many thanks

@sschuberth
Copy link
Member

ORT issues are dynamically created if there is a declared license that could not be mapped to an SPDX expression

Note to myself, this currently happens at:

fun collectIssues(): List<OrtIssue> {
val severity = pkg.concludedLicense?.let { Severity.HINT } ?: Severity.WARNING
return pkg.declaredLicensesProcessed.unmapped.map { unmappedLicense ->
OrtIssue(
severity = severity,
source = pkg.id.toCoordinates(),
message = "The declared license '$unmappedLicense' could not be mapped to a valid license or " +
"parsed as an SPDX expression."
)
}
}

@sschuberth
Copy link
Member

I have a simple proposal about how to address the topic: How about renaming the unmapped field to mapping_issues? That way searching for "issues" in the ORT result file would find both "mapping_issues" and "has_issues", so it becomes more clear that unmapped licenses are an issue source.

What do you think @mnonnenmacher and @fviernau?

@sschuberth
Copy link
Member

Or alternatively, remove collectIssues() from CuratedPackage and instead dynamically create serialized issues on the AnalyzerResult level (which would make unmapped licenses show up both as unmapped and issues; not sure if this "duplication" would be fine).

@fviernau
Copy link
Member

fviernau commented Jun 24, 2021

Creating "serialized issues" in the analyzer result would be problematic when re-applying curations in the evaluator. E.g. fixing a declared license mapping issue with a declared license mapping curation would not remove the issue from the analyzer result.

Have you considered dropping the analyzer issue for "declared license mapping" completely and adding a policy rule to the examples / default which does the same via rule violation? (In HERE internal setup we have a policy rule for this already)

@sschuberth
Copy link
Member

Have you considered dropping the analyzer issue for "declared license mapping" completely and adding a policy rule to the examples / default which does the same via rule violation?

I recall we had a similar discussion as part of #4082. But back then my impression was that 1) we do want to have issues reported for unmapped licenses, and 2) these issues should be reported as early as possible, i.e. not only in the evaluator, but already in the analyzer. Did that change?

@fviernau
Copy link
Member

fviernau commented Jun 24, 2021

I recall we had a similar discussion as part of #4082. But back then my impression was that 1) we do want to have issues reported for unmapped licenses, and 2) these issues should be reported as early as possible, i.e. not only in the evaluator, but already in the analyzer. Did that change?

Regarding 1): a policy rule would still report "issues" for declared licenses, if reporting refers to a report.
Regarding 2): Unfortunately I don't recall this conclusion from the context of #4082. I'm not sure if this has to be considered on the very general level of "issue" or if a distinction between issue types can make sense.

Probably I'm missing something in the following, but I only recall analyzer issues which are execution problems. Problems with the execution of the logic which determins the meta data. Also in the downloader and scanner the issues are only execution issues IIRC. The mapping issue however is an issue with the meta-data values, not with determining it. From that perspective I find it fits nicely into the policy, because it is similar to other issues with licenses. Would it make sense to say "issues" can only ever be execution issues? For these I would agree that reporting them as early as possible makes sense.

@sschuberth
Copy link
Member

sschuberth commented Jun 24, 2021

Regarding 2): Unfortunately I don't recall this conclusion from the context of #4082.

True, that was more how I understood @mnonnenmacher's comment from above in this issue.

The mapping issue however is an issue with the meta-data values, not with determining it. From that perspective I find it fits nicely into the policy

I agree. Implementing that "separation of concerns" consequently would also help us in our workflow to determine "who's responsible for addressing what kind of issue from what tool".

So, does that mean we agree on removing collectIssues() from CuratedPackage? And instead every user who wants this behavior needs to implement a policy rule instead, with us eventually providing an example rule for this case? @fviernau, could you simply contribute your existing implementation to examples/rules.kts?

@fviernau
Copy link
Member

fviernau commented Jun 24, 2021

So, does that mean we agree on removing collectIssues() from CuratedPackage?

From my side yes. @mnonnenmacher , agree as well?

fviernau, could you simply contribute your existing implementation to examples/rules.kts?

yes.

edit: Seems I already contributed that in 5543aed

@mnonnenmacher
Copy link
Member

Yes, if we want to leave it up to the user to decide if unmapped licenses are an issue then removing the issues and requiring a rule instead is the best solution.

sschuberth added a commit that referenced this issue Jul 1, 2021
Treating unmapped licenses as issues has caused confusion in the past as
these issues are not serialized into the ORT result despite "has_issues"
being "true" in that case. Moreover, unmapped licenses are not a
technical problem, but a metadata problem, which in many workflows will be
handled by different people than technical issues.

So do not dynamically create issues for unmapped licenses anymore;
instead the recommendation is to use the example code from [1] to report
unmapped licenses as policy violations.

Resolves #3324.

[1] https://github.com/oss-review-toolkit/ort/blob/2e8f08e/examples/rules.kts#L123-L135

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jul 3, 2021
Treating unmapped licenses as issues has caused confusion in the past as
these issues are not serialized into the ORT result despite "has_issues"
being "true" in that case. Moreover, unmapped licenses are not a
technical problem, but a metadata problem, which in many workflows will be
handled by different people than technical issues.

So do not dynamically create issues for unmapped licenses anymore;
instead the recommendation is to use the example code from [1] to report
unmapped licenses as policy violations.

Resolves #3324.

[1] https://github.com/oss-review-toolkit/ort/blob/2e8f08e/examples/rules.kts#L123-L135

Signed-off-by: Sebastian Schuberth <[email protected]>
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 a pull request may close this issue.

5 participants