Swift package manager support #73

loudmouth · 2017-09-29T09:56:24Z

This PR restructures the repo's directories to enable Swift Package Manager.

It also adds a build job to the travis matrix that ensures swift build is working. Unfortunately, SPM does not enable including other resources (i.e. bundled content) like Xcode so therefore swift test fails as cassettes cannot be found. Once support for bundling other resources with swift build, then we should update the travis script to run swift test.

Lastly, this fixes the travis builds that were broken on master.

Addresses #67

loudmouth · 2017-09-29T11:17:52Z

Only the iOS build has failed because of the infamous exit code 65 which is a known Xcode bug. Can somebody on the venmo org please restart the build? It should pass.

kiliankoe · 2017-09-29T13:41:22Z

Readme.markdown

-[![Version](https://img.shields.io/github/release/venmo/DVR.svg)](https://github.com/venmo/DVR/releases) [![Carthage compatible](https://img.shields.io/badge/Carthage-compatible-4BC51D.svg?style=flat)](https://github.com/Carthage/Carthage)
+[![Version](https://img.shields.io/github/release/venmo/DVR.svg)](https://github.com/venmo/DVR/releases)
+[![Carthage compatible](https://img.shields.io/badge/Carthage-compatible-4BC51D.svg?style=flat)](https://github.com/Carthage/Carthage)
+[![Swift Package Manager](https://rawgit.com/jlyonsmith/artwork/master/SwiftPackageManager/swiftpackagemanager-compatible.svg)](https://swift.org/package-manager/)


This is probably the most minor of minor nitpicks ever, but the design of those two badges doesn't match up. How about using shields.io's custom badges for spm compatibility? 😊 E.g. https://img.shields.io/badge/SPM-compatible-brightgreen.svg

Nitpick all you want @kiliankoe ;-)

Pushed with the badge you suggested. Thanks for the quick review!

loudmouth · 2017-10-06T17:48:09Z

Anything else that needs to be changed before merging?

eliperkins

This seems good to me! Thanks for looking into this.

Would you mind rebasing onto master to pickup the latest CI fixes?

loudmouth · 2017-12-22T15:46:09Z

Hey @eliperkins

I've just rebased

loudmouth · 2017-12-22T15:47:16Z

Shoot...have an issue the iOS build in Travis...will have to fix when i'm back at a computer.

loudmouth · 2017-12-22T17:42:11Z

@eliperkins it looks like the error code on the iOS build is the infamous error code 65 and that job
of the build should just be restarted and it will probably pass.

loudmouth · 2018-02-14T08:29:12Z

@eliperkins just thought I'd check in on the status of this PR.

Hope you're doing well.

dmiluski · 2018-02-14T23:48:41Z

Hi @loudmouth, could you rebase once more? There appears to be an Interaction.swift conflict

Add swift build to the script

loudmouth · 2018-06-12T17:00:08Z

Hey @dmiluski and @eliperkins

I'm really sorry it took me so long to get back to this—it's good to go now!

I had to update some code related to creating the test file for the upload test to pass on iOS, but everything is now green ✅

Any chance we can give this a merge and call it a day?

loudmouth · 2018-06-13T07:33:20Z

Just a note that we'll still need these two (duplicate) tickets in Swift Package Manager to be addressed for DVR to work with the swift build system on the command line:

SR-2866
SR-4725

Let's upvote there!

loudmouth · 2018-06-18T09:02:18Z

@eliperkins @kiliankoe @dmiluski

I hope I don't become annoying in doing this, but i'd love to push forward this PR and then continue onto pushing for #80 after I rebase there.

This project is awesome and I use it to test the official Contentful Swift SDK, just wanna help keep it up-to-date and working for us all :-)

eliperkins

LGTM, a few comments, but no blockers really.

eliperkins · 2018-06-18T14:42:12Z

Scripts/travis-build-test.sh

+set -x -o pipefail
+
+if [[ "$SWIFT_BUILD" == "true" ]]; then
+    swift build


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

yes exactly 👌

eliperkins · 2018-06-18T14:42:42Z

Scripts/travis-build-test.sh

+
+# -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} \


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)?

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?

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.

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 😛

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.

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. 👍

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

eliperkins · 2018-06-18T14:43:09Z

Tests/DVRTests/SessionUploadTests.swift

@@ -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)


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

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.

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

eliperkins · 2018-06-18T14:44:18Z

.travis.yml

    - xcode_scheme: DVR-macOS
      xcode_sdk: macosx
      env:
-        - DESTINATION="arch=x86_64"


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).

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

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

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 😉 )

loudmouth · 2018-06-18T16:32:58Z

I think that's all the things. I'll leave it up to you guys at this point :-)

loudmouth · 2018-06-19T19:07:48Z

@eliperkins @kiliankoe @dmiluski any chance we can merge?

I wanna rebase #80 so that CI passes there.

loudmouth force-pushed the swift-package-manager branch 3 times, most recently from a995302 to 545f1f4 Compare September 29, 2017 11:08

loudmouth force-pushed the swift-package-manager branch from 545f1f4 to 724f638 Compare September 29, 2017 11:18

kiliankoe reviewed Sep 29, 2017

View reviewed changes

eliperkins approved these changes Dec 19, 2017

View reviewed changes

loudmouth force-pushed the swift-package-manager branch 6 times, most recently from 62a35dd to 233dc65 Compare December 22, 2017 15:39

loudmouth force-pushed the swift-package-manager branch 3 times, most recently from 3f95853 to dacbbe1 Compare December 22, 2017 17:31

loudmouth force-pushed the swift-package-manager branch 2 times, most recently from 7a20a7f to 713bad7 Compare February 18, 2018 10:40

tycobbb mentioned this pull request Jun 11, 2018

Support for Swift Package Manager #79

Closed

JP Wright added 5 commits June 12, 2018 18:00

Change directory layout to support Swift Package Manager

2aeddbd

Add Package.swift; update .gitignore; update .swift-version file

05bf725

Add missing import statements to enable Swift build

e3b4aef

Update .travis.yml for Xcode 9 and add seperate build script

1d402a4

Add swift build to the script

Add SPM badge to readme

6648316

Use SPM badge that is more consistent with other badges

08669fd

loudmouth force-pushed the swift-package-manager branch 5 times, most recently from 7603602 to c510295 Compare June 12, 2018 16:18

Update xcode settings and to Swift 4.1

e95a58e

loudmouth force-pushed the swift-package-manager branch from c510295 to e95a58e Compare June 12, 2018 16:22

loudmouth added 2 commits June 12, 2018 18:28

Use travis' built-in xcpretty

2ff5967

Update file path creation to use FileManager in tests

54d8d41

eliperkins approved these changes Jun 18, 2018

View reviewed changes

loudmouth force-pushed the swift-package-manager branch from 2e7c4a1 to 26a9d1a Compare June 18, 2018 16:03

Revert travis destination for OS X

13118e5

loudmouth force-pushed the swift-package-manager branch from 26a9d1a to 13118e5 Compare June 18, 2018 16:03

dmiluski merged commit 442afed into venmo:master Jun 19, 2018

loudmouth mentioned this pull request Jun 21, 2018

Add travis job for tvOS #82

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swift package manager support #73

Swift package manager support #73

loudmouth commented Sep 29, 2017 •

edited

Loading

loudmouth commented Sep 29, 2017

kiliankoe Sep 29, 2017

loudmouth Sep 29, 2017

loudmouth commented Oct 6, 2017

eliperkins left a comment

loudmouth commented Dec 22, 2017

loudmouth commented Dec 22, 2017

loudmouth commented Dec 22, 2017

loudmouth commented Feb 14, 2018

dmiluski commented Feb 14, 2018

loudmouth commented Jun 12, 2018

loudmouth commented Jun 13, 2018 •

edited

Loading

loudmouth commented Jun 18, 2018

eliperkins left a comment

eliperkins Jun 18, 2018

loudmouth Jun 18, 2018

eliperkins Jun 18, 2018

loudmouth Jun 18, 2018 •

edited

Loading

eliperkins Jun 18, 2018

eliperkins Jun 18, 2018

loudmouth Jun 18, 2018

eliperkins Jun 18, 2018

loudmouth Jun 18, 2018

eliperkins Jun 18, 2018

loudmouth Jun 18, 2018

eliperkins Jun 18, 2018

eliperkins Jun 18, 2018

loudmouth Jun 18, 2018

loudmouth Jun 18, 2018

eliperkins Jun 18, 2018

loudmouth commented Jun 18, 2018

loudmouth commented Jun 19, 2018

Swift package manager support #73

Swift package manager support #73

Conversation

loudmouth commented Sep 29, 2017 • edited Loading

loudmouth commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loudmouth commented Oct 6, 2017

eliperkins left a comment

Choose a reason for hiding this comment

loudmouth commented Dec 22, 2017

loudmouth commented Dec 22, 2017

loudmouth commented Dec 22, 2017

loudmouth commented Feb 14, 2018

dmiluski commented Feb 14, 2018

loudmouth commented Jun 12, 2018

loudmouth commented Jun 13, 2018 • edited Loading

loudmouth commented Jun 18, 2018

eliperkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loudmouth Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loudmouth commented Jun 18, 2018

loudmouth commented Jun 19, 2018

loudmouth commented Sep 29, 2017 •

edited

Loading

loudmouth commented Jun 13, 2018 •

edited

Loading

loudmouth Jun 18, 2018 •

edited

Loading