Fix package describe support for local zip binary artifacts

[iainsmith]

swift package describe should work for targets that have binary zip dependencies.

Motivation:

#4387. Several projects including swift package index and others want to use the output of swift package describe. This extends changes made in #3810 cc @tomerd

Modifications:

This patch updates, BinaryArtifactsManager.deriveBinaryArtifactKind to return BinaryTarget.Kind.unknown if the path suffix is .zip.

This seemed like the minimal change to handle zip binary artifacts in package describe.

Alternatively it might better to extend BinaryTarget.Kind to be explicitly aware of local zips.

Let me know if there is an alternative way you'd prefer this was implemented.

Result:

Projects should be able to run swift package describe and get correct output.

[neonichu]

I thought we did not support local binary artifacts to be zipped?

[iainsmith]

I thought we did not support local binary artifacts to be zipped?

They are explicitly mentioned with examples in SE-272. Any chance that changed?

For path-based artifact SwiftPM supports artifacts as a .zip and as a raw XCFramework.

Also works in Xcode 14 for what it's worth.

Most teams that I've seen do this are using it with git-lfs.

[tomerd]

iirc supporting local zipped binary archives was added by a contributor at some point

[tomerd]

as for the fix itself, I suggest we modify loadRootPackage which already has some specific logic for the describe use case

specifically

// radar/82263304
// compute binary artifacts for the sake of constructing a project model
// note this does not actually download remote artifacts and as such does not have the artifact's type or path
let binaryArtifacts = try manifest.targets.filter{ $0.type == .binary }.reduce(into: [String: BinaryArtifact]()) { partial, target in
    if let path = target.path {
        let artifactPath = try manifest.path.parentDirectory.appending(RelativePath(validating: path))
        guard let (_, artifactKind) = try BinaryArtifactsManager.deriveBinaryArtifact(fileSystem: self.fileSystem, path: artifactPath, observabilityScope: observabilityScope) else {
            throw StringError("\(artifactPath) does not contain binary artifact")
        }
        partial[target.name] = BinaryArtifact(kind: artifactKind , originURL: .none, path: artifactPath)
    } else if let url = target.url.flatMap(URL.init(string:)) {
        let fakePath = try manifest.path.parentDirectory.appending(components: "remote", "archive").appending(RelativePath(validating: url.lastPathComponent))
        partial[target.name] = BinaryArtifact(kind: .unknown, originURL: url.absoluteString, path: fakePath)
    } else {
        throw InternalError("a binary target should have either a path or a URL and a checksum")
    }
}

and keep deriveBinaryArtifact clean, since the code to return .unknown is strictly for describe functionality and we should try to keep that logic out of the flow if possible

[iainsmith]

Thanks both. I'll take a look at reworking this and push another version.

[elsh]

@iainsmith Have you had a chance to look at reworking it?