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

Make NIOTS a conditional target dependency #544

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

Conversation

karwa
Copy link
Contributor

@karwa karwa commented Jan 21, 2022

Also remove an unneeded NIOFoundationCompat target dependency (it's only used by tests, not the AHC library itself).

NIOTS also has an unconditional target dependency on NIOFoundationCompat. In the WebURL port, I found that these were still pulling in Foundation, even though nothing actually used Foundation.

Since conditional target dependencies are a Swift 5.3 feature, this requires adding a 5.3 variant of Package.swift.

@fabianfett
Copy link
Member

fabianfett commented Jan 21, 2022

I assume the underlying bug is this: https://bugs.swift.org/browse/SR-15358

NIOFoundationCompat is still linked (as it is referenced in Package.swift), even though there is no import NIOFoundationCompat that the compiler should be able to see (as all the imports are protected with #if canImport(Network)).

I wonder if the better place to fix this would be in NIOTS directly? @Lukasa, WDYT?

@karwa
Copy link
Contributor Author

karwa commented Jan 21, 2022

as all the imports are protected with #if canImport(Network)

Actually, they are not all protected currently, but this patch adds those conditions. The main HTTPClient.swift has a plain import, as does NWError.swift (even though it doesn't actually use anything from NIOTS - it only uses types from Network).

@Lukasa
Copy link
Collaborator

Lukasa commented Jan 21, 2022

Yeah, I'm inclined to fix NIOTS so that it doesn't bring in Foundation on non-Darwin platforms, and then only merge the parts of this that conditionalise the dependency.

I've never felt good about the @ Package.swifts, I really don't like how they duplicate the package configuration and so if we can possibly avoid using them I'd prefer to.

@fabianfett
Copy link
Member

I've never felt good about the @ Package.swifts, I really don't like how they duplicate the package configuration and so if we can possibly avoid using them I'd prefer to.

If we want to add NIOFoundationCompat conditionally to the NIO TS dependencies, we will need to have two Package.swifts there, as Swift 5.2 does not support conditions. Is that okay for you @Lukasa?

@Lukasa
Copy link
Collaborator

Lukasa commented Jan 24, 2022

I'm a bit happier with it over there as that module changes less than this one.

@karwa
Copy link
Contributor Author

karwa commented Jan 24, 2022

I agree that having dual Package.swift files is unfortunate. I had to do it recently with another project in order to support DocC. You never feel good about it.

That said, NIO is planning to drop Swift 5.2 once 5.6 launches, meaning this package must do the same, and 5.6 has already branched so hopefully it won't be too long.

@Lukasa
Copy link
Collaborator

Lukasa commented Jan 24, 2022

I suppose that raises another option, which is that we could just wait until we do that drop and then make the change in NIOTS once and for all.

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.

3 participants