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

Swift package manager support #73

Merged
merged 10 commits into from
Jun 19, 2018
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ Pods
# Carthage
Carthage/Build

# Swift Package Manager
.build/

# AppCode specific files
.idea/
*.iml
Expand Down
16 changes: 10 additions & 6 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
language: objective-c # lol
osx_image: xcode9.2
language: objective-c
osx_image: xcode9.3
xcode_project: DVR.xcodeproj

script: xcodebuild -scheme "$TRAVIS_XCODE_SCHEME" -sdk "$TRAVIS_XCODE_SDK" -destination "$DESTINATION" test

matrix:
include:
- xcode_scheme: DVR-iOS
xcode_sdk: iphonesimulator
env:
- DESTINATION="OS=10.1,name=iPhone 7 Plus"
- DESTINATION="iOS Simulator,OS=11.3,name=iPhone X"
- xcode_scheme: DVR-macOS
xcode_sdk: macosx
env:
- DESTINATION="arch=x86_64"
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this need to change? I think we want to prefer attribute-based selection of destinations, rather than names (which Travis could change from underneath us at any time).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try reverting this change and see how it goes 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so it turns out that both the platform and the architecture must be specified. the final command (with env variables filled in) is:

xcodebuild -jobs `sysctl -n hw.ncpu` test -project DVR.xcodeproj -scheme DVR-macOS -sdk macosx \
  -destination "platform=macOS,arch=x86_64" ONLY_ACTIVE_ARCH=YES CODE_SIGNING_IDENTITY="" CODE_SIGNING_REQUIRED=NO | xcpretty -c

I'm hoping Apple doesn't change the operating system name that often :-D

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! I think "platform=macOS,arch=x86_64" is much better than OS X.

(especially when we need to do "platform=macOS,arch=arm64" down the line 😉 )

- DESTINATION="OS X"
- env:
- SWIFT_BUILD=true

script:
- ./Scripts/travis-build-test.sh

2 changes: 1 addition & 1 deletion DVR.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@ DVR.Session is a subclass of NSURLSession so you can use it as a drop in replace
s.ios.deployment_target = '8.0'
s.osx.deployment_target = '10.10'

s.source_files = 'DVR/*.{swift}'
s.source_files = 'Sources/DVR/*.{swift}'
end
217 changes: 116 additions & 101 deletions DVR.xcodeproj/project.pbxproj

Large diffs are not rendered by default.

8 changes: 3 additions & 5 deletions DVR.xcodeproj/xcshareddata/xcschemes/DVR-iOS.xcscheme
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<Scheme
LastUpgradeVersion = "0900"
LastUpgradeVersion = "0940"
version = "1.3">
<BuildAction
parallelizeBuildables = "YES"
Expand All @@ -26,9 +26,8 @@
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
language = ""
shouldUseLaunchSchemeArgsEnv = "YES"
codeCoverageEnabled = "YES">
codeCoverageEnabled = "YES"
shouldUseLaunchSchemeArgsEnv = "YES">
<Testables>
<TestableReference
skipped = "NO">
Expand Down Expand Up @@ -57,7 +56,6 @@
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
language = ""
launchStyle = "0"
useCustomWorkingDirectory = "NO"
ignoresPersistentStateOnLaunch = "NO"
Expand Down
4 changes: 1 addition & 3 deletions DVR.xcodeproj/xcshareddata/xcschemes/DVR-macOS.xcscheme
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<Scheme
LastUpgradeVersion = "0900"
LastUpgradeVersion = "0940"
version = "1.3">
<BuildAction
parallelizeBuildables = "YES"
Expand All @@ -26,7 +26,6 @@
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
language = ""
shouldUseLaunchSchemeArgsEnv = "YES">
<Testables>
<TestableReference
Expand Down Expand Up @@ -56,7 +55,6 @@
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
language = ""
launchStyle = "0"
useCustomWorkingDirectory = "NO"
ignoresPersistentStateOnLaunch = "NO"
Expand Down
4 changes: 1 addition & 3 deletions DVR.xcodeproj/xcshareddata/xcschemes/DVR-tvOS.xcscheme
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<Scheme
LastUpgradeVersion = "0900"
LastUpgradeVersion = "0940"
version = "1.3">
<BuildAction
parallelizeBuildables = "YES"
Expand All @@ -26,7 +26,6 @@
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
language = ""
shouldUseLaunchSchemeArgsEnv = "YES">
<Testables>
<TestableReference
Expand Down Expand Up @@ -56,7 +55,6 @@
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
language = ""
launchStyle = "0"
useCustomWorkingDirectory = "NO"
ignoresPersistentStateOnLaunch = "NO"
Expand Down
19 changes: 19 additions & 0 deletions Package.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// swift-tools-version:4.0

import PackageDescription

let package = Package(
name: "DVR",
products: [
.library(
name: "DVR",
targets: ["DVR"])
],
targets: [
.target(name: "DVR"),
.testTarget(
name: "DVRTests",
dependencies: ["DVR"])
]
)

1 change: 1 addition & 0 deletions Readme.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
[![Version](https://img.shields.io/github/release/venmo/DVR.svg)](https://github.com/venmo/DVR/releases)
![Swift Version](https://img.shields.io/badge/swift-4-orange.svg)
[![Carthage compatible](https://img.shields.io/badge/Carthage-compatible-4BC51D.svg?style=flat)](https://github.com/Carthage/Carthage)
[![Swift Package Manager](https://img.shields.io/badge/SPM-compatible-brightgreen.svg)](https://swift.org/package-manager/)

DVR is a simple Swift framework for making fake `NSURLSession` requests for iOS,
watchOS, and OS X based on [VCR](https://github.com/vcr/vcr).
Expand Down
14 changes: 14 additions & 0 deletions Scripts/travis-build-test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#!/bin/sh

set -x -o pipefail

if [[ "$SWIFT_BUILD" == "true" ]]; then
swift build
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only running build and not test because of what you mentioned in this comment, correct? #73 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes exactly 👌

exit 0
fi

# -jobs -- specify the number of concurrent jobs
# `sysctl -n hw.ncpu` -- fetch number of 'logical' cores in macOS machine
xcodebuild -jobs `sysctl -n hw.ncpu` test -project DVR.xcodeproj -scheme ${TRAVIS_XCODE_SCHEME} -sdk ${TRAVIS_XCODE_SDK} \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this script taken from the Travis macOS config script? Is there a way we could shell out to that instead (so that we don't need to duplicate the code)?

Copy link
Contributor Author

@loudmouth loudmouth Jun 18, 2018

Choose a reason for hiding this comment

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

Hey, I'm not sure I understand. This is not a duplicate exactly of the script that was inlined int eh travis.yml previously. It's standard xcodebuild arguments, some of which consume environment variables which are set by Travis.

Is there a way we could shell out to that instead?

The travis yml is calling this script, as opposed to inlining the xcodebuild command.

Am I understanding correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, my question was more around "is there a version of this command that comes in the Travis container that we can use instead of rewriting it ourselves?"

That is, we can supply these Travis-specific env vars in the .travis.yml, and Travis will generate an xcodebuild command for us. I believe there's a build.sh script that is on the machine that's running the build, but I think it might be too late by the time our build has started to use this.

I don't think this should be a major blocker, but if we can figure out how to use the Travis env vars to generate an xcodebuild command rather than rewriting the command ourselves, we can save ourselves some headaches in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, I can't seem to find any documentation about this in the Travis docs. We can skip over this for now, unless you want to do any more digging or reverse engineering 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I see @eliperkins . I'm not too sure to be honest. I just googled around for a bundled build.sh for mac builds on travis but didn't find anything myself (it was a quick google to fair).

I'm not sure how different it is to delegate to a bash script, as you could see before my change the xcodebuild command was inlined right under script: in the yml file. The benefit of doing things this way is that we can have more fine grained control (i.e. if SWIFT_BUILD is set, then avoid running xcodebuild) and add more complexity to our build/test script. Of course we could have also done this inline in the yml file, but then it looks cluttered. It's just an approach I've taken on a lot of projects with travis, but I'm happy to revert and inline everything if you prefer. Just let me know.

The build is passing again, and I reverted the one env variable for the platform specification.

Copy link
Contributor

Choose a reason for hiding this comment

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

All good with me! Let's keep this as it is in this PR.

My main concern here was duplicating the usage of the Travis-specific variables. If Travis is going to dictate the API for variables like TRAVIS_XCODE_SCHEME and such, it would be good to ensure that we use their build command rather than duplicate it ourself.

Given that we can't seem to find how they use these variables beyond what we see in the output, I think it's fine to just roll our own. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool. i can investigate more as well and try to de-dup in my follow-up which i'm interested in #80

always happy to improve CI setup

-destination "platform=${DESTINATION}" ONLY_ACTIVE_ARCH=YES CODE_SIGNING_IDENTITY="" CODE_SIGNING_REQUIRED=NO | xcpretty -c

2 changes: 1 addition & 1 deletion DVR/Cassette.swift → Sources/DVR/Cassette.swift
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ extension Cassette {
self.name = name

if let array = dictionary["interactions"] as? [[String: Any]] {
interactions = array.flatMap { Interaction(dictionary: $0) }
interactions = array.compactMap { Interaction(dictionary: $0) }
} else {
interactions = []
}
Expand Down
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import Foundation

final class SessionDownloadTask: URLSessionDownloadTask {

// MARK: - Types
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import Foundation

final class SessionUploadTask: URLSessionUploadTask {

// MARK: - Types
Expand Down
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,7 @@ class SessionUploadTests: XCTestCase {
}

private func writeDataToFile(_ data: Data, fileName: String) -> URL {
let documentsPath = NSSearchPathForDirectoriesInDomains(.documentDirectory, .userDomainMask, true)[0]
let documentsURL = URL(fileURLWithPath: documentsPath, isDirectory: true)
let documentsURL = try! FileManager.default.url(for: .documentDirectory, in: .userDomainMask, appropriateFor: nil, create: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should throw instead of crashing... Not a blocker for this PR!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i just figured since it's in the testing suite it didn't matter too much. if you think it's ok for now, i'll leave it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is totally fine! Either way, the test suite would crash anyway.

let url = documentsURL.appendingPathComponent(fileName + ".tmp")

try? data.write(to: url, options: [.atomic])
Expand Down