Comments/cleanup to Package.swift.

[MrMage]

Added some comments and deleted two implicit dependencies; let's see if it still compiles without these. Otherwise, I'll add them back.

[rebello95]

Looks like it doesn't compile without the implicit dependencies. Even if it did, I think they should be kept for purposes of version pinning

[MrMage]

I agree in terms of pinning, but I think we should pin our direct dependencies (NIO and NIOHTTP2), but not the dependencies' dependencies — those should already be pinned implicitly by pinning the direct dependencies. WDYT? In any case, tomorrow I'll add back in whatever is needed to get this to compile.

…

On 20. Feb 2019, at 18:19, Michael Rebello ***@***.***> wrote: Looks like it doesn't compile without the implicit dependencies. Even if it did, I think they should be kept for purposes of version pinning — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[rebello95]

those should already be pinned implicitly by pinning the direct dependencies

This is only true as long as the direct dependencies are actually pinning (instead of using master or a less specific tag). As long as they're doing this, it's probably fine, but we should definitely validate.

[MrMage]

Re-added the transitive dependencies with the same pinning as before; PTAL.

[kevints]

I noticed that Package.resolved isn't checked in, any particular reason for that? My understanding is that for dependencies following SemVer (like swift-nio) we should generally be using from: earliestVersionWeNeed so that apps aren't gated by us if (for example) upstream releases a security patch. Checking in Package.resolved should keep our builds reproducible but it doesn't propagate downstream.

[MrMage]

I noticed that Package.resolved isn't checked in, any particular reason for that?

I'm not sure this is good practice; at least the NIO projects don't seem to do it.

My personal stance is to be as loose as possible (i.e. use upToNextMinor/Major) but not pin more than that, hoping that our dependencies adhere to SemVer and don't break us. (At that point, we could still get more specific with our pinning in case dependencies start breaking us.)

I think @rebello95 prefers to be more specific with regards to pinning, though.

[kevints]

Checking in Package.resolved combined with some automation that runs swift package update, runs the tests, then commits the results would keep our builds green (and reproducible) and notify us if an upstream dependency change that purported to be compatible broke us without gating upstream security updates behind us.

Chatted to @Lukasa about why swift-nio doesn't use Package.resolved and he said they're a bad example to follow since they don't have many upstream dependencies (apart from themselves). But note that they usually don't pin either.

[MrMage]

Checking in Package.resolved combined with some automation that runs swift package update, runs the tests, then commits the results would keep our builds green (and reproducible) and notify us if an upstream dependency change that purported to be compatible broke us without gating upstream security updates behind us.

That does sound like it would combine the best of both worlds; would you (or someone at Apple) have spare cycles to set up the necessary automation? I'm not an expert on GitHub hooks/scripts/automation (or whatever we'd need for this).

[kevints]

Using upToNextMinor: instead of from: can easily result in "dependency hell" as well, since other libraries (for example, nio-based) could require a newer minor version of swift-nio that is still compatible with grpc-swift while still being artificially restricted.

Hypothetically:

swift-nio releases version 2.1.0
grpc-swift requires swift-nio .upToNextMinor(from "2.0.0")
example-http-client requires swift-nio from: "2.1.0"

This would result in a dependency resolution error even though swift-nio hasn't broken backwards compatibility, and it would show up in apps that were trying to use both grpc and example-http-client, probably leading to frustration with whichever package they tried to adopt second.

[MrMage]

Agreed; I think using `upToNextMajor:` combined with an automatism that notifies us when dependencies break us would be optimal.

…

On 21. Feb 2019, at 18:45, Kevin Sweeney ***@***.***> wrote: Using upToNextMinor: instead of from: can easily result in "dependency hell" as well, since other libraries (for example, nio-based) could require a newer minor version of swift-nio that is still compatible with grpc-swift while still being artificially restricted. Hypothetically: swift-nio releases version 2.1.0 grpc-swift requires swift-nio .upToNextMinor(from "2.0.0") example-http-client requires swift-nio from: "2.1.0" This would result in a dependency resolution error even though swift-nio hasn't broken backwards compatibility, and it would show up in apps that were trying to use both grpc and example-http-client, probably leading to frustration with whichever package they tried to adopt second. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[kevints]

Checking in Package.resolved combined with some automation that runs swift package update, runs the tests, then commits the results would keep our builds green (and reproducible) and notify us if an upstream dependency change that purported to be compatible broke us without gating upstream security updates behind us.

That does sound like it would combine the best of both worlds; would you (or someone at Apple) have spare cycles to set up the necessary automation? I'm not an expert on GitHub hooks/scripts/automation (or whatever we'd need for this).

For a first pass we could do this semi-manually and commit the output (just like we currently do when code-generator output changes). So someone "periodically" runs swift package update, commits the Package.resolved, then opens a PR with that change. The immediate advantage of that approach is that each sha of this repo builds reproducibly even if upstream later releases a new incompatible version.

[kevints]

By the way, this is what a hypothetical error message will look like when swift-nio releases 1.13.0:

$ swift package resolve swift-nio --version 1.13.0
Updating https://github.com/kylef/Spectre.git
Updating https://github.com/apple/swift-protobuf.git
Updating https://github.com/kylef/Commander.git
Updating https://github.com/apple/swift-nio-http2.git
Updating https://github.com/apple/swift-nio-zlib-support.git
Updating https://github.com/apple/swift-nio.git
Updating https://github.com/apple/swift-nio-nghttp2-support.git
error: dependency graph is unresolvable; found these conflicting requirements:

Dependencies:
    https://github.com/apple/swift-protobuf.git @ 1.3.1..<1.4.0
    https://github.com/kylef/Commander.git @ 0.8.0..<0.9.0
    https://github.com/apple/swift-nio-zlib-support.git @ 1.0.0..<1.1.0
    https://github.com/apple/swift-nio.git @ 1.12.0..<1.13.0
    https://github.com/apple/swift-nio-nghttp2-support.git @ 1.0.0..<1.1.0
    https://github.com/apple/swift-nio-http2.git @ 0.2.1..<0.3.0
    https://github.com/apple/swift-nio.git @ 1.13.0

[rebello95]

Little bit late, but I definitely agree with the sentiment above regarding checking in the lockfile and allowing CI to upgrade compatible dependencies 😄

[MrMage]

I've added Package.resolved and switched our dependencies to use .upToNextMajor now. Let's see what Travis thinks about this.

[MrMage]

@rebello95 @kevints, could you please take another look at this?

[rebello95]

Thinking about this a bit more, doesn't this still mean that end consumers of SwiftGRPC will now need to manually pin dependencies on their end even if we don't need to for our CI? Reading through SwiftPM's documentation on Package.resolved, it actually mentions this case:

for a package which is a dependency of other packages (e.g. a library package), that package's Package.resolved file will not have any effect on its client packages.

If I'm understanding correctly, it sounds like we should check in .exact versions in Package.swift, then essentially have CI change them to upToNextMajor, attempt to run swift package update, and update both files accordingly if the resolution had a successful diff. Does this make sense?

[MrMage]

Thinking about this a bit more, doesn't this still mean that end consumers of SwiftGRPC will now need to manually pin dependencies on their end even if we don't need to for our CI? Reading through SwiftPM's documentation on Package.resolved, it actually mentions this case:

for a package which is a dependency of other packages (e.g. a library package), that package's Package.resolved file will not have any effect on its client packages.

If I'm understanding correctly, it sounds like we should check in .exact versions in Package.swift, then essentially have CI change them to upToNextMajor, attempt to run swift package update, and update both files accordingly if the resolution had a successful diff. Does this make sense?

Pinning all our dependencies exactly would put our users in a world of hurt, because it will frequently cause unresolvable dependency graphs (see
#371 (comment) above).

To summarize the possible options:

1. Locking to exact versions is as defensive as possible, essentially always assuming that our dependencies will break us with the very next update. @rebello95, I think this is what you are suggesting.
2. The approach by @kevints is much more optimistic, assuming that using the latest versions (except for new major versions) of all dependencies will usually not break us. We would still get notified by the CI if they do break us, at which point we can go fix the newly-introduced errors or temporarily pin the dependencies until we can resolve the problem.

IIRC we have never been broken by updates SwiftProtobuf since that package reached 1.0, and we are already keeping a close eye on how SwiftNIO and SwiftNIOHTTP2 develop. Therefore, I favor approach 2. The one exception to this could be exactly pinning SwiftNIOHTTP2, as that framework is still very much in flux and is undergoing significant rewrites at the moment. In addition, it is unlikely that our users themselves depend on that package, so unresolvable dependency graphs should be unlikely there.

[kevints]

Thinking about this a bit more, doesn't this still mean that end consumers of SwiftGRPC will now need to manually pin dependencies on their end even if we don't need to for our CI? Reading through SwiftPM's documentation on Package.resolved, it actually mentions this case:

for a package which is a dependency of other packages (e.g. a library package), that package's Package.resolved file will not have any effect on its client packages.

If I'm understanding correctly, it sounds like we should check in .exact versions in Package.swift, then essentially have CI change them to upToNextMajor, attempt to run swift package update, and update both files accordingly if the resolution had a successful diff. Does this make sense?

@rebello95 Yes, end consumers depending on swift-grpc also need to check in their Package.resolved, and run swift package update periodically to keep their builds reproducible. It's completely possible that they will be pinned to different versions of libraries than grpc-swift CI is (for example if they need to quickly adopt a swift-nio security update faster than grpc-swift can). But if they're following the Package.resolved model, the only time a dependency can break them is when they run swift package update.

Pinning for example swift-nio to .exact in grpc-swift gates any security or minor API version updates behind grpc-swift, even though swift-nio follows SemVer. See my example above for an example of how this could prevent you from updating to an otherwise completely compatible swift-nio. I call out swift-nio here specifically because it's likely to be a dependency for several libraries in an end consumer's app or service and it is generally impossible for every one of them to update at once. grpc-swift might be a great citizen here and do a new release 10 minutes after every swift-nio release, but for over-pinning to succeed every ecosystem library will need to do that in lock step.

There are other great reliability properties of Package.resolved as well. A major one is that the shas of your dependencies are all pinned, and if you already have a checkout your build doesn't need network access at all. So you can make changes to your project and do swift build and swift test without any connection to the Internet (on an airplane or a locked down intranet CI server, for example).

[kevints]

@rebello95 I realized that much of what I wrote is already addressed in the swift-nio project's FAQs on its public API. One note - the .upToNextMajor(from: "...") form of version constraint used in the PR is an equivalent, longer spelling of the from: "..." form of the constraint used in the SwiftNIO FAQ; they mean the same thing.

@weissi has a few Tweets on this topic and @MrMage has already registered SwiftGRPC for the NIO compatibility suite here, so the breakage risk should be minimal.

[rebello95]

@kevints and @MrMage thanks for discussing this! I'm okay with this approach assuming that downstream consumers will check in their Package.resolved and pin these transitive dependencies as they see fit in order to allow consumers to use somewhat varying versions of dependencies. Left one comment about a non-1.x dependency, feel free to merge after.