From 9992382a2e4772fb73f6b8b35e7e3c87a4c73be7 Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Thu, 25 Apr 2024 18:39:18 +0200 Subject: [PATCH 1/6] chore(vcs): Remove some redundant failure logging Slightly improve logging in the caller to be able to remove similar logging in some implementations. Signed-off-by: Sebastian Schuberth --- downloader/src/main/kotlin/VersionControlSystem.kt | 4 +++- .../mercurial/src/main/kotlin/Mercurial.kt | 6 ------ .../subversion/src/main/kotlin/Subversion.kt | 7 ------- 3 files changed, 3 insertions(+), 14 deletions(-) diff --git a/downloader/src/main/kotlin/VersionControlSystem.kt b/downloader/src/main/kotlin/VersionControlSystem.kt index ced80cea2412a..4116a14852756 100644 --- a/downloader/src/main/kotlin/VersionControlSystem.kt +++ b/downloader/src/main/kotlin/VersionControlSystem.kt @@ -251,7 +251,9 @@ abstract class VersionControlSystem( } val workingTreeRevision = results.last().getOrElse { - throw DownloadException("$type failed to download from URL ${pkg.vcsProcessed.url}.", it) + throw DownloadException( + "$type failed to download from ${pkg.vcsProcessed.url} to '${workingTree.workingDir}'.", it + ) } pkg.vcsProcessed.path.let { diff --git a/plugins/version-control-systems/mercurial/src/main/kotlin/Mercurial.kt b/plugins/version-control-systems/mercurial/src/main/kotlin/Mercurial.kt index 65c13fcc0a002..133c278667dfd 100644 --- a/plugins/version-control-systems/mercurial/src/main/kotlin/Mercurial.kt +++ b/plugins/version-control-systems/mercurial/src/main/kotlin/Mercurial.kt @@ -29,8 +29,6 @@ import org.ossreviewtoolkit.model.VcsInfo import org.ossreviewtoolkit.model.VcsType import org.ossreviewtoolkit.utils.common.CommandLineTool import org.ossreviewtoolkit.utils.common.ProcessCapture -import org.ossreviewtoolkit.utils.common.collectMessages -import org.ossreviewtoolkit.utils.ort.showStackTrace const val MERCURIAL_LARGE_FILES_EXTENSION = "largefiles = " const val MERCURIAL_SPARSE_EXTENSION = "sparse = " @@ -104,10 +102,6 @@ class Mercurial : VersionControlSystem(MercurialCommand) { // Explicitly update the working tree to the desired revision. MercurialCommand.run(workingTree.workingDir, "update", revision).isSuccess - }.onFailure { - it.showStackTrace() - - logger.warn { "Failed to update $type working tree to revision '$revision': ${it.collectMessages()}" } }.map { revision } diff --git a/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt b/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt index fd1cdcdbc84ac..2a7bfd46c30f5 100644 --- a/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt +++ b/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt @@ -169,13 +169,6 @@ class Subversion : VersionControlSystem() { } true - }.onFailure { - it.showStackTrace() - - logger.warn { - "Failed to update the $type working tree at '${workingTree.workingDir}' to revision '$revision':\n" + - it.collectMessages() - } }.map { revision } From 8be63cb3b5d97ff34bb2b904db64a79ae4cac8d3 Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Thu, 25 Apr 2024 17:27:09 +0200 Subject: [PATCH 2/6] chore(subversion): Set `isIgnoreExternals` only once This changes the state of the `updateClient` and only needs to be set once. Signed-off-by: Sebastian Schuberth --- .../subversion/src/main/kotlin/Subversion.kt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt b/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt index 2a7bfd46c30f5..6c4f6385bc436 100644 --- a/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt +++ b/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt @@ -122,6 +122,8 @@ class Subversion : VersionControlSystem() { override fun updateWorkingTree(workingTree: WorkingTree, revision: String, path: String, recursive: Boolean) = runCatching { + clientManager.updateClient.isIgnoreExternals = !recursive + revision.toLongOrNull()?.let { numericRevision -> // This code path updates the working tree to a numeric revision. val svnRevision = SVNRevision.create(numericRevision) @@ -130,7 +132,7 @@ class Subversion : VersionControlSystem() { updateEmptyPath(workingTree, svnRevision, path) // Then deepen only the requested path in the desired revision. - clientManager.updateClient.apply { isIgnoreExternals = !recursive }.doUpdate( + clientManager.updateClient.doUpdate( workingTree.workingDir.resolve(path), svnRevision, SVNDepth.INFINITY, @@ -159,7 +161,7 @@ class Subversion : VersionControlSystem() { updateEmptyPath(workingTree, SVNRevision.HEAD, path) // Finally, deepen only the requested path in the current revision. - clientManager.updateClient.apply { isIgnoreExternals = !recursive }.doUpdate( + clientManager.updateClient.doUpdate( workingTree.workingDir.resolve(path), SVNRevision.HEAD, SVNDepth.INFINITY, From 9fc7430eb2a2594e5889b6be77e63236e8f7a058 Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Thu, 25 Apr 2024 17:40:34 +0200 Subject: [PATCH 3/6] fix(subversion): Return the actual revision instead of the requested one One should not expect checking out the requested revision to always go well, but return the actual revision so the caller can double-check the requested against the actual revision. Signed-off-by: Sebastian Schuberth --- .../subversion/src/main/kotlin/Subversion.kt | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt b/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt index 6c4f6385bc436..3daa7974ea097 100644 --- a/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt +++ b/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt @@ -169,10 +169,8 @@ class Subversion : VersionControlSystem() { /* depthIsSticky = */ true ) } - - true }.map { - revision + it.toString() } } From d9eabcbb534aa1199a0a99401a188dac157500db Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Thu, 25 Apr 2024 12:00:33 +0200 Subject: [PATCH 4/6] refactor(subversion): Unify code paths for (non-)numeric revisions Simplify code to only do a single `svn switch`, and eventually an `svn update` if a path is specified. Signed-off-by: Sebastian Schuberth --- .../subversion/src/main/kotlin/Subversion.kt | 91 +++++++++---------- 1 file changed, 43 insertions(+), 48 deletions(-) diff --git a/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt b/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt index 3daa7974ea097..76756b6612113 100644 --- a/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt +++ b/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt @@ -103,74 +103,69 @@ class Subversion : VersionControlSystem() { return getWorkingTree(targetDir) } - private fun updateEmptyPath(workingTree: WorkingTree, revision: SVNRevision, path: String) { - val pathIterator = Paths.get(path).iterator() + private fun deepenWorkingTreePath(workingTree: WorkingTree, path: String, revision: SVNRevision): Long { + val finalPath = workingTree.workingDir.resolve(path) var currentPath = workingTree.workingDir + // Avoid the "None of the targets are working copies" error by deepening one path at a time. + val pathIterator = Paths.get(path).iterator() + val pathRevisions = mutableSetOf() + while (pathIterator.hasNext()) { - clientManager.updateClient.doUpdate( + currentPath = currentPath.resolve(pathIterator.next().toFile()) + + pathRevisions += clientManager.updateClient.doUpdate( currentPath, revision, - SVNDepth.EMPTY, + if (currentPath != finalPath) SVNDepth.EMPTY else SVNDepth.INFINITY, /* allowUnversionedObstructions = */ false, /* depthIsSticky = */ true ) - - currentPath = currentPath.resolve(pathIterator.next().toFile()) } + + return pathRevisions.single() } override fun updateWorkingTree(workingTree: WorkingTree, revision: String, path: String, recursive: Boolean) = runCatching { - clientManager.updateClient.isIgnoreExternals = !recursive + // Note that the path should never be part of the URL as that would root the working tree at that path, but + // the path should be available in the working tree. + val (svnUrl, svnRevision) = revision.toLongOrNull()?.let { numericRevision -> + val url = workingTree.getRemoteUrl() + + SVNURL.parseURIEncoded(url) to SVNRevision.create(numericRevision) + } ?: run { + val url = listOf(workingTree.getRemoteUrl(), revision).joinToString("/") - revision.toLongOrNull()?.let { numericRevision -> - // This code path updates the working tree to a numeric revision. - val svnRevision = SVNRevision.create(numericRevision) + SVNURL.parseURIEncoded(url) to SVNRevision.HEAD + } - // First update the (empty) working tree to the desired revision along the requested path. - updateEmptyPath(workingTree, svnRevision, path) + clientManager.updateClient.isIgnoreExternals = !recursive - // Then deepen only the requested path in the desired revision. - clientManager.updateClient.doUpdate( - workingTree.workingDir.resolve(path), - svnRevision, - SVNDepth.INFINITY, - /* allowUnversionedObstructions = */ false, - /* depthIsSticky = */ true - ) - } ?: run { - // This code path updates the working tree to a symbolic revision. - val svnUrl = SVNURL.parseURIEncoded( - "${workingTree.getRemoteUrl()}/$revision" - ) + logger.info { + val printableRevision = svnRevision.name ?: svnRevision.number + "Switching $type '${workingTree.workingDir}' to $svnUrl at revision $printableRevision." + } - // First switch the (empty) working tree to the requested branch / tag. - clientManager.updateClient.doSwitch( - workingTree.workingDir, - svnUrl, - SVNRevision.HEAD, - SVNRevision.HEAD, - SVNDepth.EMPTY, - /* allowUnversionedObstructions = */ false, - /* depthIsSticky = */ true, - /* ignoreAncestry = */ true - ) + val workingTreeRevision = clientManager.updateClient.doSwitch( + workingTree.workingDir, + svnUrl, + svnRevision, + svnRevision, + if (path.isEmpty()) SVNDepth.INFINITY else SVNDepth.EMPTY, + /* allowUnversionedObstructions = */ false, + /* depthIsSticky = */ true + ) - // Then update the working tree in the current revision along the requested path, and ... - updateEmptyPath(workingTree, SVNRevision.HEAD, path) + logger.info { "$type working tree '${workingTree.workingDir}' is at revision $workingTreeRevision." } - // Finally, deepen only the requested path in the current revision. - clientManager.updateClient.doUpdate( - workingTree.workingDir.resolve(path), - SVNRevision.HEAD, - SVNDepth.INFINITY, - /* allowUnversionedObstructions = */ false, - /* depthIsSticky = */ true - ) + if (path.isNotEmpty()) { + logger.info { "Deepening path '$path' in $type working tree '${workingTree.workingDir}'." } + val pathRevision = deepenWorkingTreePath(workingTree, path, svnRevision) + check(pathRevision == workingTreeRevision) } - }.map { - it.toString() + + workingTreeRevision.toString() } } From f5465f4dad06a6c20776f8e27eb949834954db63 Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Thu, 25 Apr 2024 18:23:03 +0200 Subject: [PATCH 5/6] fix(subversion): Make the peg revision newer than the operative revision Ensure that always the first algorithm mentioned at [1] is applied. This avoids errors e.g. when updating to a resolved revision of a tag path. Fixes #6160. [1]: https://svnbook.red-bean.com/en/1.7/svn.advanced.pegrevs.html Signed-off-by: Sebastian Schuberth --- .../subversion/src/main/kotlin/Subversion.kt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt b/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt index 76756b6612113..5a4a217fb32f8 100644 --- a/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt +++ b/plugins/version-control-systems/subversion/src/main/kotlin/Subversion.kt @@ -147,11 +147,12 @@ class Subversion : VersionControlSystem() { "Switching $type '${workingTree.workingDir}' to $svnUrl at revision $printableRevision." } + // For "peg revision" vs. "revision" see https://svnbook.red-bean.com/en/1.7/svn.advanced.pegrevs.html. val workingTreeRevision = clientManager.updateClient.doSwitch( workingTree.workingDir, svnUrl, - svnRevision, - svnRevision, + /* pegRevision = */ SVNRevision.HEAD, + /* revision = */ svnRevision, if (path.isEmpty()) SVNDepth.INFINITY else SVNDepth.EMPTY, /* allowUnversionedObstructions = */ false, /* depthIsSticky = */ true From 8bebd959ff3ba1c670a34db1b8844a6bf6714e83 Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Thu, 25 Apr 2024 18:34:09 +0200 Subject: [PATCH 6/6] test(scanner): Enable a Subversion test that works now Recent changes made the test work. However, it may take up to 20 minutes to complete, so mark it as expensive. Signed-off-by: Sebastian Schuberth --- .../provenance/DefaultNestedProvenanceResolverFunTest.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scanner/src/funTest/kotlin/provenance/DefaultNestedProvenanceResolverFunTest.kt b/scanner/src/funTest/kotlin/provenance/DefaultNestedProvenanceResolverFunTest.kt index 228042395a273..8ec4de190df98 100644 --- a/scanner/src/funTest/kotlin/provenance/DefaultNestedProvenanceResolverFunTest.kt +++ b/scanner/src/funTest/kotlin/provenance/DefaultNestedProvenanceResolverFunTest.kt @@ -36,6 +36,7 @@ import org.ossreviewtoolkit.model.VcsInfo import org.ossreviewtoolkit.model.VcsType import org.ossreviewtoolkit.scanner.utils.DefaultWorkingTreeCache import org.ossreviewtoolkit.utils.common.Os +import org.ossreviewtoolkit.utils.test.ExpensiveTag class DefaultNestedProvenanceResolverFunTest : WordSpec() { private val workingTreeCache = DefaultWorkingTreeCache() @@ -204,7 +205,7 @@ class DefaultNestedProvenanceResolverFunTest : WordSpec() { } } - "work for Subversion tags".config(enabled = false /* This needs fixing, see ORT issue 6160. */) { + "work for Subversion tags".config(tags = setOf(ExpensiveTag)) { val provenance = RepositoryProvenance( vcsInfo = VcsInfo( type = VcsType.SUBVERSION,