Add watchOS, tvOS, iOS platforms support #176

larryonoff · 2016-11-25T11:23:40Z

Motivation

CoreLibs XCTest only supports desktop platforms. What limits us using CoreLibs XCTest on other platforms like iOS / Android. Xcode provides functionality to use XCTest framework on iOS, but this's Objective-C framework that is tightly coupled to Xcode. I see two main goals of this PR:

Start making CoreLibs XCTest framework available for other platforms (not only for desktops).
Start getting rid of XCTest framework coupled with Xcode and make CoreLibs XCTest as a basic framework for testing. I understand that this is a big deal. So this's only beginning of work.

PS. I guess this PR strongly relates to #61 because currently tests only run for macOS platform.

TODO

tests
Android?
define a way to run tests on mobile devices
what else ?

modocache · 2016-11-26T22:32:04Z

Sources/XCTest/Private/PerformanceMeter.swift

@@ -11,7 +11,7 @@
 //  Measures the performance of a block of code and reports the results.
 //

-#if os(Linux) || os(FreeBSD)
+#if os(Linux) || os(FreeBSD) || os(iOS)


As we discussed on Twitter, I think the way forward here isn't to hard-code Linux, FreeBSD, and now iOS to import Foundation. Instead, we should provide a configuration option of some kind, to compile swift-corelibs-xctest to use Foundation or SwiftFoundation, as the user desires.

To put it another way, I'm envisioning something like this:

#if defined(USE_SWIFT_FOUNDATION_XCODEPROJ) import SwiftFoundation #else import Foundation #endif

USE_SWIFT_FOUNDATION_XCODEPROJ would be specified by the user when they compile swift-corelibs-xctest, or as part of the SwiftXCTest-iOS scheme.

I think it's better assume that SwiftFoundation is default. So I suggest introducing flag with name like USE_OBJC_FOUNDATION.

Sounds good to me! @briancroom, any thoughts?

@modocache @briancroom PR was updated.

modocache · 2016-11-26T22:33:03Z

Sources/XCTest/Public/Asynchronous/XCTestCase+NotificationExpectation.swift

@@ -45,8 +45,7 @@ public extension XCTestCase {
            .default
            .addObserver(forName: Notification.Name(rawValue: notificationName),
                         object: objectToObserve,
-                         queue: nil,
-                         usingBlock: {
+                         queue: nil) {


I think this is a good change, thanks! But please submit these syntax cleanup changes as a separate pull request.

Added this change in a separate PR, see #177. I rebased this PR on PR with the syntax cleanup. In other case iOS build fails. I'll rebase on master as soon as the syntax cleanup approved.

larryonoff · 2016-11-27T12:49:01Z

@modocache thank you for the review. I'll do changes on Monday.

modocache

I kicked off the tests for #177 -- thanks!

modocache · 2016-11-28T14:44:17Z

Sources/XCTest/Private/PerformanceMeter.swift

@@ -11,7 +11,7 @@
 //  Measures the performance of a block of code and reports the results.
 //

-#if os(Linux) || os(FreeBSD)
+#if USE_OBJC_FOUNDATION || os(Linux) || os(FreeBSD)


OK, this is very close to what I had in mind. My only suggestion would be to remove the || os(Linux) || os(FreeBSD) here. Instead, have the build script, build_script.py, define USE_OBJC_FOUNDATION=1 when compiling for a non-Darwin target.

Of course, it's strange that USE_OBJC_FOUNDATION should be defined when building for Linux, so I'd change the name here. Maybe USE_IMPORT_FOUNDATION or USE_IMPORT_SWIFTFOUNDATION.

I thought ObjC Foundation exist for Linux. So just curious to introduce better name what kind of foundation we use for Linux / FreeBSD.

Done.

BTW I forgot to add USE_IMPORT_FOUNDATION for tests, so now it's fixed.

PS. I still not sure that USE_IMPORT_FOUNDATION is a good name, but I don't have any better ideas.

modocache

Excellent, thanks @larryonoff!

I like these changes, but I'll let @briancroom chime in as well before merging.

modocache · 2016-11-28T15:50:33Z

@swift-ci please test

parkera · 2016-11-28T17:05:46Z

If you want to do this for XCTest, that's fine - but I do want to point out that we have no plans to make swift-corelibs-foundation a shippable product on Darwin platforms. Our plan all along there has been to continue to use the system provided one.

larryonoff · 2016-11-28T17:21:22Z

CI just pointed out that I forgot update build script for Unix. Just did it, could anyone please check that I did it properly.

modocache · 2016-11-28T20:48:00Z

@swift-ci Please test

larryonoff · 2016-11-29T09:35:09Z

Could anyone please trigger CI? I made changes that should fix CI tests.

modocache · 2016-11-29T15:03:50Z

@swift-ci Please test

larryonoff · 2016-11-29T16:40:22Z

Since I don't have Linux env I only had to check if the changes for Linux are valid with CI help. Sorry, but I need to trigger ci build again :)

modocache · 2016-11-29T18:00:29Z

No problem! :)

modocache · 2016-11-29T18:30:49Z

@swift-ci please test

larryonoff · 2016-11-30T09:10:20Z

build failed on macOS since llvm clone failed. could you please trigger CI again :)

modocache · 2016-12-01T02:39:31Z

@swift-ci Please test

CodaFi · 2016-12-01T06:32:49Z

This is going to keep failing until swiftlang/swift#5890 gets merged.

briancroom

Hey @larryonoff, I'm so sorry for not getting to this sooner. Thanks for taking the time to look into this! I've left a couple of comments with suggestions that should simplify the required changes a bit.

I'd also like to mention, though, that while I happy to pull in these few changes in support of allowing the framework to build on iOS, I would by no means consider iOS to be a properly supported platform for Corelibs XCTest. See #34 for some previous discussion of this point.

Xcode's XCTest.framework has much more in the way of platform integration and should provide a better experience when working on any Apple platform. If this isn't the case, please file Radars indicating what you are trying to accomplish and how the framework isn't pulling its weight.

briancroom · 2016-12-01T08:03:41Z

Sources/XCTest/Private/PerformanceMeter.swift

@@ -11,7 +11,7 @@
 //  Measures the performance of a block of code and reports the results.
 //

-#if os(Linux) || os(FreeBSD)
+#if USE_IMPORT_FOUNDATION


Instead of adding this conditional compilation flag, how about we simplify the expression to something like:

#if os(macOS) import SwiftFoundation #else import Foundation #endif

See also previous discussion of this: #96

I like the idea of removing USE_IMPORT_FOUNDATION define (I generally don't like the flag). But in the case we excluding the case when we want to run both tests for SwiftFoundation and Foundation of SwiftXCTests on macOS.

@modocache @briancroom any thoughts about this?

Corelibs XCTest is really intended to build on top of Corelibs Foundation. If it works with Darwin Foundation, that's great, but it's not a core configuration for the project. I don't think we should go out of our way to add support for that as a part of this PR, at least.

I got your point. I'll do change soon.

These look great 👍

briancroom · 2016-12-01T08:11:53Z

XCTest.xcodeproj/project.pbxproj

@@ -255,6 +299,24 @@
 /* End PBXLegacyTarget section */

 /* Begin PBXNativeTarget section */
+		19D66F931DE6F57E00C112C0 /* SwiftXCTest-iOS */ = {


Rather than adding a whole new target, it should be pretty straightforward to accomplish the same goal by adding iphoneos and iphonesimulator to the SUPPORTED_PLATFORMS build setting of the existing target. You'll probably need to also bring over a couple of additional settings like the iOS deployment target, but I would it expect it to mostly just work for a simple target like this.

will this work for with SPM?

I'm not sure I understand what you're asking here. Could you elaborate?

I meant if we leave one unified target (for macOS, iOS) will Swift Package Manage be able compiling it for macOS and iOS platforms separately? e.g. I think that Carthage won't be able doing it (but I'm not sure).

Well, SwiftPM doesn't build from Xcode projects, so there's be no effect there. (It would be interesting to see sometime though if we could do away with this Xcode project altogether and just let SwiftPM generate it for us!)

I'm not sure about Carthage, though I would consider it to be their bug if it doesn't work, since xcodebuild deals with that setup just fine.

I'm not sure about Carthage, though I would consider it to be their bug if it doesn't work, since xcodebuild deals with that setup just fine.

Carthage is supporting such unified schemes since September 2015.

@ikesyo thanks for pointing it out!

I've migrated into unified scheme. Generally I checked https://github.com/mxcl/PromiseKit settings to do this.

The main thing that can be wrong is that I had to remove SwiftFoundation.framework from "Linked Frameworks and Libraries" since it doesn't compile for iOS platform. I checked that there's a setting "Link Frameworks Automatically", but I'm not sure that it works properly.

Could anyone please provide any suggestions?

Sorry, I missed this before. I think what we're going to need here is to add -framework SwiftFoundation into the Other Linker Flags build setting, but conditionalized such that it only applies to Any macOS SDK. Similar to what is described here.

larryonoff · 2016-12-01T11:56:33Z

@CodaFi thank you for mentioning. It looks that issue introduced recently. Since I was able passing macOS build some time before.

modocache · 2017-01-06T16:23:35Z

@larryonoff A good next step would be to look at the CI log, and run the exact same invocation locally. Swift CI usually builds a larger portion of all the Swift projects, and sometimes with unique settings.

modocache · 2017-01-06T16:23:42Z

@swift-ci please test

ikesyo · 2017-01-11T10:05:35Z

dyld: lazy symbol binding failed: Symbol not found: __os_log_set_nscf_formatter
  Referenced from: /Users/buildnode/jenkins/workspace/swift-corelibs-xctest-PR-osx/Ninja-DebugAssert/xctest-macosx-x86_64/Debug/SwiftFoundation.framework/Versions/A/SwiftFoundation
  Expected in: /usr/lib/libSystem.B.dylib

dyld: Symbol not found: __os_log_set_nscf_formatter
  Referenced from: /Users/buildnode/jenkins/workspace/swift-corelibs-xctest-PR-osx/Ninja-DebugAssert/xctest-macosx-x86_64/Debug/SwiftFoundation.framework/Versions/A/SwiftFoundation
  Expected in: /usr/lib/libSystem.B.dylib

It seems that by swiftlang/swift-corelibs-foundation#709, Swift Foundation does not work well on macOS 10.11 now. What is the version of the CI environment?

modocache · 2017-01-11T14:24:21Z

I can see that swift-corelibs-foundation only runs tests on Linux, and its build log prints out the version Ubuntu 16.04. Unfortunately the macOS build doesn't seem to print out the current macOS version. I can see that it's using the MacOSX10.12.sdk, but that doesn't mean much.

@shahmishal, so you know which version of macOS the CI is running? Would you merge a pull request to print out the macOS version in build-script?

ikesyo · 2017-01-11T15:33:45Z

I've submitted a pull request: swiftlang/swift-corelibs-foundation#784

larryonoff · 2017-01-11T18:10:56Z

@modocache PR swiftlang/swift-corelibs-foundation#784 merged. Could you please trigger CI?

modocache · 2017-01-11T19:31:57Z

@swift-ci please test

shahmishal · 2017-01-11T19:42:51Z

@modocache

ProductName:	Mac OS X
ProductVersion:	10.11.5
BuildVersion:	15F34

ikesyo · 2017-01-12T00:02:13Z

Another Foundation pull request related to the failure: swiftlang/swift-corelibs-foundation#785.

briancroom · 2017-01-12T05:09:52Z

Sorry guys, I was on holiday for a bit and my notification settings were messed up so I missed the activity and lost track here. I'm going to take a moment to catch up now.

briancroom · 2017-01-12T05:25:00Z

XCTest.xcodeproj/project.pbxproj

@@ -409,13 +407,22 @@
 				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
 				GCC_WARN_UNUSED_FUNCTION = YES;
 				GCC_WARN_UNUSED_VARIABLE = YES;
-				MACOSX_DEPLOYMENT_TARGET = 10.9;
+				INFOPLIST_FILE = Info.plist;
+				IPHONEOS_DEPLOYMENT_TARGET = 10.0;


I don't any specific concerns with these deployment targets, but I'm curious how you picked them? Is there a reason why it couldn't deploy further back?

There're two reasons, see them below:

swift-corelibs-xctest uses Timer.scheduledTimer(withTimeInterval:repeats:block:) that requires iOS 10.0+, macOS 10.12+, tvOS 10.0+, watchOS 3.0+. The link to the documentation.

SwiftFoundation macOS deployment target is 10.11.

PS. I think that at the moment we don't need deployment targets lower that specified. We can decrease deployment target as soon as people need it, but in this case we need to make some changes in the code.

Makes sense! Thanks for the explanation.

briancroom · 2017-01-12T05:32:36Z

XCTest.xcodeproj/project.pbxproj

 				MTL_ENABLE_DEBUG_INFO = YES;
 				ONLY_ACTIVE_ARCH = YES;
-				SDKROOT = macosx;


These SDKROOT changes are surprising to me. Can you explain what's going on here? This corresponds to the Base SDK setting in the Xcode UI.

Very good question!

Please note that most of the changes I made are based on PromiseKit and the article.

We also should note that we're making the multi-platform single-scheme framework now, so each platform needs it's own SDKROOT.

I tried the same setup for SDKROOT as for PromiseKit (just by removing SDKROOT on project level) and it works for swift-corelibs-xctest. But I had to add to specify SDKROOT for SwiftXCTestFunctionalTests target, because without SDKROOT it failed to build.

PS. I agree that it would be better having SDKROOT on project level for each platform, but Xcode only allows configuring it for architecture (I tried doing it via Xcode UI), not for platform.

It would be awesome if you provide any better solution. Thanks!

In my testing, Xcode will automatically adjust the SDKROOT used for a build such that it matches the platform of the run destination. For example, if the selected SDK is "Latest macOS" and the build has an iOS device destination selected, it will implicitly use the "Latest iOS" SDK instead. Therefore I would prefer keeping the Base SDK setting at the project level with no target overrides.

One other minor point: I think it would be better to specify the new SUPPORTED_PLATFORMS value at the level of the SwiftXCTest target, rather than at the project level, for now anyway. This is because the SwiftXCTestFunctionalTests target only works on macOS, and I don't foresee that changing.

Thanks for the comments. I did the following updates:

move SUPPORTED_PLATFORMS from project level to 'XCTest' target.

move deployment targets from project level to 'XCTest' target. Because there're relevant only for 'XCTest' target.

revert SDKROOT to macosx for project level.

briancroom · 2017-01-12T05:38:16Z

I left a couple of minor questions, but this looks good to me, @larryonoff! Thanks for your work here.

I would like to wait to merge until we can get CI green on OS X, though. Thanks @ikesyo for helping to deal with the Foundation build failures we're seeing here. I'm hopeful we can get through this soon!

'SwiftXCTest' is multi-platform single-scheme target now. Note that 'SwiftXCTestFunctionalTests' still only works on macOS and isn't planned to support other platforms.

- move SUPPORTED_PLATFORMS from project level to 'XCTest' target - move deployment targets from project level to 'XCTest' target - revert SDKROOT to macosx for project level

larryonoff · 2017-01-13T09:55:59Z

Could anyone please trigger CI again. It should still fail since swiftlang/swift-corelibs-foundation#785 not merged. But I would like to see that nothing is broken since my current changes.

modocache · 2017-01-13T13:59:43Z

@swift-ci please test

briancroom · 2017-01-13T16:37:46Z

Unfortunately the CI build won't even reach XCTest if Foundation is failing to build, so there's not any additional feedback to be gained from running the build again, AFAICT.

I'm not sure why CI seeks to have ignored your request though, @modocache!

larryonoff · 2017-01-17T09:18:10Z

Please trigger CI, since PR swiftlang/swift-corelibs-foundation#785 merged.

rintaro · 2017-01-17T09:29:25Z

@swift-ci Please test

larryonoff · 2017-01-17T13:13:33Z

All CI checks finally passed! Thanks all.

modocache · 2017-01-17T17:30:24Z

🎉 🎉 🎉

Thanks for all the persistence, everyone! :)

briancroom · 2017-01-17T20:03:52Z

Excellent!

modocache requested changes Nov 26, 2016

View reviewed changes

modocache assigned modocache and briancroom Nov 26, 2016

larryonoff force-pushed the ios-target branch 3 times, most recently from cc39485 to 98aae59 Compare November 28, 2016 10:31

modocache requested changes Nov 28, 2016

View reviewed changes

larryonoff force-pushed the ios-target branch from 98aae59 to b7f2080 Compare November 28, 2016 15:15

modocache approved these changes Nov 28, 2016

View reviewed changes

modocache removed their assignment Nov 28, 2016

larryonoff force-pushed the ios-target branch from b7f2080 to 6393dea Compare November 28, 2016 17:19

larryonoff force-pushed the ios-target branch from 6393dea to 03e2074 Compare November 29, 2016 09:30

larryonoff force-pushed the ios-target branch from 03e2074 to 344b1bb Compare November 29, 2016 15:49

larryonoff force-pushed the ios-target branch from 344b1bb to 14cee72 Compare November 30, 2016 09:05

briancroom requested changes Dec 1, 2016

View reviewed changes

larryonoff force-pushed the ios-target branch from 14cee72 to 8d1e857 Compare December 1, 2016 18:53

larryonoff mentioned this pull request Jan 11, 2017

remove trailing period after "N seconds" #182

Merged

ikesyo mentioned this pull request Jan 11, 2017

Delete _os_log_set_nscf_formatter related lines to make the framework run on macOS 10.11 swiftlang/swift-corelibs-foundation#784

Merged

briancroom reviewed Jan 12, 2017

View reviewed changes

briancroom approved these changes Jan 12, 2017

View reviewed changes

larryonoff added 2 commits January 13, 2017 12:52

Add iOS, tvOS, watchOS platforms support

7bb7faa

'SwiftXCTest' is multi-platform single-scheme target now. Note that 'SwiftXCTestFunctionalTests' still only works on macOS and isn't planned to support other platforms.

Update Xcode project settings

cb30809

- move SUPPORTED_PLATFORMS from project level to 'XCTest' target - move deployment targets from project level to 'XCTest' target - revert SDKROOT to macosx for project level

larryonoff force-pushed the ios-target branch from 2518bfe to cb30809 Compare January 13, 2017 09:52

modocache merged commit 1f20d9b into swiftlang:master Jan 17, 2017

larryonoff mentioned this pull request Jan 20, 2017

[54] Issue #54 - January 26, 2017 SwiftWeekly/swiftweekly.github.io#167

Closed

larryonoff deleted the ios-target branch February 1, 2017 15:01

Add watchOS, tvOS, iOS platforms support #176

Add watchOS, tvOS, iOS platforms support #176

Uh oh!

Conversation

larryonoff commented Nov 25, 2016

Motivation

TODO

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!

larryonoff commented Nov 27, 2016

Uh oh!

modocache 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!

larryonoff Nov 28, 2016 • 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!

modocache left a comment • 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!

modocache commented Nov 28, 2016

Uh oh!

parkera commented Nov 28, 2016

Uh oh!

larryonoff commented Nov 28, 2016

Uh oh!

modocache commented Nov 28, 2016

Uh oh!

larryonoff commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

modocache commented Nov 29, 2016

Uh oh!

larryonoff commented Nov 29, 2016

Uh oh!

modocache commented Nov 29, 2016

Uh oh!

modocache commented Nov 29, 2016

Uh oh!

larryonoff commented Nov 30, 2016

Uh oh!

modocache commented Dec 1, 2016

Uh oh!

CodaFi commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

briancroom 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!

larryonoff Nov 28, 2016 •

edited

Loading

modocache left a comment •

edited

Loading

larryonoff commented Nov 29, 2016 •

edited

Loading

CodaFi commented Dec 1, 2016 •

edited

Loading

larryonoff Dec 1, 2016 •

edited

Loading

ikesyo Dec 1, 2016 •

edited

Loading

modocache commented Jan 6, 2017 •

edited

Loading