Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't depend on NIOFoundationCompat in NIOTransportServices on Linux #211

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Cyberbeni
Copy link
Contributor

Don't depend on NIOFoundationCompat in NIOTransportServices on Linux.
This is a breaking change, see: #210

Motivation:

I'm trying to build a small utility tool based on mqtt-nio which depends on NIOTransportServices. I noticed that NIOTransportServices depends on NIOFoundationCompat on all platforms but only imports it where the Network framework is available. Removing this dependency on non-Apple platforms allows us to not import Foundations, significantly reducing the size of the build product when using the Swift Static Linux SDK.

Modifications:

This PR and a similar one to mqtt-nio.

Result:

Hello world docker image depending on mqtt-nio went from 169MB to 85MB.

@Lukasa
Copy link
Contributor

Lukasa commented Oct 14, 2024

Thanks for re-filing this! ✨ I know it's a pain that we had to back it out.

Unfortunately, this is a semver major in Swift. That means we need to think fairly hard about whether we want to issue a 2.0 for this change. My instinct is that we probably do: having the Foundation dependency leak through unnecessarily is fairly painful. It's also a very easy semver major for people to adopt, as it doesn't actually change the API at all so the range can be offered.

@glbrntt has a few other places he wants to make tweaks here as well.

cc @0xTim @adam-fowler @Joannis @FranzBusch @weissi as other interested parties.

@weissi
Copy link
Member

weissi commented Oct 14, 2024

Thanks for re-filing this! ✨ I know it's a pain that we had to back it out.

Unfortunately, this is a semver major in Swift. That means we need to think fairly hard about whether we want to issue a 2.0 for this change. My instinct is that we probably do: having the Foundation dependency leak through unnecessarily is fairly painful. It's also a very easy semver major for people to adopt, as it doesn't actually change the API at all so the range can be offered.

@glbrntt has a few other places he wants to make tweaks here as well.

cc @0xTim @adam-fowler @Joannis @FranzBusch @weissi as other interested parties.

Yes, for NIOTS I think a new SemVer makes sense

@0xTim
Copy link

0xTim commented Oct 14, 2024

Is this a SemVer major change? The public API hasn't changed (assuming the types aren't exported) and the the package platform requirements haven't changed (just one for the dependency which should be fine to change right?)

@adam-fowler
Copy link
Contributor

Is this a SemVer major change? The public API hasn't changed (assuming the types aren't exported) and the the package platform requirements haven't changed (just one for the dependency which should be fine to change right?)

It broke Hummingbird because we were careless about ensuring we added NIOFoundationCompat to the target dependencies.

@0xTim
Copy link

0xTim commented Oct 15, 2024

Is this a SemVer major change? The public API hasn't changed (assuming the types aren't exported) and the the package platform requirements haven't changed (just one for the dependency which should be fine to change right?)

It broke Hummingbird because we were careless about ensuring we added NIOFoundationCompat to the target dependencies.

Ehhh I would say that's more a SwiftPM bug/Hummingbird bug than a SemVer issue with this library (not a criticism to be clear) otherwise you end up going down a very slippery path of not being able to change dependencies in your libraries that should only be an implementation detail. This is also going to cause problems further down the line when Swift 6 is the lowest supported version (so whenever Swift 6.2 comes out) and Swift 6 language mode is adopted as due to SE-0409 all transitive dependencies are internal by default. Are we expecting that going migrating to Swift 6 language mode will be a breaking change for all libraries? Because that's going to open a can of worms

@Lukasa
Copy link
Contributor

Lukasa commented Oct 15, 2024

Ehhh I would say that's more a SwiftPM bug/Hummingbird bug than a SemVer issue with this library (not a criticism to be clear) otherwise you end up going down a very slippery path of not being able to change dependencies in your libraries that should only be an implementation detail.

I'd love for that to be true, but ultimately we're reading semver as basically meaning "if you make a change that causes a previously compiling program to stop compiling, that's a major". That's in line with the rest of NIO's positions on API changes.

The reality for us is that right now, we can't change those dependencies: Swift as a language and SwiftPM as a build system prevents them. When those deficiencies are fixed, we have the opportunity to make changes. But otherwise, we're stuck.

Are we expecting that going migrating to Swift 6 language mode will be a breaking change for all libraries?

If they don't annotate their imports with public, yes. However, SE-409 is not actually enabled in Swift 6 so far as I am aware, so right now that's not a real concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants