Skip to content

[5.3] Improve the diagnostics that are emitted when a package dependency specifies a non-existent branch or commit #2950

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 26 additions & 3 deletions Sources/PackageGraph/RepositoryPackageContainerProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,9 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve

/// The actual error that occurred.
public let underlyingError: Swift.Error

/// Optional suggestion for how to resolve the error.
public let suggestion: String?
}

/// This is used to remember if tools version of a particular version is
Expand Down Expand Up @@ -371,7 +374,7 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve
}.1
} catch {
throw GetDependenciesErrorWrapper(
containerIdentifier: identifier.repository.url, reference: version.description, underlyingError: error)
containerIdentifier: identifier.repository.url, reference: version.description, underlyingError: error, suggestion: nil)
}
}

Expand All @@ -383,8 +386,28 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve
return try getDependencies(at: revision)
}.1
} catch {
throw GetDependenciesErrorWrapper(
containerIdentifier: identifier.repository.url, reference: revision, underlyingError: error)
// Examine the error to see if we can come up with a more informative and actionable error message. We know that the revision is expected to be a branch name or a hash (tags are handled through a different code path).
if let gitInvocationError = error as? ProcessResult.Error, gitInvocationError.description.contains("Needed a single revision") {
// It was a Git process invocation error. Take a look at the repository to see if we can come up with a reasonable diagnostic.
if let rev = try? repository.resolveRevision(identifier: revision), repository.exists(revision: rev) {
// Revision does exist, so something else must be wrong.
throw GetDependenciesErrorWrapper(
containerIdentifier: identifier.repository.url, reference: revision, underlyingError: error, suggestion: nil)
}
else {
// Revision does not exist, so we customize the error.
let sha1RegEx = try! RegEx(pattern: #"\A[:xdigit:]{40}\Z"#)
let isBranchRev = sha1RegEx.matchGroups(in: revision).compactMap{ $0 }.isEmpty
let errorMessage = "could not find " + (isBranchRev ? "a branch named ‘\(revision)’" : "the commit \(revision)")
let mainBranchExists = (try? repository.resolveRevision(identifier: "main")) != nil
let suggestion = (revision == "master" && mainBranchExists) ? "did you mean ‘main’?" : nil
throw GetDependenciesErrorWrapper(
containerIdentifier: identifier.repository.url, reference: revision,
underlyingError: StringError(errorMessage), suggestion: suggestion)
}
}
// If we get this far without having thrown an error, we wrap and throw the underlying error.
throw GetDependenciesErrorWrapper(containerIdentifier: identifier.repository.url, reference: revision, underlyingError: error, suggestion: nil)
}
}

Expand Down
3 changes: 2 additions & 1 deletion Sources/Workspace/Workspace.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2007,7 +2007,8 @@ extension Workspace {
// Emit proper error if we were not able to parse some manifest during dependency resolution.
case let error as RepositoryPackageContainer.GetDependenciesErrorWrapper:
let location = PackageLocation.Remote(url: error.containerIdentifier, reference: error.reference)
diagnostics.emit(error.underlyingError, location: location)
let suggestion = error.suggestion.flatMap{ " (\($0))" }
diagnostics.emit(StringError("\(error.underlyingError)\(suggestion ?? "")"), location: location)

default:
diagnostics.emit(error)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import XCTest

import TSCBasic
import TSCUtility
import PackageLoading
import PackageModel
import PackageGraph
Expand Down Expand Up @@ -478,4 +479,62 @@ class RepositoryPackageContainerProviderTests: XCTestCase {
)
}
}

func testMissingBranchDiagnostics() {
mktmpdir { tmpDir in
// Create a repository.
let packageDir = tmpDir.appending(component: "SomePackage")
try localFileSystem.createDirectory(packageDir)
initGitRepo(packageDir)
let packageRepo = GitRepository(path: packageDir)

// Create a package manifest in it (it only needs the `swift-tools-version` part, because we'll supply the manifest later).
let manifestFile = packageDir.appending(component: "Package.swift")
try localFileSystem.writeFileContents(manifestFile, bytes: ByteString("// swift-tools-version:4.2"))

// Commit it and tag it.
try packageRepo.stage(file: "Package.swift")
try packageRepo.commit(message: "Initial")
try packageRepo.tag(name: "1.0.0")

// Rename the `master` branch to `main`.
try systemQuietly([Git.tool, "-C", packageDir.pathString, "branch", "-m", "main"])

// Create a repository manager for it.
let repoProvider = GitRepositoryProvider()
let repositoryManager = RepositoryManager(path: packageDir, provider: repoProvider, delegate: nil, fileSystem: localFileSystem)

// Create a container provider, configured with a mock manifest loader that will return the package manifest.
let manifest = Manifest.createV4Manifest(
name: packageDir.basename,
path: packageDir.pathString,
url: packageDir.pathString,
packageKind: .root,
targets: [
TargetDescription(name: packageDir.basename, path: packageDir.pathString)
]
)
let containerProvider = RepositoryPackageContainerProvider(repositoryManager: repositoryManager, manifestLoader: MockManifestLoader(manifests: [.init(url: packageDir.pathString, version: nil) : manifest]))

// Get a hold of the container for the test package.
let packageRef = PackageReference(identity: "somepackage", path: packageDir.pathString)
let container = try await { containerProvider.getContainer(for: packageRef, completion: $0) } as! RepositoryPackageContainer

// Simulate accessing a fictitious dependency on the `master` branch, and check that we get back the expected error.
do { _ = try container.getDependencies(at: "master") }
catch let error as RepositoryPackageContainer.GetDependenciesErrorWrapper {
// We expect to get an error message about `master` with a suggestion to use `main`.
XCTAssertEqual("\(error.underlyingError)", "could not find a branch named ‘master’")
XCTAssertEqual(error.suggestion, "did you mean ‘main’?")
}

// Simulate accessing a fictitious dependency on some random commit that doesn't exist, and check that we get back the expected error.
do { _ = try container.getDependencies(at: "535f4cb5b4a0872fa691473e82d7b27b9894df00") }
catch let error as RepositoryPackageContainer.GetDependenciesErrorWrapper {
// We expect to get an error message about the commit SHA, but without a suggestion.
XCTAssertMatch("\(error.underlyingError)", "could not find the commit 535f4cb5b4a0872fa691473e82d7b27b9894df00")
XCTAssertEqual(error.suggestion, nil)
}
}
}
}