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

Allow users to switch to using the NIO port of KituraNet #1324

Merged
merged 2 commits into from Aug 30, 2018

Conversation

pushkarnk
Copy link
Contributor

@pushkarnk pushkarnk commented Aug 19, 2018

We recently renamed the erstwhile KituraNIO package to KituraNet. So, we now have a KituraNet and a SwiftNIO-based KituraNet. This pull request allows Kitura users to switch to using the latter using only an environment variable named KITURA_NIO.

Description

The core of the change is only to the Swift package manifest file. In the building phase of the Kitura application, if an environment variable named KITURA_NIO is set, the Swift package manager switches to using the NIO-based KituraNet found here. The other changes in this pull request are in the Tests and mostly around two limitations of the NIO-based port - a bind issue and the absence of a FastCGI implementation.

To enable the use of the SwiftNIO-based KituraNet we use:
export KITURA_NIO=1

To switch back to using stock KituraNet in the same environment:
unset KITURA_NIO

Motivation and Context

Support for a NIO-based networking package was available until now only on a branch named kitura-nio. This pull request makes it available on the master branch.

@pushkarnk pushkarnk changed the title Allowing users to switch to using the NIO port of KituraNet Allow users to switch to using the NIO port of KituraNet Aug 19, 2018
@pushkarnk
Copy link
Contributor Author

I added CI runs with KITURA_NIO=1 on trusty, xenial(docker) and osx to the travis.yml. And we've hit the linker error.

@pushkarnk
Copy link
Contributor Author

pushkarnk commented Aug 20, 2018

I've removed CI builds on Ubuntu 16.04 while we try to address the linker error which has started to look like an Ubuntu thing.

env: SWIFT_SNAPSHOT=4.2 KITURA_NIO=1

before_install:
- if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then brew update && brew install libressl ; fi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is going to be required for all Kitura packages on macOS? If so, perhaps it would be better added to Package-Builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do

Package.swift Outdated
var kituraNetPackage: Package.Dependency

if let _ = ProcessInfo.processInfo.environment["KITURA_NIO"] {
kituraNetPackage = .package(url: "https://github.com/IBM-Swift/Kitura-NIO.git", from: "1.0.0")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is a case for tagging Kitura-NIO to 2.0.0, just to keep things closer in sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I think that's a good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any comments on that @djones6 ?

Package.swift Outdated

var kituraNetPackage: Package.Dependency

if let _ = ProcessInfo.processInfo.environment["KITURA_NIO"] {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer if ProcessInfo.processInfo.environment["KITURA_NIO"] != nil like you did in the tests.

@pushkarnk pushkarnk force-pushed the dual-switch-2 branch 4 times, most recently from 01544d9 to bc8952a Compare August 29, 2018 18:43
@ianpartridge ianpartridge merged commit ea3fc3f into Kitura:master Aug 30, 2018
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.

None yet

2 participants