Semantic Versioning: Should removing a dependency be a semver major?

Development Package Manager
,
1

The SwiftNIO project is engaged in a substantial refactor of our internal codebase to make it easier to build applications that use SwiftNIO but that do not require our POSIX-based event loops (such as iOS applications).

As part of this work we stumbled over an interesting issue. In our most recent step we changed our Package.swift so that two of our products, NIOHTTP1 and NIOWebSocket no longer import NIO or depend on it. This triggered a breakage in swift-aws-lambda-runtime.

The cause of this breakage was a mismatch between the Package.swift file and the actual imports of the module. The AWSLambdaRuntimeCore target was defined like this in Package.swift:

.target(name: "AWSLambdaRuntimeCore", dependencies: [
    .product(name: "Logging", package: "swift-log"),
    .product(name: "Backtrace", package: "swift-backtrace"),
    .product(name: "NIOHTTP1", package: "swift-nio"),
    .product(name: "_NIOConcurrency", package: "swift-nio"),
]),

However, in a couple of the files the code also included an import NIO. Previously, when NIO was a dependency of NIOHTTP1 and _NIOConcurrency, this import worked. However, when we removed the NIO dependency this import started to fail.

My question for the community is: how should we consider the semantic versioning impact of this? For my part, I see two major arguments.

The first is that this is a straightforward SemVer Major. Code that previously built, now does not. This change risks breaking adopter code and so we should delay it until a 3.0 release.

The second argument is that the adopter code was not correct, and could have been broken at any time. Most notably, it's easy to imagine a world where the Swift package manager changed its compilation model such that it broke this import. More broadly, it is also likely to be surprising to most framework developers that removing a dependency may trigger a SemVer major, but adding one won't.

I'd like to get the opinion of the community here: what do we, as a community, believe is acceptable?

8 Likes
2

I go with should not require major semver + major highlight in release notes.

if I'm rebuilding my code I expect stuff like this. Think of going from Swift 5.1 to 5.2 when suddenly every use of public generated a warning and you had to make hundreds of trivial one line changes throughout your code.

3

To be honest, to me, is a pretty straightforward big breaking change. In a similar way as removing API, this removed access to some code that was previously there, thereby breaking adopters. It's a bit weird if this were to happen in a minor release, without even some deprecation cycle or large warning signs for a long while etc.

I'm not sure about the the "SwiftPM could suddenly change how it works" -- at least this would happen in a bigger Swift release and would check off my "at least it had huge warning signs that this was about to happen".

So... yeah it IMHO absolutely is breaking; but perhaps if we did some more work so it had a nicer deprecation cycle we could be a bit more lenient about it?

I was trying to think of an example of a dependency removal from Akka... but we never did such a thing since the dependencies were so minimal :thinking: We did drop entire modules, but only after two (a few year long) minor version cycles though... Again, checking off my "at least there was loud warnings about it" thing.

2 Likes
4

IMHO modules should not expose their dependencies by default and if they do explicitly then this would be a breaking change.

3 Likes
5

Unfortunately this is at odds with how Swift behaves today. The only way to hide your dependencies is to use @_implementationOnly, which is underscored and non-supported at the moment.

4 Likes
6

Indeed, I was suggesting that advances in swift import system might make this question irrelevant.
This issue occurred quite frequently in an app while splitting it into multiple module. I now always declare explicitly in Package.swift any dependency I import.

7

I'm in agreement with Konrad here. Removing the dependency is removing APIs available so it is a breaking change.

I'm sure this will break quite a number of projects/libraries and using the excuse they aren't using SwiftPM correctly isn't helpful. Not requiring a dependency to be listed in Package.swift if another package already dependent on it is listed could be considered a "feature" of SwiftPM. Removing that "feature" would require a major release of Swift.

In a bid to be less black and white about this, If it was possible to deprecate the lack of dependency and have that run over a number of minor releases that could be acceptable. I have no idea how you do that and provide any form of warning during compilation though.

8

Agree that as things stand today, where SPM treats transitive imports as part of the public (yet implicit) API, it should be considered a major breaking change. Any changes to make transitive imports restricted would go through an evolution proposal which would consider a migration story, and only then this expectation might change.

I'm all in on explicit dependencies (grew up in build systems land with Bazel :p ) but if SPM allows this today, it is considered part of its contract and we can't qualify a usage as incorrect if we don't even provide a warning for it.

2 Likes
9

It is not a breaking change, as it hasn’t broken any defined behavior.

To put it another way, the dependencies were an implementation detail, and reliance on that is ill-advised. This is explicitly stated in SemVer 2.0.0.

Personally, I think dependencies in Swift should be hidden by default, and each file that uses a declaration should import it. That makes it obvious where dependencies are being used, and eliminates the risk of breaking something by removing a dependency elsewhere. Alternatively, there could be a compile-time warning if you don’t import something you directly use in the same file.

This extends to Swift Package Manager, too: every dependency directly used in a target should be explicitly specified.

3 Likes
10

As for the current state of Swift Package Manager, consider this: if you updated a dependency to use the next major version, that could come with breaking changes that break other packages relying on that transient dependency. Ergo, if you want to make dependency removal a breaking change, you should also make dependency updating a breaking change.

11

In my opinion this is not breaking. Nobody should rely on transitive dependencies in this way. If a project directly depends on another project, it should be explicitly added. I think this is a mistake in the way SwiftPM exposes these dependencies and should be fixed there.

We have the same problem in C/C++, where headers will be transitively included, which is nice if you want to create umbrella headers, which explicitly include a number of other headers, but should not be relied upon otherwise, because there is no guarantee that the same includes will be added on every system and/or version of the header.

3 Likes
12

Is this actually considered part of SwiftPM's contract? I don’t believe this is actually defined behavior.

As for fixing this commonly-exploited bug in SwiftPM: SemVer says it would technically be a patch version, but should probably be a major one.

13

I also think this is not a breaking change, and if I recall correctly spm going to require explicit dependencies for every module required (though I wasn’t able to find the pr).

1 Like
14

This will be a breaking change, because semantics of dependencies will change. In terms of semantics as they are today, removing a dependency is a breaking change (but agree that it should not be breaking).

In terms of the original question, I'd side with the UX for the current semantics. If people do a package version update, using semver constraints, I'd expect my code to never break if I don't change those contraints. You can argue that the dependent was not correct, but was it their fault that the system allowed them to implement it incorrectly?

15

You already broke those constraints by using an implicit dependency. In short, you are responsible for explicitly declaring anything you import into your code, or otherwise for the maintenance that may come from not doing so. So I agree with with @Saklad5 that this isn't something that should be supported.

I do want to clarify though, that the issue is that consumers could use import NIO without actually declaring it as a dependency, correct? Not that its symbols were available implicitly?

16

Correct: we have arranged to persist all of its symbols appropriately. Only code that did an import NIO without declaring the dependency breaks.

17

Removal of a dependency is a patch‐level change (unless it was @_exported). Every way in which the compiler or SwiftPM has leaked APIs from transitive dependencies has been a bug.

That is invalid. If a client does this and suffers breakage, the fault is his own for not properly declaring his own dependencies and relying on a bug.

If there were no import and it was just a leaked extension or operator, then he would have a reasonable complaint that the compiler set him up for failure. But his complaint would be properly directed at the compiler, not at the package he was using.

It was always the intended model. The fact that some tools’† arrangement of products in the file system enabled undeclared imports to be found by the compiler is an unfortunate accident.

For SwiftPM, this leak is about to be sealed. Pull request #3562 begins verifying that import statements are valid. It already passes the main tests and has been accepted in principle by the SwiftPM team. The only remaining delay is getting the fixes to the source compatibility suite merged into the continuous integration set‐up.

†SwiftPM and Xcode are not the only build systems that operate on Swift packages.

6 Likes
18

I see, that’s important info for what we decide here.

It gets me to lean on the side of respecting this future mode (which I thought was only theoretically mentioned in you first post), and therefore not considering the dependency that was not explicitly stated as break seems more of a thing hm hm…

1 Like
19

Thanks all for your valuable contributions to this discussion over the weekend. I can't speak for others but it's been extremely helpful for me to help refine my thinking.

My tentative conclusion is this: while this behaviour is believed by the SwiftPM team to be undesirable, and will begin to warn in a future SwiftPM version, the SwiftPM team is clearly nervous about the compatibility impact of simply forbidding this "transient dependency" behaviour. I would be as well! In the absence of the SwiftPM diagnostic, it can be somewhat frustrating to debug this kind of issue. Additionally, projects that build today without warnings on all extant Swift versions, and that have not violated NIO's explicit API contract, have some reason to assume that they will continue to build in the face of this kind of update. Finally, fixing the issue is straightforward for us, and while it does have some unfortunate consequences they can be lived with for now.

To that end I have put together a patch that re-adds the dependency to the library products only that NIO provides. The dependency isn't used by the products in question: it's present only to avoid breakage in downstream projects that suffer from this issue. I also have produced a tracking issue to cover actually performing this removal. For now it's tagged for NIO 3.0 as though it were a SemVer major, but it seems to be that we could consider doing it sooner if and when we have widespread adoption of Swift versions that emit warnings or errors in this kind of cofiguration.

10 Likes
20

Late to the party but my 2C - if you are exporting other packages/modules then they obviously become part of your public API. Otherwise changing dependencies should not be a major breaking change.

I completely get the arguments why it could be, but in my view if you want to import a dependency you should add it as a dependency to your package (and I'm glad to se this hole be patched). Using the above argument, then updating a major version of your dependency would also be a breaking change.

1 Like