Skip to content

Commit 6ba2c49

Browse files
tomerdtomerd
authored
improve error message when using invalid path for local binary artifacts (#4129)
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 645fe16 commit 6ba2c49

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
@@ -2295,7 +2295,7 @@ extension Workspace {
22952295
addedOrUpdatedPackages: [PackageReference],
22962296
observabilityScope: ObservabilityScope
22972297
) throws {
2298-
let manifestArtifacts = try self.parseArtifacts(from: manifests)
2298+
let manifestArtifacts = try self.parseArtifacts(from: manifests, observabilityScope: observabilityScope)
22992299

23002300
var artifactsToRemove: [ManagedArtifact] = []
23012301
var artifactsToAdd: [ManagedArtifact] = []
@@ -2315,7 +2315,7 @@ extension Workspace {
23152315
targetName: artifact.targetName
23162316
]
23172317

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

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

24552455
// 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
@@ -9330,13 +9330,13 @@ final class WorkspaceTests: XCTestCase {
93309330
delegate: MockWorkspaceDelegate()
93319331
)
93329332

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

0 commit comments

Comments
 (0)