1

Hello Swift community,

The review of SE-0272 “Package Manager Binary Dependencies" begins now and runs through November 20, 2019.

Reviews are an important part of the Swift evolution process. All reviews should be made in this thread on the Swift forums or, if you would like to keep your feedback private, directly in email to me as the review manager.

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to the Swift Package Manager?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages, libraries, or package managers with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available on the Swift Evolution website.

As always, thank you for participating in Swift Evolution.

Boris Buegling
Review Manager

21 Likes
2

Quick comments (minor)--

Why is checksum a property of Source rather than Artifact?
I think it might be best to avoid the double negative of disallowsBinaryDependencies: false.

2 Likes
3

I think the idea would be that path-based artifacts don't get a checksum, but only the URL based ones.

4

I thought about this as well when writing this but as @NeoNacho said when using path based binary dependencies a checksum should not be required.

5

This doesn’t seem to extend well to the future directions explored; it would be reasonable to require a checksum for any remote source (whether URL-based or not). Further, if fallback sources are allowed in the future, the same artifact from different sources shouldn’t have different checksums.

6

You are right any remote soured binary dependencies should have a checksum; however, I do not see how this is right not the case or how this doesn't extend well for the future directions. If we start to support different artefact stores in the future, we can create new enum cases for them which themselves require checksums again.

To your second point, I don't think the proposal mentions fallback sources but rather sources based on some condition. This would mean that there could be multiple sources defined for a artefact but these are conditioned to certain environments such as a specific platform/ABI/etc.. However, when resolving these dependencies always the first matching source will be picked and a change in the picked source can and should result in a different checksum.

If I misunderstood your points, could you please elaborate a bit more on them :)

7

Furthermore, we propose to add a new artifacts: [Artifacts]? property to the Target

Could we use artifacts: [Artifact] = [] instead, or is there a distinction between no artifacts and not setting artifacts?

1 Like
8

Thanks for this very useful proposal!

Ive got a question, that might be already answered by the design decisions made in this proposal:

During the WWDC we were informed the xcframeworks cant link a Swift Package as its dependency (due to possible ABI/module stability/versioning issues)

With this proposal, is it possible to add binary packages as subdependencies of a target that builds another xcframework?

Thanks

9

Sorry, I must have misunderstood how you are using the terms “artifact” and “source” in this context. Are you envisioning platform conditions leading to different sources or different artifacts? Put another way, are MyLibrary-Linux.zip and MyLibrary-Darwin.zip going to be different sources of one artifact or different artifacts?

If the former, then it makes sense for one artifact to support sources with different checksums. If the latter, then I think the checksum would be more appropriately modeled as a property of Artifact.

Also—a separate point—just because I have a binary dependency stored locally, doesn’t mean that I couldn’t also benefit from at least the option of having the checksum verified.

1 Like
10

I don't see any mention of support for SSH URLs. It would be nice if private artifact stores worked out-of-the-box (or at least had the same capabilities as Package URLs).

1 Like
11

I am not very familiar with private artifact stores, what does "SSH URL" mean here? Are they serving a Git repo? If yes, using the path-based option and putting the manifest plus binary in that repo should work.

12

Sorry, I think I have also misspoken above. Let me explain with an example how I first designed it to work with multiple artefacts and conditions.

let package = Package(
   ...
    targets: [
        .binaryTarget(
            name: "SomePackageLib",
            artifacts: [
                .artifact(
                    source: .url("https://github.com/some/package/releases/download/1.0.0/SomePackageMacOS-1.0.0.zip",
                                 checksum: "839F9F30DC13C30795666DD8F6FB77DD0E097B83D06954073E34FE5154481F7A"),
                    when: (platform: .macOS, hardwareSupportsFloatingNumbers: true)
                ),
                .artifact(
                    source: .url("https://github.com/some/package/releases/download/1.0.0/SomePackageUbuntu-1.0.0.zip",
                                 checksum: "CAF90169EEFA5F807D577486B9F795AB86AE2983C5C20806CFF959117E90AF18"),
                    when: (platform: .linux, hardwareSupportsFloatingNumbers: true)
                ),
            ]
        )
     ]
)

The conditions would of course be something completely different but just to give an example this is how we first envisioned to support different platforms. If you define a path based source here then it would not need a checksum.

I would like to understand your reasons behind what you think a checksum can solve for path based dependencies. For URL based, we want to avoid security issues where the remote is compromised and somebody changes the artefact without us knowing.

13

I don't have a thorough review of this proposal because it largely covers use-cases I have no meaningful experience with. However, I'd like to say that I'm strongly in favour of the decision of this proposal to both exclude non-Apple platforms and to avoid attempting to address distributing binary equivalents for source-available targets. Both of those are important goals worth dealing with, but we should aim to do so in a way that does them justice, rather than shipping something so underspecified that it does not work.

3 Likes
14

+1 overall.

As an implementation detail:

public final class Package {
    ...
    /// This disallows any binary dependency or any transitive binary dependency.
    public var disallowsBinaryDependencies: Bool
    ...
}

Why not just have allowsBinaryDependencies: Bool with a default value of true? Reading true to disable a something is a bit awkward.

Also, what about trust chains for packages? Meaning I may want to allow binary dependencies from packages X, Y, Z (maybe company packages), but not from A, B, C (maybe public repositories)?

Using the playing card deck example, I'd imagine something like this:

let package = Package(
    name: "DeckOfPlayingCards",
    products: [
        .library(name: "DeckOfPlayingCards", targets: ["DeckOfPlayingCards"]),
    ],
    dependencies: [
        .package(url: "https://github.com/apple/example-package-fisheryates.git", from: "2.0.0"),
        .package(url: "https://github.com/apple/example-package-playingcard.git", from: "3.0.0", allowsBinaryPackages: false),
    ],
    targets: [
        .target(
            name: "DeckOfPlayingCards",
            dependencies: ["FisherYates", "PlayingCard"]),
        .testTarget(
            name: "DeckOfPlayingCardsTests",
            dependencies: ["DeckOfPlayingCards"]),
    ]
)

The point of this is to allow the use of something like Firebase from my package without allowing anything else from the other dependencies I'm pulling in. I don't believe this is captured with the opt-in/opt-out Whitelist discussion in the proposal. While it is true that one is extending the same level of trust to both source and binary packages, sources can be audited and pinned to specific versions. I'm also doing it as a consumer of package dependencies vs. the package author.

Again, I think this can come later, but it should probably be mentioned in the proposal as something to consider (or reject outright for a different approach).

3 Likes
15

Two thoughts on this:

  • binaries will also be able to be pinned to specific versions, since one version of the manifest specifies concrete versions of the binary artifacts through checksums. The pinning is even slightly stronger, I don't think we catch deleting and recreating a tag.
  • my take on the opt-in for specific packages is that this is more of a workflow feature for individual tools to figure out than part of the core manifest (mentioned here). If we agree that this is true, it might make sense to mention that in the proposal.
16
  • binaries will also be able to be pinned to specific versions, since one version of the manifest specifies concrete versions of the binary artifacts through checksums. The pinning is even slightly stronger, I don't think we catch deleting and recreating a tag.

Pinning the version is fine, and the Package.resolved already does this. But there's no real correlation between the source @ v1.2.3 and the binary @ v1.2.3, just implied. :slight_smile:

  • my take on the opt-in for specific packages is that this is more of a workflow feature for individual tools to figure out than part of the core manifest (mentioned here). If we agree that this is true, it might make sense to mention that in the proposal.

Hmm... not sure I agree on that part, especially since it's SwiftPM that is now pulling in the binaries. It'd be one thing if the external binary was being pulled in via some external workflow measure and SwiftPM was just providing flag annotations for the compiler.

The only real way I could see that working with the proposal today is a pre-fetch of all of the top-level packages you depend on, adding in the disallowBinaryPackages: true setting those Package.swift files, and have your Package.swift reference the local copies instead.

17

It is trivial enough for an external tool to link against SwiftPM, load the package graph, and validate every target in the dependency graph against any kind of criteria. You could enforce no binaries, no dynamic libaries, only whitelisted URLs, a maximum number of sources, no C targets, etc.

All that can be implemented in only 28 lines (excluding the no binary check which doesn’t exist yet.)

// Package.swift
let package = Package(name: "MyValidator",
  dependencies: [.package(
    url: "https://github.com/apple/swift-package-manager", "0.5.0")]
  targets: [.target(name: "MyValidator", dependencies: ["SwiftPM-auto"])])

// main.swift
import Workspace

let swiftCompiler = AbsolutePath(
  try! Process.checkNonZeroExit(args: "which", "swiftc").spm_chomp())
let packagePath = AbsolutePath("/some/package")
let graph = try! Workspace.loadGraph(packagePath: packagePath,
  swiftCompiler: swiftCompiler, diagnostics: DiagnosticsEngine())
for package in graph.packages {
  for dependency in package.manifest.dependencies
    where !dependency.url.contains("my-company.com") {
      print("Don’t depend on third‐party packages!")
  }
}
for product in graph.reachableProducts
  where product.type == .library(.dynamic) {
    print("Don’t depend on dynamic libraries!")
}
for target in graph.reachableTargets {
  if target.sources.paths.count > 100 {
    print("Don’t depend on large modules!")
  }
  if target.sources.paths.contains(where: { $0.suffix == ".c" }) {
    print("Don’t depend on C targets!")
  }
}

All that to say I agree with @NeoNacho, that it doesn’t need to be SwiftPM’s responsibility.

1 Like
18

I completely agree with this, just like I think an isEnabled flag on something makes more sense for me than an isDisabled flag.

I agree with David here as well. It might not necessarily SPMs "responsibility", but since I'd file this feature under "security related" I would rather see it be a feature of SPM, than expect everyone to write code over and over again for their projects. (Your code sample is pretty neat though!)

19

I think there's a misunderstanding here what I mean by "workflow feature". This wouldn't be provided by libSwiftPM, but IDEs, such as Xcode, could implement it. For example, one could imagine a workflow where when adding a new package dependency through an IDE, there would be an informational prompt about what transitive dependencies that entails and which one of these are binaries.

20

I'd rather not have features pushed to the IDE side, as when doing server development on Linux, there is no Xcode. Sure, some features make sense being shoved downstream, but imho this is not one of them.