For testing Foundation on Android #1189
Conversation
From a cursory reading, the single file is simply a concatenated version of the standard files on other platforms; if reading this file directly from the NDK is impossible, then it is important to work out what the proper fallback should be. (See below.)
GMT being rewritten as GMT+0000 is certainly fine. If UTC and GMT are not treated as synonyms on all other platforms, then they should not be synonyms for Android. If, on the other hand, they are synonyms on all other platforms, then it should be fine here too--however, in that case, it should be handled in CFTimeZone.c uniformly for all platforms.
If CFTimeZone.c is where the guts of time zone processing are implemented, and if that's what's not implemented for Android, then clearly that's the correct place to patch if you're providing the corresponding functionality. If you're just logging an error, then clearly it's acceptable to throw that into the Swift initializer.
That's fine on reconsideration; this is a failable initializer and logging and returning nil pretty much upholds the semantic contract of the initializer.
This is a functionality change that shouldn't be part of an Android-specific patch. |
Foundation/NSTimeZone.swift
Outdated
| if localtime_r(&now, &info) != nil && | ||
| (tzName == "System" || tzName == String(cString: info.tm_zone)) { | ||
| (info.tm_gmtoff < 0 ? "-" : "+").withCString { | ||
| tzName = String(format: "GMT%s%02d%02d", |
There was a problem hiding this comment.
This is a valiant effort but very much incorrect. An NSTimeZone instance for a particular time zone will have different offsets from GMT depending on the date and time; it is not equivalent to a fixed GMT offset. When you don't have the information to instantiate the requested time zone, the correct result can only be nil.
|
I’ve pushed a new simpler version implementing “TimeZone.current” instead of a “System” timezone. It still has it’s limitations in that it simulates a proper timezone with a fixed GMT offset but it’s the best I can do until ICU is able to parse Android's /system/usr/share/zoneinfo/tzdata. I’d be quite reluctant to start making modifications to the very complex CFTimeZone.c and risk breaking other platforms. |
| var values = [ | ||
| TimeZone(identifier: "UTC")!, | ||
| TimeZone.current | ||
| ] |
There was a problem hiding this comment.
I guess the problem here is that Android doesn't come with a reasonable ICU?
There was a problem hiding this comment.
Indeed, options are very limited for Android date/time conversion from C
TestFoundation/TestDecimal.swift
Outdated
| XCTAssertEqual(Decimal(186243*15673), Decimal(186243) * Decimal(15673)) | ||
| #endif |
There was a problem hiding this comment.
Why is this not possible on Android?
There was a problem hiding this comment.
If it's a 32-bit vs 64-bit thing, can we express that differently so it works on other 32 bit platforms?
There was a problem hiding this comment.
Couldn't get it to compile for some reason even though there is Decimal.init(_ value: Int64)
There was a problem hiding this comment.
Error was:
/home/johnno/swifty-robot-environment/util/prepare_environment/test-foundation/Sources/TestDecimal.swift:280:38: error: arithmetic operation '186243 * 15673' (on type 'Int') results in an overflow
XCTAssertEqual(Decimal(186243*15673), Decimal(186243) * Decimal(15673))
There was a problem hiding this comment.
Was able to fix this: Decimal(Int64(186243)*Int64(15673)). I had a second look and can’t see how the other commenting out could be avoided.
There was a problem hiding this comment.
This is an incorrect use of these initializers. Int64(15673) creates a value of type Int from the literal and then converts it to a value of type Int64; indeed, consideration has been given to making this usage with literals a compiler warning. You want to write instead Decimal(186243 * 15673 as Int64).
There was a problem hiding this comment.
Thanks @xwu. Resolved.
|
If you’re interested, the only failures are now: |
TestFoundation/TestNSData.swift
Outdated
| @@ -103,7 +103,7 @@ class TestNSData: XCTestCase { | |||
|
|
|||
| func test_writeToURLOptions() { | |||
| let saveData = try! Data(contentsOf: Bundle.main.url(forResource: "Test", withExtension: "plist")!) | |||
| let savePath = URL(fileURLWithPath: "/var/tmp/Test.plist") | |||
| let savePath = URL(fileURLWithPath: NSTemporaryDirectory()+"Test1.plist") | |||
There was a problem hiding this comment.
Nit: Can you please put spaces before and after the operator? Also, why have you renamed the output file?
There was a problem hiding this comment.
Had to rename output file as bundle resources dir and tmp directory are the same the way the tests run on Android. Deleting Test.plist makes the next run of tests fail.
Foundation/NSTimeZone.swift
Outdated
| tzName = "GMT+0000" | ||
| } | ||
| else if !(tzName.hasPrefix("GMT+") || tzName.hasPrefix("GMT-")) { | ||
| NSLog("TimeZone database not available on Android") |
There was a problem hiding this comment.
Nit: it should be spelled "Time zone".
Foundation/NSTimeZone.swift
Outdated
| #if os(Android) | ||
| var now = time(nil), info = tm() | ||
| if localtime_r(&now, &info) != nil { | ||
| // NOTE: this is not a real timezone but a fixed offset from GMT. |
|
@johnno1962 there are a number of different changes here, including some purely stylistic ones. Can you separate them into individual PRs that can be merged independently? For example, your changes for |
|
Not sure I follow you. These are the minimum changes for tests to run on Android, the only stylistic changes I can see are changing XCTAssertTrue to XCTAssertEqual in a couple of places to aid debugging. I reverted the if os(Linux) || os(Android) changes in the finish as I use a compiler which treats Android as a synonym for Linux if you test for it so the number of changes is down from 70 odd files to 18. The only thing is perhaps out of place here is the NSTimeZone changes which I can separate out if you like. Let me rebase and squash and see if you’re happy with it. |
|
@johnno1962 yeah, maybe a squash will help show where we are with this :) |
|
OK, rebase should be good to go now. |
|
@swift-ci please test |
|
Generally speaking the changes look OK - there are several where the Android specific tests are stubbed out (looks like the threading and timezone naming ones), something to deal with the fact that Provided the tests pass I think this is probably good to go... |
|
@johnno1962 can you rebase and fix the conflicts please? |
|
Rebased, beginning to feel like we’ll not be able to keep this open much longer. |
|
@swift-ci please test |
|
Random typo fixed.. you’ll need to test again |
|
@swift-ci please test again |
|
@swift-ci please test |
|
Hmm. Failure is in libdispatch: |
|
@swift-ci please clean test |
|
The build errors aren’t related to this change - I’m trying to see why it’s happening. Once we’ve got it sorted out I’ll come back and rerun the tests here.
|
|
Is this anything to do with #1206? Can’t be the first time tests have been run since then. |
|
cc @shahmishal |
|
Same issue with #1258
Note that there was a successful merge build a few hours ago to master.
|
|
We are seeing this on master branch: https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-16_04-long-test/407/ This started after following commits: Git (apple/swift-llvm.git)
Git (apple/swift-clang.git)
Git (apple/swift-corelibs-foundation.git)
Git (apple/swift-compiler-rt.git)
|
|
The FileSystemManager change was merged and tested successfully a few hours before this started failing; maybe that will record what the state of the world was before then.
Alex
Sent from my iPhone 📱
… On 12 Oct 2017, at 01:30, Mishal Shah ***@***.***> wrote:
We are seeing this on master branch:
https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-16_04-long-test/407/
https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-16_10-long-test/408/
https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-14_04-long-test/1063/
This started after following commits:
Git (apple/swift-llvm.git)
Add section headers to SpecialCaseLists
[llvm-cov] Improve const-correctness of filters. NFC.
[llvm-cov] Create directory structure when filtering using -name*=
[llvm-cov] Hide files with no coverage from the index when filtering by
[llvm-cov] Fix showing title when filtering and not outputting to a
[ProfileData] Fix data racing in merging indexed profiles
Check for overflows when calculating the offset in GetGEPCost.
Convert an APInt to int64_t properly in TTI::getGEPCost().
[Dominators] Invalidate DFS numbers upon edge deletions
Don't move llvm.localescape outside the entry block in the GCOV
Git (apple/swift-clang.git)
[Driver] Disable static C++ library support on Fuchsia
Enable AddressSanitizer for Fuchsia targets
[Driver][Fuchsia] Pass --hash-style=gnu to the linker
Cleanup and generalize -shared-libasan.
[Driver] Fix -static-libsan / -shared-libsan on Darwin
Allow specifying sanitizers in blacklists
[Preprocessor] Preserve #pragma clang assume_nonnull in preprocessed
Fix templated type alias completion when using global completion cache
Use EmitPointerWithAlignment to get alignment information of the pointer
Git (apple/swift-corelibs-foundation.git)
Implemented FileManager.attributesOfFileSystem()
Fallback to statvfs in Linux for filesystem attributes
Fixed FileManager.attributesOfFileSystem tests
Fixed FileManager.attributesOfFileSystem tests (+1 squashed commit)
Git (apple/swift-compiler-rt.git)
[ubsan] Add a static runtime on Darwin
[ubsan] Fix Asan internal alloc corruption in PR33221 test.
cmake: Fix one more usage of append()
Use list(APPEND) instead of append()
[ubsan] Save binary name before parsing options
[ubsan] Make ubsan version of __sanitizer_print_stack_trace consistent
[compiler-rt] Cleanup decorators
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@shahmishal @gottesmm can you clarify which commit was reverted, so that we know what it was? |
|
This is the linux failures from yesterday? A clang commit was cherry-picked that broke lowering of atomic intrinsics. So we were emitting calls to the intrinsics instead of just using builtin llvm instructions. |
|
Thanks! |
|
@swift-ci please test and merge |
|
Tnanks @alblue 👍 👍, we’re on a roll! #1198 would be next (preferably before #1216 to minimise conflicts as the latter is quite a refactoring) then I can rebase and clean up #1113 and we’re done. Re: all the boilerplate importing the right foundation in these files I’d recommend a @_exported import something like the following in main.swift and all the other imports could be removed but that’s a PR for anther day: #if DEPLOYMENT_RUNTIME_OBJC || os(Linux)
@_exported import Foundation
@_exported import XCTest
#else
@_exported import SwiftFoundation
@_exported import SwiftXCTest
#endif |
Changes I had to make to be able to test Foundation on Android. Results are good considering the differrences of the platform to vanilla Linux:
There seems to be some problem with TestNSAttributedString.test_enumerateAttributes. An invalid dictionary is returned from CFAttributedStringGetAttributesAndLongestEffectiveRange. I couldn’t puzzle out more than that. I’ve also disabled TestURLSession.test_cancelTask as it seems to result in intermmittent crashes though I’m running Foundation from a week ago and I see related commits. Other changes (to TestNSNumber.swift for example) might be useful for other 32 bit platforms.
Thanks!