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

Port of Foundation to Android #622

Merged
merged 25 commits into from Sep 22, 2016
Merged

Port of Foundation to Android #622

merged 25 commits into from Sep 22, 2016

Conversation

johnno1962
Copy link
Contributor

@johnno1962 johnno1962 commented Sep 7, 2016

This PR contains the relatively minor changes to have Foundation compile with the Android NDK. There is an associated pull request with the small change apple/swift#4662 to build-script-impl in the swift project to pass through the required arguments to the ninja scritpt generator. The focus has been on getting the code to compile but preliminary testing of Regular expressions, NSHost, fetching Strings from URLs has been successful so far. There is a known issue with needing to hook thread exit to call JNI method DetachCurrentThread which I’ll have to look at before making a PR on swift-corelibs-libdispatch (resolved, see below).

Issues found and not resolved:
Optimising the creation of a string from zero length data - optimisation patched out for now
Fetching the current Locale language for web fetch request headers - also patched out

Changes tested on Linx and OSX build. Included is a minor change fixing a missing -lcurl when building "util/build-script —foundation" on Linux.

As foundation has binary dependencies on libxml2 and libcurl and it's not been able to automate the libdispatch crosscompile the build is dependent on a pre-built binary & header package downloaded by a series of scripts in the swift-corelibs-foundation/android directory. These binaries were built from the public repos pretty much as-is after some Makefilery. Details are in the README.md in this directory.

@@ -1264,12 +1264,15 @@ CF_PRIVATE CFStringRef __CFStringCreateImmutableFunnel3(
contentsDeallocator = __CFGetDefaultAllocator();
}

#ifndef DEPLOYMENT_TARGET_ANDROID
// crashes String.swift, line 51 when creating String from zero length Data
Copy link
Collaborator

@modocache modocache Sep 7, 2016

Choose a reason for hiding this comment

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

It'd be great to file a https://bugs.swift.org report to track this crash, and include the report number here. That way, we'll know to remove this #ifdef once the report is fixed.

Copy link
Member

@phausler phausler Sep 7, 2016

Choose a reason for hiding this comment

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

IIRC android's memcpy and memmove are not the same as Darwin. That may be the culprit of this issue.

@modocache
Copy link
Collaborator

modocache commented Sep 7, 2016

Issues found and not resolved:
Fetching the current Locale language for web fetch request headers - also patched out

I think this might be related?

return floor( Double(value ) )
}
private func ceil( value: CGFloat ) -> Double {
return floor( Double(value ) )
Copy link
Contributor

@mekjaer mekjaer Sep 7, 2016

Choose a reason for hiding this comment

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

ceil here?

@phausler
Copy link
Member

phausler commented Sep 7, 2016

For the Android port does this need to have a JNI_OnLoad or is that entry point higher?

@johnno1962
Copy link
Contributor Author

johnno1962 commented Sep 7, 2016

@phausler
Copy link
Member

phausler commented Sep 7, 2016

Will that mean we will need a hook in Thread.swift for VM context destruction?

@johnno1962
Copy link
Contributor Author

johnno1962 commented Sep 7, 2016

Absolutely. Android requires you to detach the thread and won’t allow you to do it with anything on the stack, claiming you are "stilll processing". Some help required here also. I haven’t made much headway understanding libdispatch, kqueues, etc. Threads seem to time out after 15 seconds if that sounds familiar crashing the app. Only way to stop it crashing is to keep making requests on the queue.

@@ -203,6 +206,7 @@ strlcat(char * dst, const char * src, size_t maxlen) {
}
return dstlen + srclen;
}
#endif
Copy link
Member

@parkera parkera Sep 7, 2016

Choose a reason for hiding this comment

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

Why is fd_mask defined in an else clause with definitions of strlcpy/strlcat? Are the two related in some way that I am missing?

Copy link
Contributor Author

@johnno1962 johnno1962 Sep 7, 2016

Choose a reason for hiding this comment

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

I’m just saving a line. I’ll separate them.

@johnno1962
Copy link
Contributor Author

johnno1962 commented Sep 8, 2016

New commit with responses to comments pushed. The battle with NSGeometry is won and Operation queues work if you make sure -DDEPLOYMENT_ENABLE_LIBDISPATCH is defined when you build. Less common code path does not seem to work. I’ll raise a ticket. Focus turns to the problem of detaching threads if they have interacted with JNI before they exit. Any hint about where to look appreciated.

@tkremenek
Copy link
Member

tkremenek commented Sep 16, 2016

@modocache Let's dicsuss extending the CI on swift-dev@swift.org, not in a pull request.

@johnno1962
Copy link
Contributor Author

johnno1962 commented Sep 19, 2016

Ready to go this end. All known issues resolved. @available(Android, unavailable) will have to wait.

@@ -196,6 +196,9 @@ internal func NSRequiresConcreteImplementation(_ fn: String = #function, file: S
}

internal func NSUnimplemented(_ fn: String = #function, file: StaticString = #file, line: UInt = #line) -> Never {
#if os(Android)
NSLog("\(fn) is not yet implemented. \(file):\(line)")
#endif
Copy link
Member

@parkera parkera Sep 22, 2016

Choose a reason for hiding this comment

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

Why did we add an NSLog here?

Copy link
Contributor Author

@johnno1962 johnno1962 Sep 22, 2016

Choose a reason for hiding this comment

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

fatalError() doesn’t print anything on Android (asserts do). Just giving people a pointer.

Copy link
Member

@phausler phausler Sep 22, 2016

Choose a reason for hiding this comment

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

shouldn't fatalError call __android_log_print on android?

Copy link
Contributor Author

@johnno1962 johnno1962 Sep 22, 2016

Choose a reason for hiding this comment

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

It should but this is a Foundation PR. I’ll take a look. Annoyingly, using __android_log_print requires a -llog when you link so it wouldn’t be a trivial change. Looks like a change to https://github.com/apple/swift/blob/master/stdlib/public/runtime/Errors.cpp#L204

Copy link
Contributor Author

@johnno1962 johnno1962 Sep 22, 2016

Choose a reason for hiding this comment

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

@parkera
Copy link
Member

parkera commented Sep 22, 2016

@swift-ci please test

@parkera
Copy link
Member

parkera commented Sep 22, 2016

Let's give this a shot.

@parkera parkera merged commit 16f83dd into apple:master Sep 22, 2016
@parkera
Copy link
Member

parkera commented Sep 22, 2016

We can follow up with separate changes for the rest. Thanks for tackling this @johnno1962 !

@johnno1962
Copy link
Contributor Author

johnno1962 commented Sep 22, 2016

Thanks Tony 👍

@johnno1962
Copy link
Contributor Author

johnno1962 commented Sep 23, 2016

Reminder: Example program https://github.com/SwiftJava/swift-android-samples,
Pre-compiled toolchain here http://johnholdsworth.com/android_toolchain.tgz

CFIndex maxLength = CFStringGetMaximumSizeForEncoding(CFStringGetLength(message), encoding) + 1;

if (maxLength > sizeof(stack_buffer) / sizeof(stack_buffer[0])) {
buffer = calloc(sizeof(char), maxLength);
Copy link
Contributor

@lhoward lhoward Sep 23, 2016

Choose a reason for hiding this comment

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

should check calloc() return value

Copy link
Contributor Author

@johnno1962 johnno1962 Sep 23, 2016

Choose a reason for hiding this comment

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

Too late now...

Copy link
Member

@parkera parkera Sep 23, 2016

Choose a reason for hiding this comment

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

Not too late - open up another PR. =)

Copy link
Contributor Author

@johnno1962 johnno1962 Sep 23, 2016

Choose a reason for hiding this comment

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

Alright already, new PR #655

@zayass
Copy link
Contributor

zayass commented Mar 21, 2017

Hi @johnno1962

it's not been able to automate the libdispatch crosscompile

Can you little explain why it is hard to automate and how you build this libdispatch binary?

@johnno1962
Copy link
Contributor Author

johnno1962 commented Mar 21, 2017

Things have moved on a little since this PR. Some detail about how to build libdispatch are on this PR apple/swift-corelibs-libdispatch#162

zayass added a commit to zayass/swift-corelibs-foundation that referenced this pull request Apr 18, 2017
zayass added a commit to zayass/swift-corelibs-foundation that referenced this pull request Apr 21, 2017
zayass added a commit to zayass/swift-corelibs-foundation that referenced this pull request Apr 21, 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.

None yet