-
Notifications
You must be signed in to change notification settings - Fork 382
feat(ort-project-file): Disable scan when no sources are available #11958
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
MarcelBochtler
wants to merge
1
commit into
oss-review-toolkit:main
Choose a base branch
from
boschglobal:ortproject-metadata-only
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+3
−1
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
I believe this can easily hide mistakes, e.g. user forgets to specify the provenance.
I would prefer instead to add an attribute
sourceCodeOriginswhich the user can specify. What do you think?side note: if only one of
sourceArtifactorvcsis specified,sourceCodeOriginsstill contains both (which is fine imo), but now slightly inconsistent.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.
What should the default of this attribute be? If it's an
emptyList()I'm fine with it, if it would benullthis wouldn't solve the issue that I'm trying to solve with this PR.Having dependencies without provenance is by design as described here in the minimal example. If the user is not specifying the provenance intentionally or accidentally can't be known by ORT.
Additionally, adding a
sourceCodeOriginsattribute would also require us to handle something like:Which could only result in an error.
I'm not sure what benefit this attribute would provide.
If a user wants to prioritize VCS over sourceArtifact (or vice versa), they would only configure one of them.
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 default value imho should be
null, analog to the default value ofPackagefor consistency.The semantics of
nullmeans, that the value configured here should be used.An attempt to run the license scanner against a package without provenance fails by design, if not
explicitly curated in a way that the scanner skips the scanning. Why should it be different for
OrtProjectFile?I believe no handling would need to be added, and ORT's behavior as-is fits: The scanner would report an issue that it could not scan any provenance, which makes sense, because it is configured to try
VCSonly, for which it does not know the provenance.In my view, this would fit consistently into the existing design.
If in the downloader configuration you specify
[VCS, SOURCE_ARTIFACT], then it would work for you also without specifying anysourceCodeOriginsfor the package, or?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.
Because it was decided in #10182 and documented that this should be the minimal working example:
It was not my understanding that this minimal example will only work with the ORT analyzer.
When used with the ORT Scanner, this example cannot be used because it will create a scanner issue, if the user is not specifying any of the attributes below:
What would be your approach to make the minimum example work?
Not if I understand you correctly, because I don't want the scanner to try to download anything if the minimal example is being used.
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.
Would have have a more fine grained pointer. I've skimmed through this rather large ticket and could
not find that decision.
My proposal is to enhance the minimal example for running (analyzer + scanner) as follows:
For just running analyzer, it could remain as-is.
I do not really understand why the scanner is run is no provenance is defined, and why the scanner should
not flag missing provenance, only for ORT project files but in all other cases.
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.
Just a remark: The whole purpose of this pseudo package manager is to make it really simple for users to describe their dependencies. Although it is called OrtProjectFile, it may be therefore useful to come up with a DSL that better supports this goal. From a user's point of view, the proposed
source_code_originsproperty and its consequences may not be easy to understand. Could there be a simpler way to indicate whether the package should be handled by the scanner? Just throwing in ideas:sourceproperty with possible values like a VCS URL, an artifact URL, and some reserved value likeIGNORE.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, sorry.
The decision was made in a TSC meeting linking to a specific comment.
https://github.com/oss-review-toolkit/ort-governance/blob/main/minutes/2025-12-02.md#agenda
Requiring that a user sets an empty list to specify that something is absent makes this more complex than it has to be. I would like to avoid this just to guard against "users that might forget to set the provenance".
Because the file format is designed to support both, dependencies with provenances and dependencies without it. My PR changes exactly that. The scanner is (implicitly) skipped if no provenance is set.
Because when designing this format, we also had the use-case in mind that projects like this have to be supported:
For other package managers a missing provenance should be flagged, because the metadata is not specified by ORT users but by maintainers of the (OSS) dependencies.
Uh oh!
There was an error while loading. Please reload this page.
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.
I still cannot find an indication that silencing failing provenance resoltion was considered in that TSC discussion.
Well, we have the
DownloaderConfiguration, and allows to configuresourceCodeOrigins. And semantics is, that the configured value is used by the downloader / scanner, unless an override is specified. So, as you phrased it above is imho misleading, because it sounds as if the user has to specify something explicitly, which is emtpy implicitly anyway, but I believe this is not the case.Just to make clear, with my above proposal I did not mean to add any limitation at all.
To my understanding, and my intention was, to still support all kinds of use cases.
In particular, also the use case to have the scanner flag issues in case there is no provenance, and not
to make this go unnoticed by default.
I'm not 100% sure if there is a misunderstanding still, or if we simply disagree. However, I would be interrested also in input from others.
Uh oh!
There was an error while loading. Please reload this page.
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.
In my understanding the "minimal working example" that was discussed in the design phase was referring to the package manager itself, not that any subsequent tools like the scanner could not create issues.
Silently ignoring the package in the scanner if no source code location was provided also has the risk that users get the false impression that the license detection did not find any issues, so I would also prefer that users have to make this decision consciously. Otherwise, it could also be that they have just forgotten to provide the source code location and will never notice because no warning of error is shown.
If
source_code_origins: []is too unintuitive, what about adding a more speaking property likeskip_scanner: trueorsource_code_available: falsewhich is then internally mapped tosource_code_origins: []?Just for reference, neither of the proposed solutions follows ORT's philosophy that users should provide a reason why it is ok to ignore this package in the scanner like they would have when creating an issue resolution or a curation.