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

CCurl available only for Linux or macOS #304

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

Conversation

makleso6
Copy link

@makleso6 makleso6 commented Jul 25, 2019

CCurl available only for Linux or macOS. I Use Kitura on iOS platform, so I move CCurl in other scope

Description

Create flag KITURA_IOS
for Linux or macOS ClientRequest available(no need use CCurl on iOS )
for Linux or macOS HTTP methods available(no need use CCurl on iOS )

Motivation and Context

Use Kitura WebServer on iOS platform without magic

How Has This Been Tested?

I think tests no needed

Checklist:

  • I have submitted a CLA form
  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.

makleso6 and others added 3 commits July 25, 2019 15:12
@djones6 djones6 self-requested a review September 9, 2019 09:42
Copy link
Contributor

@djones6 djones6 left a comment

Choose a reason for hiding this comment

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

@makleso6 Apologies for the delay in reviewing your PR. Other than my suggestion on the #if syntax this change looks fine to me.

We'll want to document the KITURA_IOS flag somewhere - could you add a mention of it to the README, perhaps a new heading called 'Optional compilation flags'?

@@ -14,10 +14,12 @@
* limitations under the License.
*/



#if os(Linux) || os(macOS)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd prefer us to test on the KITURA_IOS flag as it's then consistent with the conditions for the CCurl dependency in Package.swift, eg:

#if !KITURA_IOS

@@ -92,7 +92,7 @@ public class HTTP {
public static func createServer() -> HTTPServer {
return HTTPServer()
}

#if os(Linux) || os(macOS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above

@makleso6
Copy link
Author

makleso6 commented Sep 9, 2019

@djones6 I'll try it

@makleso6
Copy link
Author

makleso6 commented Sep 9, 2019

@djones6 D you think that the removal ClientRequest in !KITURA_IOS good practice?
Or I need to write ClientRequest iOS class over URLSession.
because tests on iOS does not work :(

@djones6
Copy link
Contributor

djones6 commented Sep 9, 2019

The Kitura and Kitura-net tests are heavily dependent on ClientRequest so yes, if you wanted to be able to run the tests on iOS using Kitura-net then we'd need a replacement.

You could potentially look at using Kitura-NIO instead: Kitura-NIO uses Swift-NIO and doesn't depend on Curl. To use it, set KITURA_NIO when you compile.

@CLAassistant
Copy link

CLAassistant commented Sep 27, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ makleso6
❌ djones6
You have signed the CLA already but the status is still pending? Let us recheck it.

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