Skip to content

improve error message when using invalid path for local binary artifacts #4129

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

Merged
merged 1 commit into from
Feb 15, 2022
Merged

improve error message when using invalid path for local binary artifacts #4129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
87 changes: 67 additions & 20 deletions Sources/PackageLoading/ManifestLoader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public protocol ManifestLoaderProtocol {
}

public extension ManifestLoaderProtocol {
var supportedArchiveExtension: String { "zip" }
var supportedArchiveExtensions: [String] { ["zip"] }
}

public protocol ManifestLoaderDelegate {
Expand Down Expand Up @@ -408,30 +408,69 @@ public final class ManifestLoader: ManifestLoaderProtocol {
private func validateBinaryTargets(_ manifest: Manifest, observabilityScope: ObservabilityScope) throws {
// Check that binary targets point to the right file type.
for target in manifest.targets where target.type == .binary {
guard let location = URL(string: target.url ?? target.path ?? "") else {
if target.isLocal {
guard let path = target.path else {
observabilityScope.emit(.invalidBinaryLocation(targetName: target.name))
continue
}

guard let path = path.spm_chuzzle(), !path.isEmpty else {
observabilityScope.emit(.invalidLocalBinaryPath(path: path, targetName: target.name))
continue
}

guard let relativePath = try? RelativePath(validating: path) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this a do-catch and include the underlying error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is intentional - the underlying error in this case would be a generic "the path is an invalid relative path" which we fine-tune to invalidLocalBinaryPath which explains that this must be relative to the project root

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, the question is what happens if the underlying error ever changes and includes useful info. I guess that's not super likely in this particular case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, that was my thinking here as it basically validates that the value is valid relative path

observabilityScope.emit(.invalidLocalBinaryPath(path: path, targetName: target.name))
continue
}

let validExtensions = self.supportedArchiveExtensions + BinaryTarget.Kind.allCases.filter{ $0 != .unknown }.map { $0.fileExtension }
guard let fileExtension = relativePath.extension, validExtensions.contains(fileExtension) else {
observabilityScope.emit(.unsupportedBinaryLocationExtension(
targetName: target.name,
validExtensions: validExtensions
))
continue
}
} else if target.isRemote {
guard let url = target.url else {
observabilityScope.emit(.invalidBinaryLocation(targetName: target.name))
continue
}

guard let url = url.spm_chuzzle(), !url.isEmpty else {
observabilityScope.emit(.invalidBinaryURL(url: url, targetName: target.name))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think each error case should have some distinction, e.g. this should say that the URL is empty. Just makes it easier when someone unexpectedly hits one of these, we won't have to guess which case they are encountering. Similar for invalidLocalBinaryPath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @neonichu we could add a special case for empty vs. invalid, tho we do print the bad URL/Path to make it obvious. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry, I missed that we are printing the URL now, that should be sufficient here.

continue
}

guard let url = URL(string: url) else {
observabilityScope.emit(.invalidBinaryURL(url: url, targetName: target.name))
continue
}

let validSchemes = ["https"]
guard url.scheme.map({ validSchemes.contains($0) }) ?? false else {
observabilityScope.emit(.invalidBinaryURLScheme(
targetName: target.name,
validSchemes: validSchemes
))
continue
}

guard self.supportedArchiveExtensions.contains(url.pathExtension) else {
observabilityScope.emit(.unsupportedBinaryLocationExtension(
targetName: target.name,
validExtensions: self.supportedArchiveExtensions
))
continue
}

} else {
observabilityScope.emit(.invalidBinaryLocation(targetName: target.name))
continue
}

let validSchemes = ["https"]
if target.isRemote && (location.scheme.map({ !validSchemes.contains($0) }) ?? true) {
observabilityScope.emit(.invalidBinaryURLScheme(
targetName: target.name,
validSchemes: validSchemes
))
}

var validExtensions = [self.supportedArchiveExtension]
if target.isLocal {
validExtensions += BinaryTarget.Kind.allCases.filter{ $0 != .unknown }.map { $0.fileExtension }
}

if !validExtensions.contains(location.pathExtension) {
observabilityScope.emit(.unsupportedBinaryLocationExtension(
targetName: target.name,
validExtensions: validExtensions
))
}
}
}

Expand Down Expand Up @@ -1070,6 +1109,14 @@ extension Basics.Diagnostic {
.error("invalid location for binary target '\(targetName)'")
}

static func invalidBinaryURL(url: String, targetName: String) -> Self {
.error("invalid URL '\(url)' for binary target '\(targetName)'")
}

static func invalidLocalBinaryPath(path: String, targetName: String) -> Self {
.error("invalid local path '\(path)' for binary target '\(targetName)', path expected to be relative to package root.")
}

static func invalidBinaryURLScheme(targetName: String, validSchemes: [String]) -> Self {
.error("invalid URL scheme for binary target '\(targetName)'; valid schemes are: '\(validSchemes.joined(separator: "', '"))'")
}
Expand Down
8 changes: 4 additions & 4 deletions Sources/Workspace/Workspace.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2282,7 +2282,7 @@ extension Workspace {
addedOrUpdatedPackages: [PackageReference],
observabilityScope: ObservabilityScope
) throws {
let manifestArtifacts = try self.parseArtifacts(from: manifests)
let manifestArtifacts = try self.parseArtifacts(from: manifests, observabilityScope: observabilityScope)

var artifactsToRemove: [ManagedArtifact] = []
var artifactsToAdd: [ManagedArtifact] = []
Expand All @@ -2302,7 +2302,7 @@ extension Workspace {
targetName: artifact.targetName
]

if artifact.path.extension == self.manifestLoader.supportedArchiveExtension {
if let fileExtension = artifact.path.extension, self.manifestLoader.supportedArchiveExtensions.contains(fileExtension) {
// If we already have an artifact that was extracted from an archive with the same checksum,
// we don't need to extract the artifact again.
if case .local(let existingChecksum) = existingArtifact?.source, existingChecksum == (try self.checksum(forBinaryArtifactAt: artifact.path)) {
Expand Down Expand Up @@ -2393,7 +2393,7 @@ extension Workspace {
}
}

private func parseArtifacts(from manifests: DependencyManifests) throws -> (local: [ManagedArtifact], remote: [RemoteArtifact]) {
private func parseArtifacts(from manifests: DependencyManifests, observabilityScope: ObservabilityScope) throws -> (local: [ManagedArtifact], remote: [RemoteArtifact]) {
let packageAndManifests: [(reference: PackageReference, manifest: Manifest)] =
manifests.root.packages.values + // Root package and manifests.
manifests.dependencies.map({ manifest, managed, _, _ in (managed.packageRef, manifest) }) // Dependency package and manifests.
Expand Down Expand Up @@ -2436,7 +2436,7 @@ extension Workspace {
// zip files to download
// stored in a thread-safe way as we may fetch more from "artifactbundleindex" files
let zipArtifacts = ThreadSafeArrayStore<RemoteArtifact>(artifacts.filter {
$0.url.pathExtension.lowercased() == self.manifestLoader.supportedArchiveExtension
self.manifestLoader.supportedArchiveExtensions.contains($0.url.pathExtension.lowercased())
})

// fetch and parse "artifactbundleindex" files, if any
Expand Down
73 changes: 72 additions & 1 deletion Tests/PackageLoadingTests/PD_5_3_LoadingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ class PackageDescription5_3LoadingTests: PackageDescriptionLoadingTests {
let observability = ObservabilitySystem.makeForTesting()
XCTAssertThrowsError(try loadManifest(content, observabilityScope: observability.topScope), "expected error")
testDiagnostics(observability.diagnostics) { result in
result.check(diagnostic: "invalid location for binary target 'Foo'", severity: .error)
result.check(diagnostic: "invalid local path ' ' for binary target 'Foo', path expected to be relative to package root.", severity: .error)
}
}

Expand Down Expand Up @@ -348,6 +348,77 @@ class PackageDescription5_3LoadingTests: PackageDescriptionLoadingTests {
result.check(diagnostic: "unsupported extension for binary target 'Foo'; valid extensions are: 'zip'", severity: .error)
}
}

do {
let content = """
import PackageDescription
let package = Package(
name: "Foo",
products: [
.library(name: "Foo", targets: ["Foo"]),
],
targets: [
.binaryTarget(
name: "Foo",
url: "ssh://foo/bar",
checksum: "839F9F30DC13C30795666DD8F6FB77DD0E097B83D06954073E34FE5154481F7A"),
]
)
"""

let observability = ObservabilitySystem.makeForTesting()
XCTAssertThrowsError(try loadManifest(content, observabilityScope: observability.topScope), "expected error")
testDiagnostics(observability.diagnostics) { result in
result.check(diagnostic: "invalid URL scheme for binary target 'Foo'; valid schemes are: 'https'", severity: .error)
}
}

do {
let content = """
import PackageDescription
let package = Package(
name: "Foo",
products: [
.library(name: "Foo", targets: ["Foo"]),
],
targets: [
.binaryTarget(
name: "Foo",
url: " ",
checksum: "839F9F30DC13C30795666DD8F6FB77DD0E097B83D06954073E34FE5154481F7A"),
]
)
"""

let observability = ObservabilitySystem.makeForTesting()
XCTAssertThrowsError(try loadManifest(content, observabilityScope: observability.topScope), "expected error")
testDiagnostics(observability.diagnostics) { result in
result.check(diagnostic: "invalid URL ' ' for binary target 'Foo'", severity: .error)
}
}

do {
let content = """
import PackageDescription
let package = Package(
name: "Foo",
products: [
.library(name: "Foo", targets: ["Foo"]),
],
targets: [
.binaryTarget(
name: "Foo",
path: "/tmp/foo/bar")
]
)
"""

let observability = ObservabilitySystem.makeForTesting()
XCTAssertThrowsError(try loadManifest(content, observabilityScope: observability.topScope), "expected error")
testDiagnostics(observability.diagnostics) { result in
result.check(diagnostic: "invalid local path '/tmp/foo/bar' for binary target 'Foo', path expected to be relative to package root.", severity: .error)
}
}
}

func testConditionalTargetDependencies() throws {
Expand Down
12 changes: 6 additions & 6 deletions Tests/WorkspaceTests/WorkspaceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9333,13 +9333,13 @@ final class WorkspaceTests: XCTestCase {
delegate: MockWorkspaceDelegate()
)

do {
try workspace.resolve(root: .init(packages: [foo]), observabilityScope: observability.topScope)
} catch {
XCTAssertEqual(error.localizedDescription, "invalid relative path '/best.xcframework'; relative path should not begin with '/' or '~'")
return
try workspace.resolve(root: .init(packages: [foo]), observabilityScope: observability.topScope)
testDiagnostics(observability.diagnostics) { result in
result.check(
diagnostic: "invalid local path '/best.xcframework' for binary target 'best', path expected to be relative to package root.",
severity: .error
)
}
XCTFail("unexpected success")
}
}

Expand Down