Added build-android script #1249

amraboelela · 2017-10-05T16:34:56Z

No description provided.

alblue · 2017-10-06T10:28:07Z

.gitignore

@@ -20,3 +20,6 @@ Build
 *.swp
 *.orig
 .arcconfig
+
+*.tgz
+build.ninjae


Instead of doing this, why not change the sed script so that you're using an already known extension like ~ which will be ignored? Then you don't need to add this.

alblue · 2017-10-06T10:31:44Z

CoreFoundation/Base.subproj/CFPlatform.c

@@ -1325,11 +1325,13 @@ CF_SWIFT_EXPORT void _CFThreadSetName(const char *_Nullable name) {
 }

 CF_SWIFT_EXPORT int _CFThreadGetName(char *buf, int length) {
+#if !TARGET_OS_ANDROID


Rather than wrapping this in an if ! construct, why not add an #elif TARGET_OS_ANDROID above the #elif DEPLOYMENT_TARGET_LINUX line? Then there's a sensible place to add implementations in the future should it be needed.

Also, do you know why the variable is TARGET_OS_ANDROID rather than DEPLOYMENT_TARGET_ANDROID?

alblue · 2017-10-06T10:32:32Z

CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h

@@ -27,7 +27,10 @@
 #include <CoreFoundation/ForFoundationOnly.h>
 #include <fts.h>
 #include <pthread.h>
+
+#if !TARGET_OS_ANDROID


Would using the other syntax of #if __has_include(<execinfo.h>) work here, to keep it in line with the other tests?

alblue · 2017-10-06T10:34:38Z

build-android

+                        -DLIBDISPATCH_BUILD_DIR=$SWIFT_PATH/swift-corelibs-libdispatch &&
+
+                cp -r /usr/include/uuid $ANDROID_STANDALONE_SYSROOT/usr/include 
+                sed -ie "s/-I.\/ -I\/usr\/include\/x86_64-linux-gnu  -I\/usr\/include\/x86_64-linux-gnu    -I\/usr\/include\/libxml2//" build.ninja


(From above) if this was sed -i~ instead, you would generate build.ninja~ as the backup file, which would automatically be excluded.

How about if I just do "sed -i" will it edit without backup? I don't need to generate backup.

@amraboelela Yes it should do, from the manage:

-i[SUFFIX], --in-place[=SUFFIX] edit files in place (makes backup if extension supplied)

I've seen some versions of using sed -i when you don't explicitly specify an extension, but it then takes the following word from the command line and uses that as the extension itself. I would recommend using sed -i~ so that the backups aren't committed to git (the *~ pattern is already in the .gitattributes file) and it avoids any versions of sed where running sed -i something doesn't get translated as "use something as the backup extension".

This still looks outstanding from GitHub's diffs?

alblue · 2017-10-06T10:35:31Z

.gitignore

@@ -20,3 +20,6 @@ Build
 *.swp
 *.orig
 .arcconfig
+
+*.tgz


It's not clear what this is blocking here, and would prevent people adding .tgz examples in the future (for example). Can you explain why this is needed?

Still need to know why this *.tgz is here. Can you remove this from the commit or justify why it's necessary?

alblue

Can you remove the .tgz, change the sed -ie to sed -i~, and rebase the changes (not via merge) so that we can clearly see what's added?

The changes seem to imply more than was originally added, but it's not clear if that's due to the merge and GitHub showing the differences. A clean rebase without the empty merge node and fixing the minor issues should help.

alblue · 2017-10-11T18:24:39Z

.gitignore

@@ -20,3 +20,6 @@ Build
 *.swp
 *.orig
 .arcconfig
+
+*.tgz


Still need to know why this *.tgz is here. Can you remove this from the commit or justify why it's necessary?

alblue · 2017-10-11T18:25:42Z

build-android

+                        -DLIBDISPATCH_BUILD_DIR=$SWIFT_PATH/swift-corelibs-libdispatch &&
+
+                cp -r /usr/include/uuid $ANDROID_STANDALONE_SYSROOT/usr/include 
+                sed -ie "s/-I.\/ -I\/usr\/include\/x86_64-linux-gnu  -I\/usr\/include\/x86_64-linux-gnu    -I\/usr\/include\/libxml2//" build.ninja


This still looks outstanding from GitHub's diffs?

alblue

Can you revert the .gitignore so that it doesn't make any changes to this commit?

Also, can you rebase the code onto master and get rid of the merge nodes so that we can see what is happening? The merge node committed previously makes it a little unclear as to what is here. Doing a git pull --rebase should fix this up.

alblue · 2017-10-11T18:56:49Z

.gitignore

@@ -20,3 +20,4 @@ Build
 *.swp
 *.orig
 .arcconfig
+


This file doesn't need to be changed, and the addition of a newline doesn't add any value. Can you revert the file to what it was beforehand?

amraboelela · 2017-10-11T19:18:43Z

I am now using -i~ but git status gives:

(use "git add ..." to include in what will be committed)

build.ninja~

Also I am not able to rebase the branch correctly.

amraboelela · 2017-10-12T15:22:45Z

I am finally able to rebase it after fixing the conflicts :)

alblue · 2017-10-12T15:46:22Z

@swift-ci please test

alblue · 2017-10-12T16:49:04Z

@swift-ci please test

alblue

Sorry to be a pain - just when all the tests had run successfully, too :)

The TestNSLock.swift has ended up reverting a recent PR - can you remove this change from your request?

alblue · 2017-10-12T18:05:38Z

TestFoundation/TestNSLock.swift

@@ -41,14 +41,11 @@ class TestNSLock: XCTestCase {
        }

        let thread = Thread() {
-            lock.lock()


This appears to revert #1261 presumably accidentally. Can you remove this file from this PR?

xwu

Since there's some more edits required anyway, here's some drive-by formatting nits.

xwu · 2017-10-12T22:55:28Z

Foundation/Host.swift

@@ -28,6 +28,10 @@ open class Host: NSObject {
    internal var _names = [String]()
    internal var _addresses = [String]()

+    #if os(Android)


Nit: #if and #endif should be de-indented (no spaces at the beginning of the line before #) and the enclosed statements should be indented as though it isn't surrounded by a compile-time condition (that is, static internal let NI_MAXHOST here should align with static internal let _current).

xwu · 2017-10-12T22:55:41Z

Foundation/Host.swift

@@ -36,7 +40,7 @@ open class Host: NSObject {
    }

    static internal func currentHostName() -> String {
-        let hname = UnsafeMutablePointer<Int8>.allocate(capacity: Int(NI_MAXHOST))
+       let hname = UnsafeMutablePointer<Int8>.allocate(capacity: Int(NI_MAXHOST))


Please restore the original indentation.

This one as well.

xwu · 2017-10-12T22:55:54Z

Foundation/Host.swift

@@ -65,6 +69,9 @@ open class Host: NSObject {
    }

    internal func _resolveCurrent() {
+        #if os(Android)


Same nit as above about indentation of #if and #endif.

xwu · 2017-10-12T22:56:05Z

Foundation/Host.swift

@@ -65,6 +69,9 @@ open class Host: NSObject {
    }

    internal func _resolveCurrent() {
+        #if os(Android)
+        return
+        #else


Likewise with indentation of #else.

xwu · 2017-10-12T22:56:40Z

Foundation/Thread.swift

@@ -256,7 +256,11 @@ open class Thread : NSObject {
        let maxSupportedStackDepth = 128;
        let addrs = UnsafeMutablePointer<UnsafeMutableRawPointer?>.allocate(capacity: maxSupportedStackDepth)
        defer { addrs.deallocate(capacity: maxSupportedStackDepth) }
+        #if os(Android)


Same nit here about indentation of #if, #else and #endif.

xwu · 2017-10-12T22:57:49Z

Foundation/Thread.swift

@@ -270,6 +274,9 @@ open class Thread : NSObject {
    }

    open class var callStackSymbols: [String] {
+        #if os(Android)


Same nit here about #if, #else and #endif; please also de-indent return [] by four spaces so that it aligns with return backtraceAddresses.

alblue · 2017-10-13T08:28:16Z

@amraboelela There was another change recently from @johnno1962 that was merged yesterday regarding Android support: #1189

Can you rebase the changes on top of these and test that your changes still make sense? I'm not sure if there's overlap (the lack of conflicts suggests not) but it would be worth verifying it for Android support.

Separately, if you're interested, you can configure your git repository with git config pull.rebase true which means whenever you do a pull, it'll automatically rebase it instead of creating merge nodes. This makes it much easier to keep up-to-date with the branches :)

If you're doing local changes, you might also want to use git config rebase.autostash true - this allows you to a pull in place while not losing your local changes.

The above will apply to the single repository that you run in; if you want them to apply to all repositories, run git config --global with the above. Hope that's of interest!

Finally, keep up the good work. I know there's been a lot of back-and-forth on this, as well as delays, for which I apologise. I am working to clear the backlog and want to merge these changes in so keep going👍🏻

amraboelela · 2017-10-13T15:50:04Z

All good now?

amraboelela · 2017-10-13T15:58:57Z

@alblue did Apple assign you the task of "cleaning out" all the PRs in Foundation? :)

What about swift repo, I have a related PR there needs to get moving: swiftlang/swift#11870

And also in libdispatch: swiftlang/swift-corelibs-libdispatch#302

alblue

Few minor nits - @xwu might be able to clarify the indentation specifics too

alblue · 2017-10-13T16:43:00Z

Foundation/Host.swift

    }

    internal func _resolve() {
+#if os(Android)
+    return


It looks like the indentation of this return doesn't line up - it should be one indent to the right, I think. @xwu can you confirm?

Yeah, return needs four additional preceding spaces.

alblue · 2017-10-13T16:45:00Z

.gitignore

@@ -10,6 +10,7 @@ project.xcworkspace

 # build files generated by the configure script
 *.ninja
+*.ninja~


Since we're making other changes to this script, I'd suggest putting *~ at the top of the file and removing this line. That way any backup file won't get committed. It's a fairly standard thing to have in Linux repos, but tends to be forgotten on Mac ones.

See e.g. https://github.com/github/gitignore/blob/master/Global/Linux.gitignore#L1

alblue · 2017-10-13T16:46:00Z

Foundation/Host.swift

@@ -28,6 +28,10 @@ open class Host: NSObject {
    internal var _names = [String]()
    internal var _addresses = [String]()

+#if os(Android)
+        static internal let NI_MAXHOST = 1025


This line looks like it doesn't line up with the static internal let below - can you indent it to the left?

Please don't forget to address this comment

alblue · 2017-10-13T16:46:16Z

Foundation/Thread.swift

@@ -270,6 +274,9 @@ open class Thread : NSObject {
    }

    open class var callStackSymbols: [String] {
+#if os(Android)
+            return []


I think the return here should line up with the return of the return backtraceAddresses below. It's not rendering as that in the GitHub UI. Maybe a mix of tabs and spaces causing problems?

Looks like all spaces here, but agree that return needs to be de-indented four spaces.

alblue · 2017-10-13T16:47:35Z

@alblue did Apple assign you the task of "cleaning out" all the PRs in Foundation? :)

No, I'm just trying to help clear out the queue :)

alblue · 2017-10-13T16:54:04Z

One other thing might help - there's two sorts of changes here; stubbing out APIs that don't work on Android, and the build script to help build it. Can we split those into two PRs? The build-script and the .gitignore can go in one and the fixes to help it build in another.

In swift's swiftlang/swift#11870 repo there seems to be a proposal of moving towards a CMake instead of autotools for building the code. Is that an approach we can think of using here?

amraboelela · 2017-10-13T17:18:25Z

If we don’t include those changes, the build would give errors, so they should go together.

amraboelela · 2017-10-13T17:25:17Z

The cmake proposal is for libdispatch I think. But the cmake script is not ready yet, so I suggest to keep the autoconf build script for now.

alblue · 2017-10-13T18:39:10Z

If we don’t include those changes, the build would give errors, so they should go together.

I was thinking, maybe we commit the code changes first, then have the build script as a dependent change.

The cmake proposal is for libdispatch I think. But the cmake script is not ready yet, so I suggest to keep the autoconf build script for now.

OK. We don't have a CMake list for now in foundation, so that makes sense. It would be interesting to know how much it would be to convert it to CMake in the future. Maybe that's something that @compnerd could advise with.

alblue · 2017-10-13T19:26:56Z

@swift-ci please test

xwu · 2017-10-14T19:43:02Z

CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h


-#if __has_include(<malloc/malloc.h>)
-#include <malloc/malloc.h>
+#if __has_include(<execinfo.h>)


What is going on here now? Why is execinfo.h being conditionally included twice?

alblue · 2017-10-16T11:02:54Z

Foundation/FileManager.swift

@@ -343,6 +343,9 @@ open class FileManager : NSObject {
        This method replaces fileSystemAttributesAtPath:.
     */
    open func attributesOfFileSystem(forPath path: String) throws -> [FileAttributeKey : Any] {
+#if os(Android)
+        NSUnimplemented()


In this line, NSUnimplemented() means 'This hasn't been implemented yet, but it will be in the future' - whereas NSUnsupported() means that 'This will not be implemented on this platform'. Can you confirm this is intended to be implemented in the future? If not, using NSUnsupported() would be preferable.

I think we should keep it NSUnimplemented, as someone later should be able to implement it in Android.

alblue · 2017-10-16T13:13:07Z

Foundation/Thread.swift

@@ -256,7 +256,11 @@ open class Thread : NSObject {
        let maxSupportedStackDepth = 128;
        let addrs = UnsafeMutablePointer<UnsafeMutableRawPointer?>.allocate(capacity: maxSupportedStackDepth)
        defer { addrs.deallocate(capacity: maxSupportedStackDepth) }
+#if os(Android)
+        let count = maxSupportedStackDepth


The count here is the number of successful frames returned by backtrace, and that they've been filled in by the caller. Since they won't have been any returned, isn't 0 the correct answer in the face of not being implemented? Admittedly this is being stubbed out from the caller below, but if it gets added elsewhere later then it would be better if this didn't contain a known bad number.

alblue · 2017-10-16T13:15:37Z

build-android

+
+set -e
+
+cd "$(dirname $0)/.." || exit 1


Small observation; this changes the directory you're currently in. If you wanted to just find out what directory you are in, you can do it in parenthesis e.g.

https://github.com/apple/swift/blob/cb3fcd4b83410f93558bb449695e1f1c2e69cfb9/utils/swift_build_support/test_swift_build_support.sh#L5

alblue · 2017-10-16T13:17:10Z

@swift-ci please test

alblue · 2017-10-16T15:33:25Z

@swift-ci please test

alblue · 2017-10-16T18:26:19Z

@swift-ci please test

alblue · 2017-10-17T09:33:38Z

@swift-ci please test

alblue · 2017-10-17T10:40:42Z

@swift-ci please test and merge

amraboelela · 2017-10-17T15:23:25Z

Yeaaah :)
Thank you @alblue

amraboelela mentioned this pull request Oct 5, 2017

Added build-android script #1241

Closed

alblue suggested changes Oct 6, 2017

View reviewed changes

alblue suggested changes Oct 11, 2017

View reviewed changes

amraboelela force-pushed the build-android-script branch from 11df58b to a406011 Compare October 11, 2017 18:45

alblue suggested changes Oct 11, 2017

View reviewed changes

amraboelela force-pushed the build-android-script branch from 02cee11 to a406011 Compare October 11, 2017 19:08

amraboelela force-pushed the build-android-script branch from 096cb57 to 2573116 Compare October 12, 2017 15:16

alblue approved these changes Oct 12, 2017

View reviewed changes

alblue suggested changes Oct 12, 2017

View reviewed changes

xwu reviewed Oct 12, 2017

View reviewed changes

amraboelela force-pushed the build-android-script branch 3 times, most recently from 4d5bc3f to 2ba419d Compare October 13, 2017 15:49

amraboelela mentioned this pull request Oct 13, 2017

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

Closed

alblue suggested changes Oct 13, 2017

View reviewed changes

alblue mentioned this pull request Oct 13, 2017

Required For HTTPS URLSession requests to work on Android #1264

Merged

xwu reviewed Oct 14, 2017

View reviewed changes

amraboelela force-pushed the build-android-script branch from 6653b57 to ad169db Compare October 15, 2017 14:28

alblue reviewed Oct 16, 2017

View reviewed changes

alblue approved these changes Oct 16, 2017

View reviewed changes

amraboelela force-pushed the build-android-script branch from 74fd485 to 59c4cbe Compare October 16, 2017 16:50

Added build-android script

b42c144

amraboelela force-pushed the build-android-script branch from 51bbadb to b42c144 Compare October 16, 2017 17:05

swift-ci merged commit 12c371a into swiftlang:master Oct 17, 2017

amraboelela deleted the build-android-script branch October 20, 2017 19:25

Added build-android script #1249

Added build-android script #1249

Uh oh!

Conversation

amraboelela commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amraboelela commented Oct 11, 2017

Uh oh!

amraboelela commented Oct 12, 2017

Uh oh!

alblue commented Oct 12, 2017

Uh oh!

alblue commented Oct 12, 2017

Uh oh!

alblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xwu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alblue commented Oct 13, 2017

Uh oh!

amraboelela commented Oct 13, 2017

Uh oh!

amraboelela commented Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amraboelela commented Oct 13, 2017 •

edited

Loading

amraboelela Oct 16, 2017 •

edited by alblue

Loading