Changes required for swift-4.1-branch port to Android #1518

johnno1962 · 2018-04-16T16:23:06Z

Hi Apple,

Could you please consider merging this PR derived from the changes required to get the swift-4.1-branch running on Android. The bulk of the PR has to adjust the memory model of CF instances that use NSbridged classes for storage. I found that the sizeof the CF structure (which is more of a “stride”) was greater than the instance size Swift allocated resulting in the initialisation of the CF instance writing off the end of allocated Swift instance memory. This is catered for by the _CFPad struct and probably applies to all 32 bit ports of Swift. The remainder of changes are various tweaks to get tests running and to accommodate the environment inside an Android application for things such as NSUserDefaults.

Test results are:

Test Suite 'All tests' failed at 2018-04-16 16:16:34.606
	 Executed 1239 tests, with 23 failures (0 unexpected) in 17.16 (17.16) seconds

millenomi · 2018-04-16T20:43:10Z

CoreFoundation/Base.subproj/CFBase.h

@@ -448,6 +448,12 @@ CF_EXPORT double kCFCoreFoundationVersionNumber;
 #define kCFCoreFoundationVersionNumber_iOS_9_x_Max 1299
 #endif

+#ifdef __ANDROID__
+typedef uint16_t _swift_rc_type;


Types that are private to CF should be in camel-case style with the _CF prefix. _CFSwiftRCCount? _CFSwingReferenceCount?

This should not be in CFBase.h at all. This is a public header and this is totally internal to CF.

Thanks for the comments. I can change this type to _CFSwiftRCCount and #ifdef ANDROID’s in this PR to DEPLOYMENT_TARGET_ANDROID no problem. CFBase.h seemed to be the only place I could define this type which is included by both CFString.h and CFRuntime.h. It is also important it is available to Swift in NSCFString.swift for the code to hold together so if an accommodation could be made here that would make the most sense to me.

I think we talked about it previously, but you explain again why the RC type is 16 bits only on android?

I think it’s unlikely it’s actually 16 bits on Android, more likely there is no _swift_weak_rc at all but using 16 bits is a way to confine the changes required for Android to various macros. What I do know is that Swift ivars start 8 bytes into the instance so this is one way to model this.

If the layout doesn't include a weak retain count, something is deeply, deeply weird in compiler land.

¯\_(ツ)_/¯

Can you figure out why the compiler is laying out things differently on Android?

millenomi · 2018-04-16T20:43:40Z

CoreFoundation/Base.subproj/CFInternal.h

@@ -216,7 +216,7 @@ extern void __CFGenericValidateType_(CFTypeRef cf, CFTypeID type, const char *fu
 #define __CFBitfield64GetValue(V, N1, N2)	(((V) & __CFBitfield64Mask(N1, N2)) >> (N2))
 #define __CFBitfield64SetValue(V, N1, N2, X)	((V) = ((V) & ~__CFBitfield64Mask(N1, N2)) | ((((uint64_t)X) << (N2)) & __CFBitfield64Mask(N1, N2)))

-#if __LP64__
+#if __LP64__ || defined(__ANDROID__)


What is the need here? Can we document why we need to special-case Android, or can we use a compiler capability flag if that's not the case?

This #if LP64 seems to be a leftover as __CFInfoType is always uint64_t in the actual definition in CFRuntime.h. I added this defined(ANDROID) to silence the many warnings telling me something was awry. This #if should probably be removed altogether.

millenomi · 2018-04-16T20:44:26Z

CoreFoundation/Base.subproj/CFRuntime.h

@@ -193,8 +193,8 @@ CF_EXPORT void _CFRuntimeUnregisterClassWithTypeID(CFTypeID typeID);
 typedef struct __CFRuntimeBase {
    // This matches the isa and retain count storage in Swift
    uintptr_t _cfisa;
-    uint32_t _swift_strong_rc;
-    uint32_t _swift_weak_rc;
+    _swift_rc_type _swift_strong_rc;


(See above.)

millenomi · 2018-04-16T20:44:42Z

CoreFoundation/Base.subproj/CFUtilities.c

@@ -1000,6 +1000,7 @@ void CFLog1(CFLogLevel lev, CFStringRef message) {
        CFStringGetCString(message, buffer, maxLength, encoding);

    __android_log_print(priority, tag, "%s", buffer);
+    fprintf(stderr, "%s\n", buffer);


What is this for? Isn't __android_log_print enough?

This is helpful when you are running an Android binary (such as the tests) from the command line using an “adb shell". Otherwise NSLogs are only visible using an “adb logcat” by which time the messages have lost their context. This doesn’t affect normal running of an actual application and the logging to stderr goes into the void.

millenomi · 2018-04-16T20:45:39Z

CoreFoundation/PlugIn.subproj/CFBundle.c

@@ -714,7 +714,7 @@ static CFBundleRef _CFBundleCreate(CFAllocatorRef allocator, CFURLRef bundleURL,

    bundle->_url = newURL;

-#if !DEPLOYMENT_RUNTIME_OBJC && !DEPLOYMENT_TARGET_WINDOWS
+#if !DEPLOYMENT_RUNTIME_OBJC && !DEPLOYMENT_TARGET_WINDOWS && !defined(__ANDROID__)


I'm thinking if, for consistency, there shouldn't be a DEPLOYMENT_TARGET_ANDROID that we could use. I'm also pretty sure there are other FHS bits — it should be entirely enabled or entirely disabled.

What exactly is FHS?? There may be other areas I should be #ifdeffing out but this is the only one that was causing a crash.

FHS is the Filesystem Hierarchy Spec, but in this context it refers to bundles that store contents pursuant that spec: https://forums.swift.org/t/proposal-foundation-allowing-for-system-bundles-on-linux/7417/6

Android is not a FHS system and should not have those on.

millenomi · 2018-04-16T20:57:55Z

Foundation/NSCFArray.swift

@@ -12,7 +12,9 @@ import CoreFoundation

 internal final class _NSCFArray : NSMutableArray {
    deinit {
+#if !os(Android)


I'm pretty sure this is semantically incorrect. Can you explain why not this?

Ahhh, yes… this does rather stand out as being an area that should be investigated. Basically if this call is made the app will crash further down the line due presumably to freeing invalid block or freeing them when they shouldn’t be. I've made a few passes at trying to get to the bottom of this but had no luck. Avoiding making this call solves the problem at the cost of a minor leak. I’m not even sure self is a CFArray instance at this point as even just asking for the count will crash the app and I have no idea where the pointer is coming from.

I'd rather we understand this before a merge.

This is resolved - see below.

millenomi · 2018-04-16T20:58:46Z

Foundation/NSObjCRuntime.swift

@@ -228,6 +228,14 @@ internal struct _CFInfo {
    }
 }

+internal struct _CFPad {
+#if os(Android)


I'd want compiler people to chime in on this to understand if the guarantee this code relies on re: sizing will hold. Who could we cc here?

millenomi · 2018-04-16T20:59:08Z

Foundation/NSString.swift

@@ -630,6 +630,10 @@ extension NSString {
    }

    public var boolValue: Bool {
+#if os(Android)


This makes it inconsistent from other platforms only on Android. What's the reasoning here?

For whatever reason the NSString.boolValue implementation crashes and it exercises quite a bit of code including charsets which is where the problem most likely lies. I put this equivalent in as a stop-gap to keep moving. If you ask me this is what the implementation should be anyway. The Scanner/Charset version seems to make a bit of a meal of it.

In general I don't believe stopgaps that diverge semantics are mergeable. We should probably understand the root case instead. (I do understand this means potentially more work, sorry about that.)

The semantics do not diverge. The Android code is exactly equivalent to the existing implementation.

OK, I think I’ve found a solution to this and the NSCFArray _CFDeinit problem though not an entirely satisfactory explanation. I found that in this code using character sets you could log the character set instance but not the set.inverted instance. set.inverted is a little unusual in that it calls _CFCharacterSetCreateMutableCopy to duplicate the charset before inverting it which calls _CFRuntimeCreateInstance so NSCharacterSet is not being used as the substrate for the CF instance. I found by adding 4 bytes to the memory allocated (but not initialised) here these problems go away and the tests run. I’d add a comment but I don’t quite know what to say as a justification! Perhaps the author of the bridging code could chime in as to why this would be the case. NSCharacterSet is a bit of a special case as it uses _SwiftNativeFoundationType for “boxing” mutable and inimitable instances.

millenomi · 2018-04-16T20:59:47Z

TestFoundation/TestFileManager.swift

@@ -228,6 +228,9 @@ class TestFileManager : XCTestCase {
    }

    func test_fileSystemAttributes() {
+#if os(Android)


Patches with tests that fail can't be merged. Would you like to instead work with us to figure out a way to mark the attributes API as not available on Android?

Filemanager.swift marks this as NSUnimplmented so I didn’t have much option if the tests were to run. Perhaps it should be made unavailable if you can suggest the syntax for me but then what should the test do?

If this only fails on Android, sure — it will be more noise if anyone sets up CI for this, though. In general, on platforms we CI, we'd just #if the test content out with an explanatory comment.

ok, I’ll remove the XCFail as there is nothing that can be done about this.

millenomi · 2018-04-16T21:00:35Z

TestFoundation/TestNSNumber.swift

@@ -1087,33 +1087,33 @@ class TestNSNumber : XCTestCase {
            XCTAssertEqual(NSNumber(value: UInt.min).stringValue, "0")
            XCTAssertEqual(NSNumber(value: UInt.min + 1).stringValue, "1")
            XCTAssertEqual(NSNumber(value: UInt.max).stringValue, "4294967295")
-            XCTAssertEqual(NSNumber(value: UInt.max - 1).stringValue, "4294967294")
+            XCTAssertEqual(NSNumber(value: UInt.max - 1 as UInt).stringValue, "4294967294")


Is this and similar a 32-bit guarantee violation? Why doesn't it work on Android as written? If it is incorrect, it should be diagnosed as incorrect on all platforms to avoid forking the language.

This is a strange problem indeed which I don’t know the solution for other than the change you see. It doesn’t seem to be 32 bit specific but something not quite right about the compiler and subtraction. I’m seeing warnings I don’t expect and the errors I had to fix with this change.

Sources/TestNSNumber.swift:1148:50: warning: '-' is deprecated: Please use explicit type conversions or Strideable methods for mixed-type arithmetics.
XCTAssertEqual(NSNumber(value: Int64.max - 1).stringValue, "9223372036854775806")
^
Sources/TestNSNumber.swift:1090:53: error: arithmetic operation '4294967295 + 4294967295' (on unsigned 32-bit integer type) results in an overflow
XCTAssertEqual(NSNumber(value: UInt.max - 1).stringValue, "4294967294")
~~~~~~~~ ^ ~

The compiler is basically built right out of the swift-4.1-branch with only three minor unrelated patches.

Something is deeply weird with that error, but the fix makes sense.

You may want to add a comment for this line (or jira if there is one) as otherwise it may get reverted by someone 'cleaning up' the tests in the future.

https://bugs.swift.org/browse/SR-7469

I think we still need @spevans' suggested comment here

Will include in next push

parkera · 2018-04-16T21:17:03Z

CoreFoundation/Base.subproj/CFBase.h

@@ -448,6 +448,12 @@ CF_EXPORT double kCFCoreFoundationVersionNumber;
 #define kCFCoreFoundationVersionNumber_iOS_9_x_Max 1299
 #endif

+#ifdef __ANDROID__
+typedef uint16_t _swift_rc_type;


This should not be in CFBase.h at all. This is a public header and this is totally internal to CF.

parkera · 2018-04-16T21:26:27Z

CoreFoundation/Base.subproj/CFPlatform.c

@@ -297,6 +297,9 @@ CF_EXPORT CFStringRef CFCopyFullUserName(void) {
    uid_t euid;
    __CFGetUGIDs(&euid, NULL);
    struct passwd *upwd = getpwuid(euid ? euid : getuid());
+#ifdef __ANDROID__


I would like to use a TARGET_OS_ANDROID value instead, if possible, to match the rest of our macros.

parkera · 2018-04-16T21:27:35Z

CoreFoundation/PlugIn.subproj/CFBundle_Main.c

@@ -79,12 +79,20 @@ static CFBundleRef _CFBundleGetMainBundleAlreadyLocked(void) {
        CFStringRef str = NULL;
        CFURLRef executableURL = NULL, bundleURL = NULL;
        _initedMainBundle = true;
+#ifdef __ANDROID__


I would stick with the CF behavior of using the current path in this case.

codestergit · 2018-04-18T13:21:36Z

This might be dumb but @johnno1962 @millenomi what is the benefit of porting it to the android. May I know the usecase.

johnno1962 · 2018-04-18T14:59:33Z

Dunno. Why did Hillary climb Everest? Why did the chicken cross the road? Why do people learn Haskel? I think the main idea is to open the possibility of code sharing across the two platforms.

spevans · 2018-04-18T16:10:21Z

@johnno1962 Regarding test_fileSystemAttributes(), it might be best to mark the original function unavailable as you suggest above. Its a bit ugly but I think you can use the following:

#if os(Android)
@available(*, unavailable, message: "Unsuppported on this platform")
open func attributesOfFileSystem(forPath path: String) throws -> [FileAttributeKey : Any] {
   NSUnsupported()
}
#else
open func attributesOfFileSystem(forPath path: String) throws -> [FileAttributeKey : Any] {
  .
  .
  .
}
#endif

johnno1962 · 2018-04-18T19:46:01Z

Thanks @spevans this works I’ll add it to the next commit.

johnno1962 · 2018-04-18T19:55:37Z

Just an observation here.. I’m sure you’re wondering @parkera why all this padding (8 bytes extra to instances ), 4 bytes extra to CF structs is necessary when it wasn’t for last year’s ~4.0 release or for Linux. I’ve done a bit of digging and the odd thing is, while Swift ivars start at offset 8 from self in binaries built with the new swift-4.1-branch compiler they were 12 bytes from self using the last release which is more what you would expect. This, along with the extra 4 bytes in CF structs to align the now int64_t _cfinfoa (a.k.a CFInfo) which used to be char[8] result in the instances being smaller than the CFstructs which use them. Is there a friendly local compiler bod we can consult at this stage whether this offset change is explicable?

parkera · 2018-04-18T20:00:43Z

If the swift runtime changed the layout of that type, then we need to change the layout of our matching struct to match on all platforms.

johnno1962 · 2018-04-18T20:57:09Z

This is what changed: SwiftJava/swift@ae1c984, mystery solved. strong count is 22 bits, weak is 7 bits

parkera · 2018-04-18T21:19:05Z

Can we start a separate PR just to fix up the size of the ref count for CFBase then?

johnno1962 · 2018-04-18T21:23:52Z

Sure, for all non-__LP64__ not just Android?

johnno1962 · 2018-04-22T14:14:09Z

Now the memory model is cleaned up in #1525 I’ve been able to tidy up this PR by removing the messy padding of allocations and the tests run through fine now on Android 5.1.1 though 7.0. Ready to merge.

spevans · 2018-04-22T14:17:05Z

@swift-ci please test

spevans · 2018-04-22T15:10:47Z

CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h

@@ -403,6 +403,8 @@ static inline _Bool _withStackOrHeapBuffer(size_t amount, void (__attribute__((n
 static inline int _direntNameLength(struct dirent *entry) {
 #ifdef _D_EXACT_NAMLEN  // defined on Linux
    return _D_EXACT_NAMLEN(entry);
+#elseif DEPLOYMENT_TARGET_ANDROID


@johnno1962 Build is failing, this needs to be #elif

odd. it built on macOS, anyway it’s amended. Can you try again please?

spevans · 2018-04-22T15:21:06Z

@swift-ci please test

parkera · 2018-04-23T15:53:30Z

CoreFoundation/NumberDate.subproj/CFTimeZone.c

+    // Approximate with gmtoff - could be general default.
+    struct tm info;
+    time_t now = time(NULL);
+    if (NULL != localtime_r(&now, &info))


Style nitpick - for more than one line, please use { }

Will do in the next —amend if we can reach agreement on what to do about bundles.

parkera · 2018-04-23T15:54:14Z

CoreFoundation/PlugIn.subproj/CFBundle_Main.c

@@ -79,12 +79,20 @@ static CFBundleRef _CFBundleGetMainBundleAlreadyLocked(void) {
        CFStringRef str = NULL;
        CFURLRef executableURL = NULL, bundleURL = NULL;
        _initedMainBundle = true;
+#if DEPLOYMENT_TARGET_ANDROID
+        const char *bundlePath = getenv("CFFIXED_USER_HOME") ?: getenv("TMPDIR") ?: "/data/local/tmp";


I'm still not sure I understand this. What does a main bundle pointed at what appears to be a temporary directory get you?

We need a bundle directory for things like path(toResource:ofType:) to work but on Android there is no equivalent and resources are stored compressed in the app’s .apk to be read on demand through a stream. The model I’m proposing is to copy the resources out to the only writable directory available (the cacheDir) which is also the TMPDIR required for other parts of foundation to work (downloads etc.). Perhaps it would be clearer to have a separately named environment variable like CFBUNDLE_HOME but that would be just more to set up for apps to work and to document.

If you're using path(toResource:ofType:) then you are also asserting you know where your resources are and how they're structured. This is not the case for the contents of a potentially system-shared temporary directory.

If clients are handling their own copying of resources, they should not be using Bundle APIs to fetch paths to them, unless they are copying an actually well-formed bundle somewhere and using Bundle(url:) or Bundle(path:).

parkera · 2018-04-23T15:55:03Z

CoreFoundation/Preferences.subproj/CFPreferences.c

@@ -440,7 +440,11 @@ static CFURLRef _CFPreferencesURLForStandardDomainWithSafetyLevel(CFStringRef do
    CFURLRef theURL = NULL;
    CFAllocatorRef prefAlloc = __CFPreferencesAllocator();
 #if DEPLOYMENT_TARGET_MACOSX || DEPLOYMENT_TARGET_LINUX || DEPLOYMENT_TARGET_WINDOWS
+#if DEPLOYMENT_TARGET_ANDROID


Just double checking here -- we set both DEPLOYMENT_TARGET_LINUX and DEPLOYMENT_TARGET_ANDROID for android?

In the current build script both are set. For android the existing conditionals for Linux apply.

parkera · 2018-04-23T15:55:39Z

CoreFoundation/URL.subproj/CFURL.c

 #include <syslog.h>
+#else
+#include <sys/syslog.h>


Why change this from __has_include style?

This is just a minor portability fix. We need a syslog.h but probably not both and if you include sys/syslog.h on android which has syslog.h you can end up including the host os one.

parkera · 2018-04-23T15:57:00Z

Foundation/Bundle.swift

-
+#if os(Android)
+    public convenience init(for aClass: AnyClass) {
+        self.init(path: Bundle.main.bundlePath)!


Sorry but I don't think this is the right way to fix this. I don't want to get stuck in a situation where anybody on Android can pass any value to this and always get the main bundle. We need to think about future compatibility.

What class is Alamofire passing in here?

The line of code is here: https://github.com/SwiftJava/Alamofire/blob/master/Source/SessionManager.swift#L121. It’s not very important what the bundle is in this case as it is just used to get the library version for request headers but this and perhaps other code needs the function for compatibility. Currently main bundle is the only bundle we could return (which isn’t really a bundle anyway) for now.

If that's building with SPM, that line of code is a bug in the third-party library — Alamofire is assuming it is in its own (framework) bundle with its own Info.plist, which is not a tenable assumption outside of the supported build paths except on Darwin.

The goal is compatibility. If at all possible OSS code should run unmodified.

I think we need to get a bit of perspective here. The code currently executes NSUnimplemented() so the contract is long broken. This is a pragmatic Android-specific step up in functionality that returns “a bundle” if this method happens to be called rather than crashing the app. Android binaries are monolithic so there is really only a main bundle.

Alamofire is relying on an assumption that isn't true on all platforms, and its OSS nature makes it trivial to propose a patch upstream. That's up to Alamofire to fix, not to Foundation to work around, and I think it's safe to say the core team is in broad agreement here.

parkera · 2018-04-23T16:18:26Z

TestFoundation/TestNSNumber.swift

@@ -1087,33 +1087,33 @@ class TestNSNumber : XCTestCase {
            XCTAssertEqual(NSNumber(value: UInt.min).stringValue, "0")
            XCTAssertEqual(NSNumber(value: UInt.min + 1).stringValue, "1")
            XCTAssertEqual(NSNumber(value: UInt.max).stringValue, "4294967295")
-            XCTAssertEqual(NSNumber(value: UInt.max - 1).stringValue, "4294967294")
+            XCTAssertEqual(NSNumber(value: UInt.max - 1 as UInt).stringValue, "4294967294")


I think we still need @spevans' suggested comment here

parkera · 2018-04-23T16:18:54Z

Looking pretty good, just a few minor things and we can get this merged.

millenomi · 2018-04-18T13:35:58Z

CoreFoundation/Base.subproj/CFBase.h

@@ -448,6 +448,12 @@ CF_EXPORT double kCFCoreFoundationVersionNumber;
 #define kCFCoreFoundationVersionNumber_iOS_9_x_Max 1299
 #endif

+#ifdef __ANDROID__
+typedef uint16_t _swift_rc_type;


Can you figure out why the compiler is laying out things differently on Android?

millenomi · 2018-04-23T21:14:13Z

Foundation/Bundle.swift

@@ -53,8 +53,16 @@ open class Bundle: NSObject {
        self.init(path: url.path)
    }

-    public init(for aClass: AnyClass) { NSUnimplemented() }
-
+#if os(Android)


It is still not correct to alter the semantics of this method just because of a third-party library. If that library is using Bundle(for:) on any non-Darwin platform, it is simply not implemented, and should for now (until we get to it) #if !canImport(Darwin) to fetch Bundle.main instead.

It is Foundation's job to fix its nonworking APIs, but it is generally speaking not Foundation's job to go against its own stated contract for the sake of a single downstream client, even a common one.

millenomi · 2018-04-23T21:15:23Z

Foundation/Bundle.swift

-
+#if os(Android)
+    public convenience init(for aClass: AnyClass) {
+        self.init(path: Bundle.main.bundlePath)!


If that's building with SPM, that line of code is a bug in the third-party library — Alamofire is assuming it is in its own (framework) bundle with its own Info.plist, which is not a tenable assumption outside of the supported build paths except on Darwin.

millenomi · 2018-04-23T21:17:26Z

CoreFoundation/PlugIn.subproj/CFBundle.c

@@ -714,7 +714,7 @@ static CFBundleRef _CFBundleCreate(CFAllocatorRef allocator, CFURLRef bundleURL,

    bundle->_url = newURL;

-#if !DEPLOYMENT_RUNTIME_OBJC && !DEPLOYMENT_TARGET_WINDOWS
+#if !DEPLOYMENT_RUNTIME_OBJC && !DEPLOYMENT_TARGET_WINDOWS && !DEPLOYMENT_TARGET_ANDROID


The previous comment on finding the other FHS declarations and disabling them still applies, I believe. It will break CFBundle if not so.

I’ve gone through the code again and gated anything I could find about FHS out for Android.

There seems to be a bug in github where I can’t reply directly to your next comment about path(toResource:ofType:) so I’ll add a note here. One of the goals of the Android port is that code for iOS should run on Android and it’s likely that code will use the path(toResource:ofType:) pattern somewhere. On Android It’s up to the Java side of the app to copy resources required into the cache/tmp directory then the Swift/Foundation side can find these resources inside this directory as if it was a bundle so the code runs unmodified.

I appreciate the use case, but again, it breaks our contract that Bundle.main be where the resources for a specific bundle actually are (not potentially will be if the author is careful), and that this be a directory that's either specific to the application or at worst contains existing application executable and resource data for that one application.

I outlined a workaround above — since apps need to write custom code to copy resources to some filesystem directory, it is no stretch, if they want Bundle API to fetch their own resources, to ask that they construct a Bundle instance to the copied data.

I will also reiterate that any library code using Bundle(for:) that builds with the Swift Package Manager and then asserts the returned bundle is exclusive to that library is essentially in undefined behavior territory, and should be fixed upstream.

(Also — that breaks if multiple components on Android are using Swift, have equally-named resources or resource directories {which isn't that unlikely: I assume careless copying code could easily clobber an en.lproj folder}, and are running at the same time.)

Perhaps there just is a misunderstanding here. This is not a system shared directory. It is the app's home, combination Documents and tmp directory on iOS with a path something like: /data/data/com.example.user.myapplication/cache and is the only directory an app has read write access to I am aware of. There's no candidate for a bundle directory other than the approach I suggest of copying resources into this directory on Android as resources are unpacked from their .apk (zip file) on demand. This is what the zip file for an Android app looks like:

Users-iMac:swift-corelibs-foundation user$ unzip -l ~/swift-android-kotlin/app/build/outputs/apk/app-debug.apk Archive: /Users/user/swift-android-kotlin/app/build/outputs/apk/app-debug.apk Length Date Time Name --------- ---------- ----- ---- 2788 00-00-1980 00:00 AndroidManifest.xml 1211 00-00-1980 00:00 META-INF/CERT.RSA 31429 00-00-1980 00:00 META-INF/CERT.SF 31367 00-00-1980 00:00 META-INF/MANIFEST.MF 246 01-01-1970 01:00 META-INF/kotlin-runtime.kotlin_module 43 01-01-1970 01:00 META-INF/kotlin-stdlib-jre7.kotlin_module 2090 01-01-1970 01:00 META-INF/kotlin-stdlib.kotlin_module 46044 00-00-1980 00:00 classes.dex 752 00-00-1980 00:00 classes2.dex 926 01-01-1970 01:00 kotlin/annotation/annotation.kotlin_builtins 3689 01-01-1970 01:00 kotlin/collections/collections.kotlin_builtins 726 01-01-1970 01:00 kotlin/internal/internal.kotlin_builtins 14202 01-01-1970 01:00 kotlin/kotlin.kotlin_builtins 2296 01-01-1970 01:00 kotlin/ranges/ranges.kotlin_builtins 4866 01-01-1970 01:00 kotlin/reflect/reflect.kotlin_builtins 10933048 00-00-1980 00:00 lib/armeabi-v7a/libFoundation.so 359812 00-00-1980 00:00 lib/armeabi-v7a/libXCTest.so 661056 00-00-1980 00:00 lib/armeabi-v7a/libc++_shared.so 2009280 00-00-1980 00:00 lib/armeabi-v7a/libcurl.so 668356 00-00-1980 00:00 lib/armeabi-v7a/libdispatch.so 25911876 00-00-1980 00:00 lib/armeabi-v7a/libscudata.so 2045828 00-00-1980 00:00 lib/armeabi-v7a/libscui18n.so 1418788 00-00-1980 00:00 lib/armeabi-v7a/libscuuc.so 1079184 00-00-1980 00:00 lib/armeabi-v7a/libsqlite3.so 5259060 00-00-1980 00:00 lib/armeabi-v7a/libswiftCore.so 31020 00-00-1980 00:00 lib/armeabi-v7a/libswiftGlibc.so 506816 00-00-1980 00:00 lib/armeabi-v7a/libswiftRemoteMirror.so 383140 00-00-1980 00:00 lib/armeabi-v7a/libswiftSwiftOnoneSupport.so 2810068 00-00-1980 00:00 lib/armeabi-v7a/libswifthello.so 1783196 00-00-1980 00:00 lib/armeabi-v7a/libxml2.so 620 00-00-1980 00:00 res/anim-v21/design_bottom_sheet_slide_in.xml 620 00-00-1980 00:00 res/anim-v21/design_bottom_sheet_slide_out.xml ...

Should I mark the whole Bundle api unavailable? There is also no prospect of a OSS fix upstream to Alamofire as the code I’m using is a fork of a fork. If it were me I would make the fix I suggest apply to all Linux systems in the interests of compatibility. If I were to fix Alamofire there just be some other library which falls at this hurdle.

But we’ve discussed these issues a few times and we’re going around in circles. What are the minimum changes I have proposed you would have me remove to get this PR merged? I’ll have to maintain a fork (undesirable as that may be) to get what I need to get done done.

The following changes are, I believe, at issue:

Bundle(for:) should not return an arbitrary bundle. It should stay callable. (My personal intent is to implement it on Linux as soon as resources allow.)

Bundle.main should not pick an arbitrary path whose forming substantially differs from other platforms. It should go on the existing fallback path unless that breaks existing well-formed code.

CFPreferences… should not assume that any CFBundle path is writable or a correct location for user data. All paths returned by CFBundle must be treated as read-only.

Also, someone should send Alamofire a patch, I believe. (For the reasons I outlined, Alamofire's code is not well-formed.)

OK.. how does it look now?

millenomi · 2018-04-23T21:18:55Z

CoreFoundation/PlugIn.subproj/CFBundle_Main.c

@@ -79,12 +79,20 @@ static CFBundleRef _CFBundleGetMainBundleAlreadyLocked(void) {
        CFStringRef str = NULL;
        CFURLRef executableURL = NULL, bundleURL = NULL;
        _initedMainBundle = true;
+#if DEPLOYMENT_TARGET_ANDROID
+        const char *bundlePath = getenv("CFFIXED_USER_HOME") ?: getenv("TMPDIR") ?: "/data/local/tmp";


If you're using path(toResource:ofType:) then you are also asserting you know where your resources are and how they're structured. This is not the case for the contents of a potentially system-shared temporary directory.

If clients are handling their own copying of resources, they should not be using Bundle APIs to fetch paths to them, unless they are copying an actually well-formed bundle somewhere and using Bundle(url:) or Bundle(path:).

millenomi · 2018-04-24T00:12:12Z

@swift-ci please test

johnno1962 · 2018-04-24T00:22:15Z

Thanks

millenomi · 2018-04-24T16:10:44Z

@swift-ci please test

johnno1962 · 2018-05-08T14:26:58Z

Hi @millenomi, is there anything more you need me to do to get this merged? It would be good to get this tidied away.

millenomi · 2018-05-09T00:23:53Z

Whoops, this fell off the radar while I was lost in the woods implementing bridging.

millenomi · 2018-05-09T00:24:29Z

@swift-ci please test and merge

johnno1962 · 2018-05-09T10:15:18Z

Thank @millenomi, Happy hunting with bridging.

millenomi suggested changes Apr 16, 2018

View reviewed changes

parkera reviewed Apr 16, 2018

View reviewed changes

johnno1962 force-pushed the master branch from 66d42da to ec4456e Compare April 18, 2018 08:07

johnno1962 mentioned this pull request Apr 18, 2018

Cater for new Swift reference count representation #1525

Merged

johnno1962 force-pushed the master branch 10 times, most recently from 07d8312 to f6e3910 Compare April 22, 2018 14:09

spevans reviewed Apr 22, 2018

View reviewed changes

johnno1962 force-pushed the master branch from f6e3910 to e254279 Compare April 22, 2018 15:19

parkera reviewed Apr 23, 2018

View reviewed changes

johnno1962 force-pushed the master branch from e254279 to 2582ebd Compare April 23, 2018 21:06

millenomi reviewed Apr 23, 2018

View reviewed changes

Changes required for swift-4.1-branch port to Android

25131e8

johnno1962 force-pushed the master branch from 2582ebd to 25131e8 Compare April 23, 2018 21:55

Remove contentious wokrarounds

df28468

millenomi approved these changes May 9, 2018

View reviewed changes

swift-ci merged commit 0e91936 into swiftlang:master May 9, 2018

NobodyNada mentioned this pull request Jun 9, 2018

[4.1.1] URLSession.dataTask causes "Segmentation fault" uraimo/buildSwiftOnARM#15

Closed

This was referenced Jul 12, 2018

Cater for new Swift reference count representation #1626

Merged

Android changes for Swift 4.1 cherrypicked into 4.2 #1631

Merged

Changes required for swift-4.1-branch port to Android #1518

Changes required for swift-4.1-branch port to Android #1518

Uh oh!

Conversation

johnno1962 commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

millenomi Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

johnno1962 Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnno1962 Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

johnno1962 Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

johnno1962 Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

millenomi Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnno1962 Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnno1962 Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnno1962 Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millenomi Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

johnno1962 commented Apr 16, 2018 •

edited

Loading

millenomi Apr 16, 2018 •

edited

Loading

johnno1962 Apr 17, 2018 •

edited

Loading

johnno1962 Apr 17, 2018 •

edited

Loading

johnno1962 Apr 17, 2018 •

edited

Loading

johnno1962 Apr 17, 2018 •

edited

Loading

millenomi Apr 16, 2018 •

edited

Loading

johnno1962 Apr 17, 2018 •

edited

Loading

johnno1962 Apr 17, 2018 •

edited

Loading

johnno1962 Apr 18, 2018 •

edited

Loading

millenomi Apr 16, 2018 •

edited

Loading

johnno1962 Apr 17, 2018 •

edited

Loading

codestergit commented Apr 18, 2018 •

edited

Loading