-
Notifications
You must be signed in to change notification settings - Fork 353
feat(Provenance): Add DirectoryProvenance as a LocalProvenance
#9872
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
Conversation
a984e1f to
2471efa
Compare
2471efa to
1e3c8cf
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@ ## main #9872 +/- ## ============================================ + Coverage 57.38% 57.47% +0.08% - Complexity 1673 1691 +18 ============================================ Files 346 346 Lines 12759 12801 +42 Branches 1209 1219 +10 ============================================ + Hits 7322 7357 +35 - Misses 4972 4976 +4 - Partials 465 468 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1e3c8cf to
c2f8552
Compare
DirectoryProvenance as a LocalProvenance (削除ここまで)LocalProvenance sub-interface (追記ここまで)
c2f8552 to
c6cbdbc
Compare
@sschuberth failing tests seem unrelated.
Could you have a look and maybe restart them?
A review would be awesome as well.
c6cbdbc to
c0a51c2
Compare
@sschuberth failing tests seem unrelated.
The "Unable to create proxy for sealed class" failures in org.ossreviewtoolkit.scanner.ScannerTest are real. I guess the problem is that nothing currently implements the LocalProvenance interface. So you probably have to add a DirectoryProvenance at the same time.
Thanks @sschuberth, I missed that. I'm already working on the DirectoryProvenance over at provenance-directory.
So it would be easy enough to append it here.
However, it is quite a large branch, since a lot of whens require a DirectoryProvenance case.
Most of which are for KnownProvenance input, so I can not default to UnknownProvenance behavior.
I started with reasonable implementation for the DirectoryProvenance behavior, but the scope of required does not seem to end. So I was considering switching to RemoteProvenance inputs to avoid the usage of DirectoryProvenance for now.
Any thoughts or other ideas how to keep the PR small?
Most of which are for
KnownProvenanceinput, so I can not default toUnknownProvenancebehavior.
Can't you "cheat" a bit by first limiting some / all KnownProvenance cases to RemoteProvenance (which semantically is no difference compared to before), and only later bit-by-bit extend respective code paths to also handle LocalProvenance?
So I was considering switching to
RemoteProvenanceinputs to avoid the usage ofDirectoryProvenancefor now.
Can't you "cheat" a bit by first limiting some / all
KnownProvenancecases toRemoteProvenance(which semantically is no difference compared to before), and only later bit-by-bit extend respective code paths to also handleLocalProvenance?
Great to see we are on the same page here. I will do just that.
9322fb4 to
9ffd2c5
Compare
LocalProvenance sub-interface (削除ここまで)DirectoryProvenance as a LocalProvenance (追記ここまで)
@sschuberth I added the DirectoryProvenance now and "cheated" the when cases.
Let me know what you think, if you have any reservations.
Commit message:
It is defined by its local directory path only.
We should probably say "absolute" / "canonical" / "real" path, and do the implementation accordingly.
it the new class can not be used right now.
"the new class can not be used right now."
However the presence of a new
KnownProvenanceclass, results
"However, the presence of a new KnownProvenance class results"
Whereever
"Wherever"
excaption
"exception"
Provenance Hierarchy
"provenance hierarchy"
9ffd2c5 to
9fc284d
Compare
9fc284d to
4d39003
Compare
cfdad33 to
4b97905
Compare
4b97905 to
cdeeddb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could combine this with line 85 to
is DirectoryProvenance, UnknownProvenance -> false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But actually, at least when a PackageConfiguration is used in the context of a RepositoryConfiguration, the Identifier (and Provenance) actually is allowed to refer to a Project. So we should probably implement a proper check for DirectoryProvenance in that case after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I assumed we would need a proper check sooner or later, which is why I added the case as a separate line.
Since this would touch the attributes of the PackageConfiguration, I assumed the best cause of action was to postpone any changes for the sake of a shorter PR and handle DirectoryProvenace like UnknownProvenance for now. I'd recommend to handle this, when we actually start using the DirectoryProvenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is changed, the docs need to be adjusted as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change requires some careful thinking: We actually do want to be able to scan projects with DirectoryProvenance in the end, at least in the case when analyzer and scanner are run on the same machine. So is it really correct to limit us to RemoteProvenance here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are probably multiple places, where the limit to RemoteProvenance is not correct in order to handle the DirectoryProvenance, once it is fully implemented.
But right now, while the class is dormant, it is either this or cast the arguments. Otherwise we would have to handle it everywhere from the get go as well.
side note: as this attribute is named packageProvenances, we probably need to address the shift from packages to projects as (primary) input for the scanner at some point, since you repeatedly stated before that a DirectoryProvenace can not be a Package due to lack of a remote.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message refers to "known provenance", but the check is against RemoteProvenance, so this does not match.
This also somewhat relates to my comment above, that we actually do want to be able to scan a DirectoryProvenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the code and the exception's text are technically not reflecting the same thing.
However, while the DirectoryProvenance is unused, the RemoteProvenance is practically the same as KnownProvenance. Similar to my comment above, this is a temporary change, which aims to avoid issues due unfinished class handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. ^^
cdeeddb to
84cf646
Compare
150b157 to
19e6d80
Compare
19e6d80 to
534213d
Compare
534213d to
0f33b19
Compare
In contrast to the previously added `RemoteProvenance` stands the `LocalProvenance`, which has no remote source, but instead references a local input of some kind. The `DirectoryProvenance` references a local project directory, which is lacking supported (remote) version control. It is defined by its canonical path only. Since ORT needs further refactoring until `DirectoryProvenance` can be fully utilized, the new class can not be used right now. However the presence of a new `KnownProvenance` class results in `when` conditional cases not being exhaustive anymore. To circumvent this issue, the following changes were made: 1. Wherever possible `RemoteProvenance` is used as parameter type instead of `KnownProvenance`. 2. When necessary `KnownProvenance`s are cast to `RemoteProvenance`. 3. If `Provenance` is expected, `DirectoryProvenance` is handled like `UnknownProvenance`. For instances of `Package` the new default data structure should be `RemoteProvenance`, as a `Package` by definition requires a remote. The exception being `hash` and `storageKey`, which both required a default value, which was set to the `canonicalPath`. However, since the rest of the code does not handle `DirectoryProvenance`, this should remain unused for now. See [1] for more context on the new provenance hierarchy. [1]: oss-review-toolkit#8803 (comment) Signed-off-by: Jens Keim <jens.keim@forvia.com>
Casts were removed from `FileListResolver`, `ProvenanceBasedFileStorage`, `ProvenanceDownloader`, and `Scanner`. In order to avoid casting `KnownProvenance` to `RemoteProvenance`, a lot of parameters need to be changed to `RemoteProvenance`. With the exception of `ProvenanceBasedFileStorage`, which now uses `UnknownProvenance` exceptions just like `ProvenanceBasedPostgresStorage`. Signed-off-by: Jens Keim <jens.keim@forvia.com>
Signed-off-by: Jens Keim <jens.keim@forvia.com>
0f33b19 to
450b5eb
Compare
Just a rebase onto 66.1.0 for potential use in my POC.
As discussed in the weekly developer meeting today, I have for you the working branch for handling DirectoryProvenance instead of omitting it from every KnownProvenance check in ORT:
diff: main...pepper-jk:ort:provenance-local-handling
commits: https://github.com/pepper-jk/ort/commits/provenance-local-handling/
tests: https://github.com/pepper-jk/ort/actions/runs/17458366518/job/49577013217
The branch is still rough, I just extracted it from the POC. I believe for proper handling of the DirectoryProvenance for packages still requires some more work, but it could be an alternate route for this PR.
So far it is just a byproduct of my POC, where I am focused on using DirectoryProvenance as Analyzer input via Repository, based on #10575.
Uh oh!
There was an error while loading. Please reload this page.
In contrast to the previously added
RemoteProvenancestands theLocalProvenance, which has no remote source, but instead references a local input of some kind.The
DirectoryProvenancereferences a local project directory, which is lacking supported (remote) version control. It is defined by its local directory path only.Since ORT needs further refactoring until
DirectoryProvenancecan be fully utilized, it the new class can not be used right now. However the presence of a newKnownProvenanceclass, results inwhenconditional cases not being exhaustive anymore.To circumvent this issue, the following changes were made:
RemoteProvenanceis used as parameter type instead ofKnownProvenance.KnownProvenances are cast toRemoteProvenance.Provenanceis expected,DirectoryProvenanceis handled likeUnknownProvenance.The excaption being
hashandstorageKey, which both default to an empty string. However, since the rest of the code does not handleDirectoryProvenance, this should not become an issue.For more context on the new Provenance Hierarchy, see: #8803 (comment)
Signed-off-by: Jens Keim jens.keim@forvia.com