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

Rewrite of PR #1103 + addition Android specific patches #1113

Closed
wants to merge 2 commits into from

Conversation

johnno1962
Copy link
Contributor

@johnno1962 johnno1962 commented Jul 11, 2017

Hi, This PR is a rewrite of #1103 after discussion that started just after it was merged. This is a restructuring of the Android specific api to provide the path to a certificates authorities file required for libcurl to work. It also includes a few other patches required for foundation to build for Android.

@johnno1962 johnno1962 changed the title Rewite of PR #1103 after discussion + addition Andoird specific patches Rewrite of PR #1103 after discussion + addition Andoird specific patches Jul 11, 2017
@johnno1962 johnno1962 changed the title Rewrite of PR #1103 after discussion + addition Andoird specific patches Rewrite of PR #1103 + addition Andoird specific patches Jul 11, 2017
@johnno1962 johnno1962 changed the title Rewrite of PR #1103 + addition Andoird specific patches Rewrite of PR #1103 + addition Android specific patches Jul 11, 2017
#else
typealias maxhostlen_t = socklen_t
#endif
if getnameinfo(info.ai_addr, sa_len, host, maxhostlen_t(NI_MAXHOST), nil, 0, flags) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is going on here? The fourth argument should be of type size_t on all platforms, should it not? I'm not aware of any type known as maxhostlen_t. If necessary, you can simply use numericCast(NI_MAXHOST) on all platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, top tip. Didin’t know about numericCast()!

try! CFURLSession_easy_setopt_ptr(rawHandle, CFURLSessionOptionCAINFO, caInfo).asError()
}
}
if URLSession.sslUnsafeBypassPeerVerify {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unsafe should not be part of this name. In Swift, "safe" and "unsafe" refer to memory safety and there is nothing unsafe about this particular property.

Furthermore, as this is a Boolean property, it should be named according to Swift guidelines for Boolean properties (i.e., it should begin with a verb such as "is," "has," or in this case "verify"). Probably best to align with underlying cURL usage and phrase this in the positive, as in: URLSession.verifyPeerSSLCertificate (default: true).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now verifyPeerSSLCertificate.

else if let caInfo = URLSession._sslCertificateAuthorityFile {
try! CFURLSession_easy_setopt_int(rawHandle, CFURLSessionOptionSSL_VERIFYPEER, 1).asError()
try! CFURLSession_easy_setopt_ptr(rawHandle, CFURLSessionOptionCAINFO, caInfo).asError()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Else...? That is, what happens if the user wants verification but has a nil value for _sslCertificateAuthorityFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No bypass + no authority file == SSL not working.

extension URLSession {
/// See https://curl.haxx.se/docs/sslcerts.html
/// For SSL to work you need a "cacert.pem" to be accessible at the
/// by setting URLSession.sslCertificateAuthorityFile to a file URL.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sentence is ungrammatical--"to be accessible at the <what???> by setting"...

/// by setting URLSession.sslCertificateAuthorityFile to a file URL.
/// Downloadable here: https://curl.haxx.se/docs/caextract.html
/// Alternatively you can bypass SSL peer verification altogether:
/// URLSession.sslUnsafeBypassPeerVerify = true (not advised)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These doc comments should be affixed to the corresponding properties. Here, they are documenting a fileprivate static var, which is not useful in autogenerated documentation.

sslUnsafeBypassPeerVerify = false
free(_sslCertificateAuthorityFile)
guard value.isFileURL && FileManager.default.fileExists(atPath: value.path) else {
NSLog("*** sslCertificateAuthorityFile not FileURL or does not exist ***")
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Put single quotes around properties (i.e. 'sslCertificateAuthorityFile').
  • "FileURL" is neither a real type name nor an English word.

Reword:

'sslCertificateAuthorityFile' does not represent a file that exists

/// Downloadable here: https://curl.haxx.se/docs/caextract.html
/// Alternatively you can bypass SSL peer verification altogether:
/// URLSession.sslUnsafeBypassPeerVerify = true (not advised)
fileprivate static var _sslCertificateAuthorityFile: UnsafeMutablePointer<Int8>?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be _sslCertificateAuthorityFilePathPtr or something like that? It's not simply the backing variable for sslCertificateAuthorityFile.

return URL(fileURLWithPath: String(cString: value))
}
else {
return URL(fileURLWithPath: "NOVALUE")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh? There is a standard way to indicate the absence of something in Swift, and that's nil. This computed property should be of type URL?, and here the return value should be nil.

sslUnsafeBypassPeerVerify = false
free(_sslCertificateAuthorityFile)
guard value.isFileURL && FileManager.default.fileExists(atPath: value.path) else {
NSLog("*** sslCertificateAuthorityFile not FileURL or does not exist ***")
Copy link
Member

Choose a reason for hiding this comment

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

this should probably be fatal

@@ -138,7 +140,12 @@ open class Host: NSObject {
}
let sa_len: socklen_t = socklen_t((family == AF_INET6) ? MemoryLayout<sockaddr_in6>.size : MemoryLayout<sockaddr_in>.size)
let lookupInfo = { (content: inout [String], flags: Int32) in
if getnameinfo(info.ai_addr, sa_len, host, socklen_t(NI_MAXHOST), nil, 0, flags) == 0 {
#if os(Android)
Copy link
Member

Choose a reason for hiding this comment

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

our other guards in Foundation are outdented to the first column

return URL(fileURLWithPath: String(cString: value))
}
else {
return URL(fileURLWithPath: "NOVALUE")
Copy link
Member

Choose a reason for hiding this comment

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

again also should be fatal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, better than a nil I figure.

@johnno1962
Copy link
Contributor Author

Ready to roll?

else if let caInfo = URLSession._sslCertificateAuthorityFilePathPtr {
try! CFURLSession_easy_setopt_int(rawHandle, CFURLSessionOptionSSL_VERIFYPEER, 1).asError()
try! CFURLSession_easy_setopt_ptr(rawHandle, CFURLSessionOptionCAINFO, caInfo).asError()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, what happens if neither of these cases is true? It should be a fatal error, probably.

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 11, 2017

Choose a reason for hiding this comment

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

It may work one day if libcurl starts to support Android for SSL. Also, At this point we don’t know if the user is making an SSL request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see.

/// Downloadable here: https://curl.haxx.se/docs/caextract.html
public static var sslCertificateAuthorityFile: URL {
set(value) {
verifyPeerSSLCertificate = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Setting sslCertificateAuthorityFile should not automatically flip the switch on the second, independent setting.

If the user bothered to turn off certificate verification, then it should stay off until it's explicitly flipped on again. Nothing about setting a file path for CA information implies that you'd automatically want to verify SSL certificates. That is, after all, the intuition that led you to separate these two things into two properties instead of one.

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 11, 2017

Choose a reason for hiding this comment

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

I’ll push back on this one. There is no point in setting the CA file if verifyPeerSSLCertificate isn’t true. This is for safety.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then there should not be two distinct properties.

Does cURL automatically discard the one setting when the other is set? If not, then users would have no expectation that it's the case here.

Moreover, your implementation does nothing in particular to provide such "safety" if a user sets this property and then the Boolean. Doing one thing vs. another when two independent properties are set in a different order is difficult to reason about.

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 12, 2017

Choose a reason for hiding this comment

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

Originally there was only one property a string which could be null, a path or a dummy value. In some ways it was better but the code got to be a bit of a mess when we started using URLs. The only alternative would be some kind of magic enum that looked after the ownership of the file path pointer. While it’s interesting to meditate on making it airtight I think we need to draw a guillotine down on this. Its a very niche api. The api is at least simple which is a good start and it’s not so difficult to reason about. Between the two properties the last set takes effect though the coupling is regrettable its unavoidable.

Copy link
Collaborator

@xwu xwu Jul 12, 2017

Choose a reason for hiding this comment

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

Yes, I recall. I'm not arguing for a more complicated API. What I'm saying is, since you've chosen the path of presenting two properties, they should be independent of each other. This is consistent with user expectation both in Foundation and in cURL. Precisely because this is a niche API that will not be amply documented, it's important that these expectations are met. It is very surprising that...

NSSession.verifyPeerSSLCertificate = false
NSSession.sslCertificateAuthorityFile = url

...gives a different result than...

NSSession.sslCertificateAuthorityFile = url
NSSession.verifyPeerSSLCertificate = false

...keeping in mind that the default for verifyPeerSSLCertificate is true, so if a user wants that setting to be false, he or she will necessarily have to set that value somewhere explicitly.

I know of nowhere in the standard library or Foundation where setting one public property implicitly changes another public property, but not vice versa, in this way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(This comment was in reference to the previous design. Let me think about this one. I like it better...)

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 12, 2017

Choose a reason for hiding this comment

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

While I enjoy our online jousts this one is getting a bit OTT :) An app dev has two choices: either set verifyPeerSSLCertificate to false (not advised but can be useful if you self-sign during development) or set sslCertificateAuthorityFile to a valid URL of a pem fille copied from app resources. If they do both the more secure option prevails. If they do neither SSL doesn’t work until libcurl supports Android. I’d realy like to keep this simple rather than create a minefield of interlocks that need to be implemented and documented.

I could put in another guard if it would help:

            guard verifyPeerSSLCertificate else {
                fatalError("'sslCertificateAuthorityFile' set while peer verification disabled")
            }
            guard value.isFileURL && FileManager.default.fileExists(atPath: value.path) else {
                fatalError("'sslCertificateAuthorityFile' does not reference a file that exists: \(value.path)")
            }

I’d rather not implement get/set on verifyPeerSSLCertificate and introduce bugs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, thanks for bearing with me. I do like this design better. No need for the fatalError guard since you've documented the behavior. In fact, I think I like it better without the additional guard, as the guard itself is triggered or not depending on the order in which the properties are set :P

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 12, 2017

Choose a reason for hiding this comment

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

Cool. Guard removed. My preference. Now, about elided newlines in multiline strings...

Copy link
Collaborator

Choose a reason for hiding this comment

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

:D

verifyPeerSSLCertificate = true
free(_sslCertificateAuthorityFilePathPtr)
guard value.isFileURL && FileManager.default.fileExists(atPath: value.path) else {
fatalError("'sslCertificateAuthorityFile' set: not file URL or does not exist ***")
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about let's use a grammatical error message: "'sslCertificateAuthorityFile' does not reference a file that exists"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

return URL(fileURLWithPath: String(cString: value))
}
else {
fatalError("'sslCertificateAuthorityFile' get: no value available")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Likewise here: "'sslCertificateAuthorityFile' has not been set"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Altered to return nil

#if os(Android)
extension URLSession {
fileprivate static var _sslCertificateAuthorityFilePathPtr: UnsafeMutablePointer<Int8>?
/// You can bypass SSL peer verification altogether (not advised)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doc comments have a particular style in Swift. Here, let's try to mimic that:

/// A Boolean that is `true` if peer SSL certificates are to be verified.
///
/// You can bypass verification by setting this property to `false` (not recommended).

/// See https://curl.haxx.se/docs/sslcerts.html
/// For SSL to work you need a "cacert.pem" to be accessible at the path
/// provided by setting URLSession.sslCertificateAuthorityFile to a URL.
/// Downloadable here: https://curl.haxx.se/docs/caextract.html
Copy link
Collaborator

Choose a reason for hiding this comment

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

/// The URL representing a PEM-encoded certificate authority file to be used for peer SSL certificate verification.
///
/// See:
/// - https://curl.haxx.se/docs/sslcerts.html
/// - https://curl.haxx.se/docs/caextract.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Updated

@alblue
Copy link
Contributor

alblue commented Jul 12, 2017

@swift-ci please test

@johnno1962
Copy link
Contributor Author

Sorry, fix of typo..

@johnno1962
Copy link
Contributor Author

johnno1962 commented Jul 12, 2017

@phausler I think this PR is ready now and I’ve just done a final test run. Merge/squish away!

@johnno1962
Copy link
Contributor Author

johnno1962 commented Jul 14, 2017

For reference, the android CACerts are there but are not prepared correctly for OpenSSL 1.0.x to use http://www.javacms.tech/questions/1728116/how-to-make-ssl-peer-verify-work-on-android

@phausler
Copy link
Member

@swift-ci please test

return
}
guard value.isFileURL && FileManager.default.fileExists(atPath: value.path) else {
fatalError("'sslCertificateAuthorityFile' does not reference a file that exists: \(value.path)")
Copy link
Member

Choose a reason for hiding this comment

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

So if the path is a directory this won't hit btw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

D'oh

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 14, 2017

Choose a reason for hiding this comment

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

I’ve got a fix. Do you want me to push it through?

            var isDir: ObjCBool = false
            guard value.isFileURL && FileManager.default.fileExists(atPath: value.path,
                                                                    isDirectory: &isDir) && !isDir else {
                fatalError("'sslCertificateAuthorityFile' does not reference a file that exists: \(value.path)")
            }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed

guard value.isFileURL && FileManager.default.fileExists(atPath: value.path) else {
var isDir: ObjCBool = false
guard value.isFileURL && FileManager.default.fileExists(atPath: value.path,
isDirectory: &isDir) && !isDir else {
Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me

@phausler
Copy link
Member

@swift-ci please test

@johnno1962
Copy link
Contributor Author

johnno1962 commented Jul 15, 2017

Did that second "please test" go through?

@xwu
Copy link
Collaborator

xwu commented Jul 15, 2017

@johnno1962 ...Uh, did you mean to invalidate testing for this PR by pushing unrelated changes?

@@ -30,7 +30,11 @@ public func NSTemporaryDirectory() -> String {
return tmpdir
}
}
#if os(Android)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be outdented, if I recall, according to Philippe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just trying to follow the existing guards in the file.. Should I change those too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nah, you're right. I didn't see the other ones.

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 15, 2017

Choose a reason for hiding this comment

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

The following would also need to be changed but I’m trying to avoid conflicts.
modified: Foundation/Data.swift
modified: Foundation/DateFormatter.swift
modified: Foundation/NSData.swift
modified: Foundation/NSObjCRuntime.swift
modified: Foundation/NSPathUtilities.swift
modified: Foundation/NSUserDefaults.swift
modified: Foundation/NumberFormatter.swift
modified: Foundation/Process.swift

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 15, 2017

Choose a reason for hiding this comment

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

/data/local/tmp isn’t writable by an app anyway so I’ve reverted this change. You have to set TMPDIR env var to the Context.getCacheDir().getPath() for various things to work.

@johnno1962
Copy link
Contributor Author

johnno1962 commented Jul 15, 2017

Testing didn’t go through anyway for some reason… and I've resynced to master so these are required

@pushkarnk
Copy link
Collaborator

@swift-ci please test

1 similar comment
@pushkarnk
Copy link
Collaborator

@swift-ci please test

@pushkarnk
Copy link
Collaborator

pushkarnk commented Jul 17, 2017

👆You need to request twice, sometimes 😃

@johnno1962
Copy link
Contributor Author

Thanks @pushkarnk, looks good to me now.

@johnno1962
Copy link
Contributor Author

@phausler, would you like me to delete the “android" directory. It’s obsolete now.

@parkera
Copy link
Member

parkera commented Jul 18, 2017

Sure, that'd be good.

/// See:
/// - https://curl.haxx.se/docs/sslcerts.html
/// - https://curl.haxx.se/docs/caextract.html
public static var sslCertificateAuthorityFile: URL? {
Copy link
Member

Choose a reason for hiding this comment

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

Wait a minute here - are we attempting to add new Android-specific API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s Linux specific really where libcurl is being used but only essential on Android where the CA root certificates don't work with OpenSSL 1.0. Otherwise, no https: requests work. http://www.javacms.tech/questions/1728116/how-to-make-ssl-peer-verify-work-on-android

Copy link
Member

Choose a reason for hiding this comment

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

Where does the developer get the value to pass to this API? When do they set it? Why can't we just do the right thing for them automatically so they don't have platform-specific API to use?

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 18, 2017

Choose a reason for hiding this comment

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

It’s a long story on Android…. very difficult to replicate in C. The simplest way is to add a raw asset for the cacert.pem file then copy that to a place where you can get the actual file path in Java and pass that to Swift to set using this api. https://github.com/SwiftJava/swift-android-samples/blob/master/swifthello/src/main/java/net/zhuoweizhang/swifthello/SwiftHello.java#L35https://github.com/SwiftJava/swift-android-samples/blob/master/swifthello/src/main/swift/Sources/main.swift#L20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way we don’t fill up corefoundation with quite a bit of android specific NDK code.

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 18, 2017

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 18, 2017

Choose a reason for hiding this comment

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

OK, so it looks like an NDK solution is insurmountable. There simply isn’t enough api to find resources on an app that starts out as Java as far as I can see. The original implementation was an enivronment variable avoiding an API but that was wasn't secure which is how we arrived here.

@johnno1962
Copy link
Contributor Author

I thnk this is as ready to merge as it will ever be.

@xwu
Copy link
Collaborator

xwu commented Aug 24, 2017

@johnno1962 Can we leave the TimeZone changes out of this PR?

It’s quite another kettle of fish and the changes here aren’t clearly correct (for example: is GMT guaranteed to be equal to UTC for all time going forward? why does “GMT” need to be rewritten to “GMT+00”? should any of this be done in the Swift portion of Foundation or in CF? is simply logging the error acceptable, or should this fatalError on Android? ultimately, a complete solution would be to figure out how to hook up CF to Android’s tzinfo—this should not be insurmountable and may not even be more involved than the workaround.)

@johnno1962
Copy link
Contributor Author

No problem. It was mainly to get the tests working. I’ll check again if there is a solution in the NDK.

@johnno1962
Copy link
Contributor Author

@xwu I’ve resubmitted the time zone fix under #1189 in the finish. I’ve looked into the NDK options and they are very limited like they are for any UNIX. On my device there is no /usr/share/zoneinfo/ database but a single file in a proprietary format known only to Java. Answering your reservations one by one UTC and GMT are synonnyms for all intents an purposes in this context and GMT needs to be rewritten to GMT+0000 as this is the only format CFTimeZone.c understands without acces to a database by name. I’d rather not try to alter CFTimeZone.c as it is sourced differently from the swift and swift is a higher level language. An invalid Timezone should be fatal when you are debugging but a foundation library should not crash peoples apps out in the field IMO so I’ve opted for the latter. I’ve introduced a pseudo timezone “System” which people can use to format dates to the timezone of the device.

@xwu
Copy link
Collaborator

xwu commented Aug 26, 2017

@johnno1962 Let's discuss the time zone issue on #1189, then.

@amraboelela
Copy link
Contributor

Please don't delete the files in android directory as I am working on fixing them. Thanks.

@alblue
Copy link
Contributor

alblue commented Oct 13, 2017

@johnno1962 this PR has become somewhat stale with everything else that's been going on; can we close this one down and then later open a new PR for when the remaining changes can be viewed? I presume you've got the changes in a branch and can call them back when needed. Keeping it open won't help get it reviewed any faster, and rebasing/amending after the work on other PRs isn't likely to be of benefit.

By the way, it's easier for me to go through and review PRs when they're targeted to a specific issue; when there's multiple different potentially different changes overlaid (and many review comments) I tend to get lost. So I appreciate the work you've been doing in splitting them up and drip-feeding them as PRs so we can move things along. Thanks!

@johnno1962
Copy link
Contributor Author

@alblue, I’m still of the opinion this PR can be rehabilitated once the other PR’s have landed. It’s actually quite minor and I’d like to see it through on principle. Once #1198 has landed I’ll rebase and you’ll see what I mean.

@alblue
Copy link
Contributor

alblue commented Oct 13, 2017

@johnno1962 the problem is (from my reviewer's perspective) is that this thread has had 90+ comments on it and covered several different versions of the changes that you want to make, some of which are no longer relevant to the ultimate change set and with commentary that has diverged. So even if this change can be rehabilitated on this PR, it's still going to be difficult to go through and figure out what is still relevant and what isn't.

On the other hand, a new PR rebased onto master as smaller patches is much easier to get through, because they can be reviewed individually. It also helps from a punctuality perspective because more recently created PRs are towards the top of the list of reviews, and this one is languishing down at the bottom (so I have to remember to go and look for it specifically).

Finally, for this particular change set, it was a rebase and split from a prior PR with a number and 'android specific patches' as the title - so it's not really clear to me what this is actually trying to achieve.

So once the other patch set you're working on is changed, then rebasing and taking whatever is outstanding on this PR and creating a new one will make it much easier for me (and others) to help review and move through, especially if the resulting PR is relatively small. More frequent smaller PRs that are targeted to fix a specific issue are better than one large one with a lot of different changes.

Copy link
Contributor

@amraboelela amraboelela left a comment

Choose a reason for hiding this comment

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

Please explain the changes which seems to be affecting non-android environments.

@@ -27,7 +27,9 @@
#include <CoreFoundation/ForFoundationOnly.h>
#include <fts.h>
#include <pthread.h>
#ifndef __ANDROID__
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to have conflict with #1249

@@ -528,16 +532,16 @@ extension _HTTPURLProtocol: _EasyHandleDelegate {
}
}

func transferCompleted(withErrorCode errorCode: Int?) {
func transferCompleted(withError error: NSError?) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this related to Android only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait for the rebase

@johnno1962
Copy link
Contributor Author

johnno1962 commented Oct 13, 2017

Hi @alblue, chipping away at this rather ill-fated PR I’ve created #1264 which contains the original nub of the original PR. Hopefully we can get this merged without too much drama.

@johnno1962
Copy link
Contributor Author

johnno1962 commented Oct 13, 2017

@alblue et all. This PR has been rebased & tested and can be merged as is. It contains three changes:

  1. The original change to Foundation/NSURLSession/http/EasyHandle.swift that started all this off.
  2. Deleting the out of date scripts in the android directory.
  3. The remainder are the minor changes to have Foundation build for Android.

As item 3. will clash with @amraboelela’s versions there is also PR #1264 which will contains just items 1. & 2. which you could also merge - Take your pick. This PR has been built on OSX and Android where the foundation tests were run to competition (with the last commit applied for android.)

 Executed 1161 tests, with 25 failures (0 unexpected) in 16.021 (16.021) seconds

You can ignore almost all of the above comments as in the end the PR was presented separately and rebased to pretty much just the original changes to Foundation/NSURLSession/http/EasyHandle.swift

Thanks for the drive on merging outstanding PRs this week. It’s been a big help to me keeping sane!

@alblue
Copy link
Contributor

alblue commented Oct 16, 2017

Since #1264 has been put through test-and-merge, I'm going to close this down and leave any outstanding fixes to other/newly filed PRs. Thanks for splitting it up and making the process easier for us to review!

@alblue alblue closed this Oct 16, 2017
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.

7 participants