Skip to content

Commit df41ada

Browse files
tomerdtomerd
committed
improve error message when using invalid path for local binary artifacts
motivation: clearer, more actionable error message changes: * catch invalid relative path for binary artifacts and emit a clear diagnostics * overall improve validation of binary targets path and urls * add tests rdar://84365161
1 parent ff6daf7 commit df41ada

File tree

4 files changed

+149
-31
lines changed

4 files changed

+149
-31
lines changed

‎Sources/PackageLoading/ManifestLoader.swift

Lines changed: 67 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ public protocol ManifestLoaderProtocol {
7676
}
7777

7878
public extension ManifestLoaderProtocol {
79-
var supportedArchiveExtension: String { "zip" }
79+
var supportedArchiveExtensions: [String] { ["zip"] }
8080
}
8181

8282
public protocol ManifestLoaderDelegate {
@@ -408,30 +408,69 @@ public final class ManifestLoader: ManifestLoaderProtocol {
408408
private func validateBinaryTargets(_ manifest: Manifest, observabilityScope: ObservabilityScope) throws {
409409
// Check that binary targets point to the right file type.
410410
for target in manifest.targets where target.type == .binary {
411-
guard let location = URL(string: target.url ?? target.path ?? "") else {
411+
if target.isLocal {
412+
guard let path = target.path else {
413+
observabilityScope.emit(.invalidBinaryLocation(targetName: target.name))
414+
continue
415+
}
416+
417+
guard let path = path.spm_chuzzle(), !path.isEmpty else {
418+
observabilityScope.emit(.invalidLocalBinaryPath(path: path, targetName: target.name))
419+
continue
420+
}
421+
422+
guard let relativePath = try? RelativePath(validating: path) else {
423+
observabilityScope.emit(.invalidLocalBinaryPath(path: path, targetName: target.name))
424+
continue
425+
}
426+
427+
let validExtensions = self.supportedArchiveExtensions + BinaryTarget.Kind.allCases.filter{ $0 != .unknown }.map { $0.fileExtension }
428+
guard let fileExtension = relativePath.extension, validExtensions.contains(fileExtension) else {
429+
observabilityScope.emit(.unsupportedBinaryLocationExtension(
430+
targetName: target.name,
431+
validExtensions: validExtensions
432+
))
433+
continue
434+
}
435+
} else if target.isRemote {
436+
guard let url = target.url else {
437+
observabilityScope.emit(.invalidBinaryLocation(targetName: target.name))
438+
continue
439+
}
440+
441+
guard let url = url.spm_chuzzle(), !url.isEmpty else {
442+
observabilityScope.emit(.invalidBinaryURL(url: url, targetName: target.name))
443+
continue
444+
}
445+
446+
guard let url = URL(string: url) else {
447+
observabilityScope.emit(.invalidBinaryURL(url: url, targetName: target.name))
448+
continue
449+
}
450+
451+
let validSchemes = ["https"]
452+
guard url.scheme.map({ validSchemes.contains($0) }) ?? false else {
453+
observabilityScope.emit(.invalidBinaryURLScheme(
454+
targetName: target.name,
455+
validSchemes: validSchemes
456+
))
457+
continue
458+
}
459+
460+
guard self.supportedArchiveExtensions.contains(url.pathExtension) else {
461+
observabilityScope.emit(.unsupportedBinaryLocationExtension(
462+
targetName: target.name,
463+
validExtensions: self.supportedArchiveExtensions
464+
))
465+
continue
466+
}
467+
468+
} else {
412469
observabilityScope.emit(.invalidBinaryLocation(targetName: target.name))
413470
continue
414471
}
415472

416-
let validSchemes = ["https"]
417-
if target.isRemote && (location.scheme.map({ !validSchemes.contains($0) }) ?? true) {
418-
observabilityScope.emit(.invalidBinaryURLScheme(
419-
targetName: target.name,
420-
validSchemes: validSchemes
421-
))
422-
}
423473

424-
var validExtensions = [self.supportedArchiveExtension]
425-
if target.isLocal {
426-
validExtensions += BinaryTarget.Kind.allCases.filter{ $0 != .unknown }.map { $0.fileExtension }
427-
}
428-
429-
if !validExtensions.contains(location.pathExtension) {
430-
observabilityScope.emit(.unsupportedBinaryLocationExtension(
431-
targetName: target.name,
432-
validExtensions: validExtensions
433-
))
434-
}
435474
}
436475
}
437476

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

1112+
static func invalidBinaryURL(url: String, targetName: String) -> Self {
1113+
.error("invalid URL '\(url)' for binary target '\(targetName)'")
1114+
}
1115+
1116+
static func invalidLocalBinaryPath(path: String, targetName: String) -> Self {
1117+
.error("invalid local path '\(path)' for binary target '\(targetName)', path expected to be relative to package root.")
1118+
}
1119+
10731120
static func invalidBinaryURLScheme(targetName: String, validSchemes: [String]) -> Self {
10741121
.error("invalid URL scheme for binary target '\(targetName)'; valid schemes are: '\(validSchemes.joined(separator: "', '"))'")
10751122
}

‎Sources/Workspace/Workspace.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2282,7 +2282,7 @@ extension Workspace {
22822282
addedOrUpdatedPackages: [PackageReference],
22832283
observabilityScope: ObservabilityScope
22842284
) throws {
2285-
let manifestArtifacts = try self.parseArtifacts(from: manifests)
2285+
let manifestArtifacts = try self.parseArtifacts(from: manifests, observabilityScope: observabilityScope)
22862286

22872287
var artifactsToRemove: [ManagedArtifact] = []
22882288
var artifactsToAdd: [ManagedArtifact] = []
@@ -2302,7 +2302,7 @@ extension Workspace {
23022302
targetName: artifact.targetName
23032303
]
23042304

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

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

24422442
// fetch and parse "artifactbundleindex" files, if any

‎Tests/PackageLoadingTests/PD_5_3_LoadingTests.swift

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ class PackageDescription5_3LoadingTests: PackageDescriptionLoadingTests {
234234
let observability = ObservabilitySystem.makeForTesting()
235235
XCTAssertThrowsError(try loadManifest(content, observabilityScope: observability.topScope), "expected error")
236236
testDiagnostics(observability.diagnostics) { result in
237-
result.check(diagnostic: "invalid location for binary target 'Foo'", severity: .error)
237+
result.check(diagnostic: "invalid local path ' ' for binary target 'Foo', path expected to be relative to package root.", severity: .error)
238238
}
239239
}
240240

@@ -348,6 +348,77 @@ class PackageDescription5_3LoadingTests: PackageDescriptionLoadingTests {
348348
result.check(diagnostic: "unsupported extension for binary target 'Foo'; valid extensions are: 'zip'", severity: .error)
349349
}
350350
}
351+
352+
do {
353+
let content = """
354+
import PackageDescription
355+
let package = Package(
356+
name: "Foo",
357+
products: [
358+
.library(name: "Foo", targets: ["Foo"]),
359+
],
360+
targets: [
361+
.binaryTarget(
362+
name: "Foo",
363+
url: "ssh://foo/bar",
364+
checksum: "839F9F30DC13C30795666DD8F6FB77DD0E097B83D06954073E34FE5154481F7A"),
365+
]
366+
)
367+
"""
368+
369+
let observability = ObservabilitySystem.makeForTesting()
370+
XCTAssertThrowsError(try loadManifest(content, observabilityScope: observability.topScope), "expected error")
371+
testDiagnostics(observability.diagnostics) { result in
372+
result.check(diagnostic: "invalid URL scheme for binary target 'Foo'; valid schemes are: 'https'", severity: .error)
373+
}
374+
}
375+
376+
do {
377+
let content = """
378+
import PackageDescription
379+
let package = Package(
380+
name: "Foo",
381+
products: [
382+
.library(name: "Foo", targets: ["Foo"]),
383+
],
384+
targets: [
385+
.binaryTarget(
386+
name: "Foo",
387+
url: " ",
388+
checksum: "839F9F30DC13C30795666DD8F6FB77DD0E097B83D06954073E34FE5154481F7A"),
389+
]
390+
)
391+
"""
392+
393+
let observability = ObservabilitySystem.makeForTesting()
394+
XCTAssertThrowsError(try loadManifest(content, observabilityScope: observability.topScope), "expected error")
395+
testDiagnostics(observability.diagnostics) { result in
396+
result.check(diagnostic: "invalid URL ' ' for binary target 'Foo'", severity: .error)
397+
}
398+
}
399+
400+
do {
401+
let content = """
402+
import PackageDescription
403+
let package = Package(
404+
name: "Foo",
405+
products: [
406+
.library(name: "Foo", targets: ["Foo"]),
407+
],
408+
targets: [
409+
.binaryTarget(
410+
name: "Foo",
411+
path: "/tmp/foo/bar")
412+
]
413+
)
414+
"""
415+
416+
let observability = ObservabilitySystem.makeForTesting()
417+
XCTAssertThrowsError(try loadManifest(content, observabilityScope: observability.topScope), "expected error")
418+
testDiagnostics(observability.diagnostics) { result in
419+
result.check(diagnostic: "invalid local path '/tmp/foo/bar' for binary target 'Foo', path expected to be relative to package root.", severity: .error)
420+
}
421+
}
351422
}
352423

353424
func testConditionalTargetDependencies() throws {

‎Tests/WorkspaceTests/WorkspaceTests.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9333,13 +9333,13 @@ final class WorkspaceTests: XCTestCase {
93339333
delegate: MockWorkspaceDelegate()
93349334
)
93359335

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

0 commit comments

Comments
 (0)