Evaluator: Allow making arbitrary file checks on the project's source tree #5679

fviernau · 2022-08-24T10:35:32Z

See individual commits.

Note: The project's source tree will be cloned in the evaluator only if the rules needed it / if the file checks are actually used.

This implements parts of #5621.

codecov · 2022-08-24T10:49:16Z

Codecov Report

Merging #5679 (48a0c9d) into main (5ce9aaf) will not change coverage.
The diff coverage is n/a.

❗ Current head 48a0c9d differs from pull request most recent head 47de9f5. Consider uploading reports for the commit 47de9f5 to get more accurate results

@@            Coverage Diff            @@
##               main    #5679   +/-   ##
=========================================
  Coverage     65.54%   65.54%           
  Complexity     2212     2212           
=========================================
  Files           271      271           
  Lines         16600    16600           
  Branches       3473     3473           
=========================================
  Hits          10881    10881           
  Misses         4575     4575           
  Partials       1144     1144

Flag	Coverage Δ
funTest-analyzer-docker	`74.58% <0.00%> (ø)`
test	`32.01% <0.00%> (ø)`

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mnonnenmacher

You use different prefixes for the example commits: "example.rules", "examples.rules", "example.rules.kts", please align.

cli/src/main/kotlin/commands/EvaluatorCommand.kt

examples/evaluator-rules/src/main/resources/example.rules.kts

cli/src/main/kotlin/commands/EvaluatorCommand.kt

sschuberth · 2022-08-24T20:00:40Z

evaluator/src/main/kotlin/SourceTree.kt

+import org.ossreviewtoolkit.model.config.DownloaderConfiguration
+import org.ossreviewtoolkit.utils.ort.createOrtTempDir
+
+class SourceTree private constructor(


Do we really need this new class? Can't we simply use the existing WorkingTree instead?

The reason I added this class will be more obvious when you look at what get's added in the following commits.
That's all IMO evaluator specific logic. Basically helper functions to use for implementing RuleMatchers and / or policy rules.

I saw the upcoming changes, but I'm still not convinced. Looks like the helper functions would as well operate on a WorkingTree.

I saw the upcoming changes, but I'm still not convinced.

let me just try on more time to convince you:

I believe even if these helper functions could all be implemented in WorkingTree encapsulation does make sense, because

It is impossible that API change in the analyzer (working tree) breaks a policy rule implementation. So, the rules API is
independent from working tree.

The API can be designed to reflect exactly the requirements from the rules. This is exactly what is needed to arrive at easy to read rules. In particular from my experience it's hard to foresee the exact API needs when implementing new policy rules use cases, and my gut feeling is that this encapsulation will make more and more sense the more functions are being added.

The functions can be implement based on the working tree, but it is not required. The encapsulation allows
changing the implementation. For example a file existence check could be changed to not work on the cloned source but
on the ScanResult, if that contained a full list of all files.

The helper functions are basically factored out logic from the rule matchers added to OrtResult. Exposing that logic is
important to expose because the logic inside rule matchers is not re-usable. For example the rule matcher hasFile()
contains logic to find the actual files. If you want to re-use the logic to find the files, the logic cannot be put into hasFile()
matcher but needs to be exposed somewhere. That somewhere is the SourceTree class I've added. I don't see why that somewhere should be WorkingTree.

Is any of the above points somewhat more convincing?

BTW.: I'm planning to add matchers for the commit history, and therefore create (not expose) a working tree instance inside the sourcetree class.

Anyhow, as I plan to make a couple of further PRs on top, can we just keep it for now and make that decision when that work is done (guess we know more by then)? If it's really not needed then I'll refactor it away.

can we just keep it for now and make that decision when that work is done

I'd actually prefer to get that sorted out now. IMO we had to much of "I need this urgently now so let's merge it"-style of changes recently, and ORT's code base is starting to suffer from suboptimal code design decisions (with only a single use-case in mind).

I'd actually prefer to get that sorted out now.

Ok, then let's sort it out asap.

IMO we had to much of "I need this urgently now so let's merge it"-style of changes recently, and ORT's code base is starting to suffer from suboptimal code design decisions (with only a single use-case in mind).

Maybe I wasn't clear enough. I said that I would do the refactoring in a later change if we by then consider it reasonable. So, the code base wouldn't suffer from it in the long run. Did you get that?

I prefer a rather iterative approach as I believe the proposed refactoring is a bit too early and can be done with gained knowledge a couple of days later in a following iteration.

So, the code base wouldn't suffer from it in the long run. Did you get that?

I did get that. But such promises were made in the past, and then that planned refactoring never happened, or only after a very long time.

I prefer a rather iterative approach

Iterative is fine, but iterative should also mean that it's already going into the right direction, and not introducing stuff that gets removed later on (where foreseeable).

What I'd like to discuss first with you and at least also @mnonnenmacher is whether providing access to the source / working tree should really become a "first class citizen" of the evaluator as currently sketched. When I first read about #5621, I was hoping that would be mostly implemented by helper functions implemented in EPAM's rules.kts itself, and not so much in ORT upstream.

evaluator/src/main/kotlin/SourceTree.kt

evaluator/src/test/kotlin/OrtResultRuleTest.kt

Move `getRepositoryPath()` to `OrtResultExtensions` to enable re-use in an upcoming change. Signed-off-by: Frank Viernau <frank_viernau@epam.com>

… tree Allow access to the project's source tree in order to enable doing arbitrary checks on files like the ones in repolinter [1]. Adding arbitrary file checks do make the rules API more powerful, as it allows highly customizable checks which can automate parts of the checks typically done prior to open sourcing a project. Not using a third-party tool makes sense as it is simpler to use, because it sticks to a single way (rules.kts) for writing the policy rules. Note that the implementation intentionally is limited to the project's source tree, e.g. it does not work for dependency source, because the doing such checks throught the dependency tree does not have obvious need and is not feasible anyway in terms of exection time. [1] https://github.com/todogroup/repolinter Signed-off-by: Frank Viernau <frank_viernau@epam.com>

Signed-off-by: Frank Viernau <frank_viernau@epam.com>

Some policy rules do not only require the result of hasFile(), but also the actual matching files if any. The same applies to hasDirectory(). So, expose the logic for finding the files and directories. Signed-off-by: Frank Viernau <frank_viernau@epam.com>

Signed-off-by: Frank Viernau <frank_viernau@epam.com>

fviernau · 2022-09-07T09:54:32Z

I've created a new PR which is more minimal: #5754.
Let's continue with that PR to agree on the concept and get it merged.

fviernau · 2022-09-12T14:37:06Z

The code of this PR has been refactored and is superseeded by:

evaluator: Extend the API for making file checks against the project's source code #5778
Evaluator: Allow making arbitrary file checks on the project's source, take 2 #5754

fviernau requested a review from a team as a code owner August 24, 2022 10:35

fviernau force-pushed the evaluator-enable-arbitrary-file-checks branch from 2eeae8c to 6525ee2 Compare August 24, 2022 10:38

mnonnenmacher requested changes Aug 24, 2022

View reviewed changes

cli/src/main/kotlin/commands/EvaluatorCommand.kt Show resolved Hide resolved

examples/evaluator-rules/src/main/resources/example.rules.kts Outdated Show resolved Hide resolved

fviernau force-pushed the evaluator-enable-arbitrary-file-checks branch 2 times, most recently from 3d921be to 09d7f45 Compare August 24, 2022 20:00

sschuberth requested changes Aug 24, 2022

View reviewed changes

fviernau force-pushed the evaluator-enable-arbitrary-file-checks branch from 09d7f45 to 7c95528 Compare August 24, 2022 21:02

fviernau added 12 commits August 24, 2022 23:39

FreeMarkerTemplateProcesser: Extract getRepositoryPath()

f93abe5

Move `getRepositoryPath()` to `OrtResultExtensions` to enable re-use in an upcoming change. Signed-off-by: Frank Viernau <frank_viernau@epam.com>

OrtResultRule: Add the matcher sourceTreeHasFile()

cb0c793

Signed-off-by: Frank Viernau <frank_viernau@epam.com>

example.rules: Illustrate the use of sourceTreeHasFile()

334d57b

Signed-off-by: Frank Viernau <frank_viernau@epam.com>

OrtResultRule: Add the matcher sourceTreeHasDirectory()

f743390

Signed-off-by: Frank Viernau <frank_viernau@epam.com>

example.rules: Illustrate the use of hasDirectory() and hasFile()

c259973

Signed-off-by: Frank Viernau <frank_viernau@epam.com>

example.rules: Illustrate the use of findDirectories()

08796ba

Signed-off-by: Frank Viernau <frank_viernau@epam.com>

OrtResultRule: Add the matcher sourceTreeHasFileWithContents()

7bc6406

Signed-off-by: Frank Viernau <frank_viernau@epam.com>

example.rules: Illustrate the use of sourceTreeHasFileWithContents()

7d69ba1

Signed-off-by: Frank Viernau <frank_viernau@epam.com>

ExamplesFunTest: Sort the list of expected violations alphabetically

7af81d4

Signed-off-by: Frank Viernau <frank_viernau@epam.com>

example.rules: Add a section for the 'prior to open sourcing' use case

47de9f5

Signed-off-by: Frank Viernau <frank_viernau@epam.com>

fviernau force-pushed the evaluator-enable-arbitrary-file-checks branch from 7c95528 to 47de9f5 Compare August 24, 2022 21:39

fviernau requested review from sschuberth and mnonnenmacher August 24, 2022 21:39

fviernau mentioned this pull request Sep 7, 2022

Minor preparations for the implementation of file checks #5750

Merged

fviernau changed the title ~~Evaluator: Allow making arbitrary file checks on the project's source tree~~ ON HOLD: Evaluator: Allow making arbitrary file checks on the project's source tree Sep 7, 2022

sschuberth added the on hold Pull requests that cannot currently be merged label Sep 7, 2022

sschuberth changed the title ~~ON HOLD: Evaluator: Allow making arbitrary file checks on the project's source tree~~ Evaluator: Allow making arbitrary file checks on the project's source tree Sep 7, 2022

fviernau closed this Sep 12, 2022

fviernau deleted the evaluator-enable-arbitrary-file-checks branch September 12, 2022 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Evaluator: Allow making arbitrary file checks on the project's source tree #5679

Evaluator: Allow making arbitrary file checks on the project's source tree #5679

fviernau commented Aug 24, 2022

codecov bot commented Aug 24, 2022 •

edited

Loading

mnonnenmacher left a comment

sschuberth Aug 24, 2022

fviernau Aug 24, 2022

sschuberth Aug 24, 2022

fviernau Aug 24, 2022

fviernau Aug 25, 2022

sschuberth Aug 25, 2022

fviernau Aug 25, 2022 •

edited

Loading

sschuberth Aug 26, 2022

fviernau commented Sep 7, 2022

fviernau commented Sep 12, 2022

Evaluator: Allow making arbitrary file checks on the project's source tree #5679

Evaluator: Allow making arbitrary file checks on the project's source tree #5679

Conversation

fviernau commented Aug 24, 2022

codecov bot commented Aug 24, 2022 • edited Loading

Codecov Report

mnonnenmacher left a comment

Choose a reason for hiding this comment

sschuberth Aug 24, 2022

Choose a reason for hiding this comment

fviernau Aug 24, 2022

Choose a reason for hiding this comment

sschuberth Aug 24, 2022

Choose a reason for hiding this comment

fviernau Aug 24, 2022

Choose a reason for hiding this comment

fviernau Aug 25, 2022

Choose a reason for hiding this comment

sschuberth Aug 25, 2022

Choose a reason for hiding this comment

fviernau Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

sschuberth Aug 26, 2022

Choose a reason for hiding this comment

fviernau commented Sep 7, 2022

fviernau commented Sep 12, 2022

codecov bot commented Aug 24, 2022 •

edited

Loading

fviernau Aug 25, 2022 •

edited

Loading