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.

Sign up for GitHub

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

feat(Repository): Use KnownProvenance instead of VcsInfo #8764

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat(Repository): Use KnownProvenance instead of VcsInfo #8764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
73f851a
refactor(VcsInfo): Replace `matches` method with `equalsDisregardingP…
pepper-jk Jun 20, 2024
9ffaeb0
feat(Provenance): Add `matches(Provenance)` method
pepper-jk Jun 12, 2024
7969f0b
refactor(Provenance): Allow blank `resolvedRevision` for `VcsInfo.EMPTY`
pepper-jk Jun 18, 2024
ba2a2a2
feat(Repository): Use `KnownProvenance` instead of `VcsInfo`
pepper-jk Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion analyzer/src/main/kotlin/Analyzer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import org.ossreviewtoolkit.model.AnalyzerResult
import org.ossreviewtoolkit.model.AnalyzerRun
import org.ossreviewtoolkit.model.OrtResult
import org.ossreviewtoolkit.model.Repository
import org.ossreviewtoolkit.model.RepositoryProvenance
Copy link
Member

Choose a reason for hiding this comment

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

Commit message:

  • Again use passive voice
  • Typo in "generallized" -> "generalized"
  • "Repository" -> "Repository class" (use backticks so it's clear you refer to the class name)
  • "accomidateing" -> "accommodating"
  • "then main" -> "the main"
  • "depreacted" -> "deprecated"
  • "realted" -> "related"

import org.ossreviewtoolkit.model.config.AnalyzerConfiguration
import org.ossreviewtoolkit.model.config.Excludes
import org.ossreviewtoolkit.model.config.RepositoryConfiguration
Expand Down Expand Up @@ -135,11 +136,16 @@ class Analyzer(private val config: AnalyzerConfiguration, private val labels: Ma

val workingTree = VersionControlSystem.forDirectory(info.absoluteProjectPath)
val vcs = workingTree?.getInfo().orEmpty()
val revision = workingTree?.getRevision().orEmpty()
val nestedVcs = workingTree?.getNested()?.filter { (path, _) ->
// Only include nested VCS if they are part of the analyzed directory.
workingTree.getRootPath().resolve(path).startsWith(info.absoluteProjectPath)
}.orEmpty()
val repository = Repository(vcs = vcs, nestedRepositories = nestedVcs, config = info.repositoryConfiguration)
val repository = Repository(
provenance = RepositoryProvenance(vcs, revision),
nestedRepositories = nestedVcs,
config = info.repositoryConfiguration
)

val endTime = Instant.now()

Expand Down
17 changes: 7 additions & 10 deletions cli/src/funTest/assets/git-repo-expected-output.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
---
repository:
vcs:
type: "GitRepo"
url: "https://github.com/oss-review-toolkit/ort-test-data-git-repo?manifest=manifest.xml"
revision: "31588aa8f8555474e1c3c66a359ec99e4cd4b1fa"
path: ""
vcs_processed:
type: "GitRepo"
url: "https://github.com/oss-review-toolkit/ort-test-data-git-repo.git?manifest=manifest.xml"
revision: "31588aa8f8555474e1c3c66a359ec99e4cd4b1fa"
path: ""
provenance:
vcs_info:
type: "GitRepo"
url: "https://github.com/oss-review-toolkit/ort-test-data-git-repo?manifest=manifest.xml"
revision: "31588aa8f8555474e1c3c66a359ec99e4cd4b1fa"
path: ""
resolved_revision: "31588aa8f8555474e1c3c66a359ec99e4cd4b1fa"
nested_repositories:
spdx-tools:
type: "Git"
Expand Down
17 changes: 7 additions & 10 deletions cli/src/funTest/assets/gradle-all-dependencies-expected-result-with-curations.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
---
repository:
vcs:
type: "Git"
url: "<REPLACE_URL>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/gradle/src/funTest/assets/projects/synthetic/gradle"
vcs_processed:
type: "Git"
url: "<REPLACE_URL_PROCESSED>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/gradle/src/funTest/assets/projects/synthetic/gradle"
provenance:
vcs_info:
type: "Git"
url: "<REPLACE_URL>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/gradle/src/funTest/assets/projects/synthetic/gradle"
resolved_revision: "<REPLACE_REVISION>"
config:
excludes:
paths:
Expand Down
17 changes: 7 additions & 10 deletions cli/src/funTest/assets/semver4j-ort-result.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
---
repository:
vcs:
type: "Git"
url: "https://github.com/vdurmont/semver4j.git"
revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
path: ""
vcs_processed:
type: "Git"
url: "https://github.com/vdurmont/semver4j.git"
revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
path: ""
provenance:
vcs_info:
type: "Git"
url: "https://github.com/vdurmont/semver4j.git"
revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
path: ""
resolved_revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
config:
excludes:
scopes:
Expand Down
2 changes: 1 addition & 1 deletion evaluator/src/test/kotlin/ProjectSourceRuleTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ private fun createOrtResult(
}

return OrtResult.EMPTY.copy(
repository = Repository(vcsInfo),
repository = Repository(RepositoryProvenance(vcsInfo, vcsInfo.revision)),
analyzer = AnalyzerRun.EMPTY.copy(
result = AnalyzerResult.EMPTY.copy(
projects = setOf(
Expand Down
3 changes: 2 additions & 1 deletion evaluator/src/test/kotlin/TestData.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import org.ossreviewtoolkit.model.Package
import org.ossreviewtoolkit.model.PackageLinkage
import org.ossreviewtoolkit.model.Project
import org.ossreviewtoolkit.model.Repository
import org.ossreviewtoolkit.model.RepositoryProvenance
import org.ossreviewtoolkit.model.ScanResult
import org.ossreviewtoolkit.model.ScanSummary
import org.ossreviewtoolkit.model.ScannerDetails
Expand Down Expand Up @@ -180,7 +181,7 @@ val allProjects = setOf(

val ortResult = OrtResult(
repository = Repository(
vcs = VcsInfo.EMPTY,
provenance = RepositoryProvenance(VcsInfo.EMPTY, ""),
config = RepositoryConfiguration(
excludes = Excludes(
paths = listOf(
Expand Down
17 changes: 7 additions & 10 deletions helper-cli/src/funTest/assets/create-analyzer-result-from-pkg-list-expected-output.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
---
repository:
vcs:
type: "Git"
url: "https://github.com/example/project.git"
revision: "2222222222222222222222222222222222222222"
path: "vcs-path/project"
vcs_processed:
type: "Git"
url: "https://github.com/example/project.git"
revision: "2222222222222222222222222222222222222222"
path: "vcs-path/project"
provenance:
vcs_info:
type: "Git"
url: "https://github.com/example/project.git"
revision: "2222222222222222222222222222222222222222"
path: "vcs-path/project"
resolved_revision: "2222222222222222222222222222222222222222"
config:
excludes:
scopes:
Expand Down
3 changes: 2 additions & 1 deletion helper-cli/src/main/kotlin/commands/CreateAnalyzerResultFromPackageListCommand.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import org.ossreviewtoolkit.model.PackageReference
import org.ossreviewtoolkit.model.Project
import org.ossreviewtoolkit.model.RemoteArtifact
import org.ossreviewtoolkit.model.Repository
import org.ossreviewtoolkit.model.RepositoryProvenance
import org.ossreviewtoolkit.model.Scope
import org.ossreviewtoolkit.model.VcsInfo
import org.ossreviewtoolkit.model.VcsType
Expand Down Expand Up @@ -118,7 +119,7 @@ internal class CreateAnalyzerResultFromPackageListCommand : CliktCommand(
environment = Environment()
),
repository = Repository(
vcs = projectVcs.normalize(),
provenance = RepositoryProvenance(projectVcs.normalize(), projectVcs.revision),
config = RepositoryConfiguration(
excludes = Excludes(
scopes = listOf(
Expand Down
20 changes: 19 additions & 1 deletion model/src/main/kotlin/Provenance.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,16 @@ sealed interface Provenance {
* True if this [Provenance] refers to the same source code as [pkg], assuming that it belongs to the package id.
*/
fun matches(pkg: Package): Boolean

/**
Fixed Show fixed Hide fixed
* True if this [Provenance] matches the [Provenance][other] both in type and attributes.
*/
fun matches(other: Provenance): Boolean
}

data object UnknownProvenance : Provenance {
override fun matches(pkg: Package): Boolean = false
override fun matches(other: Provenance): Boolean = false
}

sealed interface KnownProvenance : Provenance
Expand All @@ -53,6 +59,8 @@ data class ArtifactProvenance(
val sourceArtifact: RemoteArtifact
) : KnownProvenance {
override fun matches(pkg: Package): Boolean = sourceArtifact == pkg.sourceArtifact
override fun matches(other: Provenance): Boolean =
other is ArtifactProvenance && sourceArtifact == other.sourceArtifact
}

/**
Expand All @@ -72,13 +80,23 @@ data class RepositoryProvenance(
val resolvedRevision: String
) : KnownProvenance {
init {
Copy link
Member

Choose a reason for hiding this comment

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

Why haven't you decided for keeping the require as-is, and make the tests construct instances with non-empty resolved revision?

I'm asking to me it seems that if repository provenance is sued, the VCS info should not be empty, because otherwise it would be an Unkown provenance, wouldn't it?

Copy link
Contributor Author

@pepper-jk pepper-jk Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You are probably correct. Changing the tests¹ to use "main" as revision instead of blank, like I did for model/src/test/kotlin/OrtResultTest.kt, might be the better way to solve the issue with the tests, than allowing blank resolvedRevisions in RepositoryProvenance for VcsInfo.Empty.

However, the Analyzer also allows VcsInfo.Empty to be passed from workingTree to the Repository. And with VcsInfo.Empty workingTree will very likely also return an empty revision. So in order to not change that behavior of the Analyzer and Repository in this case, I added this exception to RepositoryProvenance.

I have not yet noticed any issues regarding non-Repository uses of the RepositoryProvenance.

Still you are bringing up some good points, maybe the Analyzer behavior could change in this case? Then we could revert the exception to RepositoryProvenance. We could dial this in once we handle other Provenances in the Analyzer.

¹ evaluator/src/test/kotlin/TestData.kt
model/src/test/kotlin/licenses/TestData.kt
reporter/src/testFixtures/kotlin/TestData.kt

require(resolvedRevision.isNotBlank()) { "The resolved revision must not be blank." }
require(resolvedRevision.isNotBlank() || vcsInfo == VcsInfo.EMPTY) {
"The resolved revision must not be blank."
}
}

/**
* Return true if this provenance matches the processed VCS information of the [package][pkg].
*/
override fun matches(pkg: Package): Boolean = vcsInfo == pkg.vcsProcessed

/**
* Return true if the [Provenance][other] is a [RepositoryProvenance] and its normalized vcsInfo matches this
* provenance's normalized [VcsInfo][vcsInfo].
*/
override fun matches(other: Provenance): Boolean =
other is RepositoryProvenance && resolvedRevision == other.resolvedRevision &&
other.vcsInfo.normalize().equalsDisregardingPath(vcsInfo.normalize())
}

/**
Expand Down
38 changes: 24 additions & 14 deletions model/src/main/kotlin/Repository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.ossreviewtoolkit.model

import com.fasterxml.jackson.annotation.JsonIgnore
import com.fasterxml.jackson.annotation.JsonInclude

import org.ossreviewtoolkit.model.config.RepositoryConfiguration
Expand All @@ -31,13 +32,7 @@ data class Repository(
/**
* Original VCS-related information from the working tree containing the analyzer root.
*/
val vcs: VcsInfo,

/**
* Processed VCS-related information from the working tree containing the analyzer root that has e.g. common
* mistakes corrected.
*/
val vcsProcessed: VcsInfo = vcs.normalize(),
val provenance: KnownProvenance,

/**
* A map of nested repositories, for example Git submodules or Git-Repo modules. The key is the path to the
Expand All @@ -57,24 +52,39 @@ data class Repository(
*/
@JvmField
val EMPTY = Repository(
vcs = VcsInfo.EMPTY,
vcsProcessed = VcsInfo.EMPTY,
provenance = RepositoryProvenance(VcsInfo.EMPTY, ""),
nestedRepositories = emptyMap(),
config = RepositoryConfiguration()
)
}

@JsonIgnore
val vcs = if (provenance is RepositoryProvenance) {
provenance.vcsInfo
} else {
VcsInfo.EMPTY
}

@JsonIgnore
val vcsProcessed = if (provenance is RepositoryProvenance) {
provenance.vcsInfo.normalize()
} else {
VcsInfo.EMPTY
}

/**
* Return the path of [vcs] relative to [Repository.vcs], or null if [vcs] is neither [Repository.vcs] nor contained
* in [nestedRepositories].
* Return the path of [vcs] relative to [Repository.provenance], or null if [vcs] is neither
* [Repository.provenance] nor contained in [nestedRepositories].
*/
fun getRelativePath(vcs: VcsInfo): String? {
fun VcsInfo.matches(other: VcsInfo) = type == other.type && url == other.url && revision == other.revision
if (this.provenance !is RepositoryProvenance) return null

val normalizedVcs = vcs.normalize()

if (vcsProcessed.matches(normalizedVcs)) return ""
if (vcsProcessed.equalsDisregardingPath(normalizedVcs)) return ""

return nestedRepositories.entries.find { (_, nestedVcs) -> nestedVcs.normalize().matches(normalizedVcs) }?.key
return nestedRepositories.entries.find { (_, nestedVcs) ->
nestedVcs.normalize().equalsDisregardingPath(normalizedVcs)
}?.key
}
}
5 changes: 5 additions & 0 deletions model/src/main/kotlin/VcsInfo.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ data class VcsInfo(
* Return a [VcsInfoCurationData] with the properties from this [VcsInfo].
*/
fun toCuration() = VcsInfoCurationData(type, url, revision, path)

Copy link
Member

Choose a reason for hiding this comment

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

Commit message nits:

  • Duplicate "method" in first sentence.
  • Please use passive voice, i.e. no "we", "I", "us" etc.

Copy link
Contributor Author

@pepper-jk pepper-jk Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I overhauled the commit message after the changes to the commit.

EDIT: had to force push the old version, in order to see your reviews on the correct commit.
I will go through everything first before pushing any changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed new commits with code changes only.
I will squash them once they are reviewed and accepted.
Afterwards I will update the commit messages and force push.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed the squashed commits with the overhauled messages.

/**
* Return true if this [VcsInfo] equals the given [VcsInfo][other], disregarding the path.
*/
fun equalsDisregardingPath(other: VcsInfo) = type == other.type && url == other.url && revision == other.revision
}

/**
Expand Down
17 changes: 7 additions & 10 deletions model/src/test/assets/analyzer-result-with-dependency-graph.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
---
repository:
vcs:
type: "Git"
url: "https://github.com/vdurmont/semver4j.git"
revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
path: ""
vcs_processed:
type: "Git"
url: "https://github.com/vdurmont/semver4j.git"
revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
path: ""
provenance:
vcs_info:
type: "Git"
url: "https://github.com/vdurmont/semver4j.git"
revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
path: ""
resolved_revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
config: {}
analyzer:
start_time: "2021-04-26T05:48:05.390356Z"
Expand Down
17 changes: 7 additions & 10 deletions model/src/test/assets/gradle-all-dependencies-expected-result.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
---
repository:
vcs:
type: "Git"
url: "https://github.com/oss-review-toolkit/ort.git"
revision: "d27a886818b8ebba8e97e914133ec3cc650a534b"
path: "analyzer/src/funTest/assets/projects/synthetic/gradle"
vcs_processed:
type: "Git"
url: "https://github.com/oss-review-toolkit/ort.git"
revision: "d27a886818b8ebba8e97e914133ec3cc650a534b"
path: "analyzer/src/funTest/assets/projects/synthetic/gradle"
provenance:
vcs_info:
type: "Git"
url: "https://github.com/oss-review-toolkit/ort.git"
revision: "d27a886818b8ebba8e97e914133ec3cc650a534b"
path: "analyzer/src/funTest/assets/projects/synthetic/gradle"
resolved_revision: "d27a886818b8ebba8e97e914133ec3cc650a534b"
config:
excludes:
paths:
Expand Down
17 changes: 7 additions & 10 deletions model/src/test/assets/result-with-issues-graph-old.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
---
repository:
vcs:
type: "Git"
url: "https://github.com/pbassiner/sbt-multi-project-example.git"
revision: "31687c099ea6d645e819ef9d6ac9fc4c757a96bc"
path: ""
vcs_processed:
type: "Git"
url: "https://github.com/pbassiner/sbt-multi-project-example.git"
revision: "31687c099ea6d645e819ef9d6ac9fc4c757a96bc"
path: ""
provenance:
vcs_info:
type: "Git"
url: "https://github.com/pbassiner/sbt-multi-project-example.git"
revision: "31687c099ea6d645e819ef9d6ac9fc4c757a96bc"
path: ""
resolved_revision: "31687c099ea6d645e819ef9d6ac9fc4c757a96bc"
config: {}
analyzer:
start_time: "1970-01-01T00:00:00Z"
Expand Down
17 changes: 7 additions & 10 deletions model/src/test/assets/result-with-issues-graph.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
---
repository:
vcs:
type: "Git"
url: "https://github.com/pbassiner/sbt-multi-project-example.git"
revision: "31687c099ea6d645e819ef9d6ac9fc4c757a96bc"
path: ""
vcs_processed:
type: "Git"
url: "https://github.com/pbassiner/sbt-multi-project-example.git"
revision: "31687c099ea6d645e819ef9d6ac9fc4c757a96bc"
path: ""
provenance:
vcs_info:
type: "Git"
url: "https://github.com/pbassiner/sbt-multi-project-example.git"
revision: "31687c099ea6d645e819ef9d6ac9fc4c757a96bc"
path: ""
resolved_revision: "31687c099ea6d645e819ef9d6ac9fc4c757a96bc"
config: {}
analyzer:
start_time: "1970-01-01T00:00:00Z"
Expand Down
Loading
Loading