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

#6 macOS Support #25

Merged
merged 1 commit into from
Mar 9, 2017
Merged

#6 macOS Support #25

merged 1 commit into from
Mar 9, 2017

Conversation

128keaton
Copy link
Collaborator

@128keaton 128keaton commented Mar 4, 2017

Adds support for macOS (resolves #6)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 58a4249 on 128keaton:master into ** on iwasrobbed:master**.

@iwasrobbed
Copy link
Collaborator

Thanks @128keaton ! Mind describing the project changes real quick? Might be easier than me trying to visually see what's changed

@iwasrobbed iwasrobbed changed the title macOS Support #6 macOS Support Mar 4, 2017
@128keaton
Copy link
Collaborator Author

128keaton commented Mar 5, 2017

@iwasrobbed I actually changed very little. I added a new Target for macOS, then just allowed 'ownership' of the needed files to the target. I shimmed code in two files to allow the compiler to use the correct frameworks for each platform, and the correct line of code to open the URL.

Here was the correction for the URL opening: https://github.com/iwasrobbed/Down/blob/58a4249f8f22fa51d9d3bbbca499c57f80ae46a7/Source/Views/DownView.swift#L101

And for the string extension:
https://github.com/iwasrobbed/Down/blob/58a4249f8f22fa51d9d3bbbca499c57f80ae46a7/Source/Extensions/NSAttributedString%2BHTML.swift#L9

Lastly, looks like Xcode automatically updated some naming schemes to conform, I couldn't make the framework's name 'Down-macOS' because the dash made Xcode lash out. That and the basic file changes covers MOST of the 'major' changes according to git.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 642972a on 128keaton:master into ** on iwasrobbed:master**.

@128keaton
Copy link
Collaborator Author

err I didn't want those to be pulled..wtf Github.

@iwasrobbed
Copy link
Collaborator

Thanks @128keaton! (btw, you'd need to fork your local branch if you don't want that commit showing up here; otherwise, any commit to your master will be on this PR)

@iwasrobbed
Copy link
Collaborator

I think the 642972a commit is fine to keep, I'll have a look later

@iwasrobbed iwasrobbed self-requested a review March 6, 2017 14:39
Copy link
Collaborator

@iwasrobbed iwasrobbed left a comment

Choose a reason for hiding this comment

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

The only thing I see missing is a new test target for the macOS path.

  • Here's an example if you wouldn't mind adding and making sure it passes locally before committing your .travis.yml changes

Down's .travis.yml file is in the root dir: https://github.com/iwasrobbed/Down/blob/master/.travis.yml

@128keaton
Copy link
Collaborator Author

@iwasrobbed okay I'll see if I can figure it out. I've never used tests or travis before. If anyone else who knows how to write tests just makes the changes in their branch I'll close this request and you can use theirs

@iwasrobbed
Copy link
Collaborator

@128keaton Ah no worries, no need to write new tests most likely (we just need to execute the existing tests against your new macOS target).

The best thing to do is just run that tests command locally on your Terminal cli for the iOS target and then adapt it so we can also add one for the macOS target (like that other example I showed was doing)

Existing iOS test command:

set -o pipefail && xcodebuild test -project Down.xcodeproj -scheme "Down-iOS" -destination "platform=iOS Simulator,name=iPhone 5,OS=9.3" -enableCodeCoverage YES ONLY_ACTIVE_ARCH=YES | xcpretty -c

I imagine the macOS version would look like

set -o pipefail && xcodebuild test -project Down.xcodeproj -scheme "Down-macOS" -sdk macosx -destination "platform=OS X,arch=x86_64" -enableCodeCoverage YES ONLY_ACTIVE_ARCH=YES | xcpretty -c

If that works locally on your computer, we hopefully shouldn't have problems just appending it to our .travis.yml file and getting it passing.

Does that help clarify it?

@128keaton
Copy link
Collaborator Author

128keaton commented Mar 6, 2017

@iwasrobbed well I can't get OS X tests to run, at all, Early unexpected exit, operation never finished bootstrapping - no restart will be attempted. I've added the OS X framework to the testing binaries, tried codesigning, cleaning, relaunching Xcode, rebooting. Stumped.
Log: https://gist.github.com/128keaton/d4cf59fc2bde7ec4670ab7ec506d866b

@128keaton
Copy link
Collaborator Author

Okay, I created a tests target for OS X (oops) and the command works if I specify the testing scheme.

@iwasrobbed
Copy link
Collaborator

@128keaton Welcome to the fun (but necessary) world of testing :) If things are working as planned, feel free to add that new command to the travis script and commit the changes and the PR will re-run w/ those tests

@128keaton
Copy link
Collaborator Author

Yup. I need to start doing this with all of my projects. Currently learning Travis. Let me check it out and I'll push the test branch

* Created two new Targets
* Updated .travis.yml file 
* Updated schemes for testing
* Update Down.podspec
@128keaton
Copy link
Collaborator Author

Lets see if this works!

@coveralls
Copy link

coveralls commented Mar 9, 2017

Coverage Status

Changes Unknown when pulling c1ff17e on 128keaton:master into ** on iwasrobbed:master**.

@iwasrobbed
Copy link
Collaborator

Travis is really finicky and looks like it errored out for internal reasons, so I restarted the build to see if it'd clear itself 🍿 👀

@coveralls
Copy link

coveralls commented Mar 9, 2017

Coverage Status

Changes Unknown when pulling c1ff17e on 128keaton:master into ** on iwasrobbed:master**.

@iwasrobbed iwasrobbed merged commit 544f788 into johnxnguyen:master Mar 9, 2017
@iwasrobbed
Copy link
Collaborator

Look at that, worked like a charm. Thanks again (and also thanks for your patience & for rebasing) @128keaton !

@128keaton
Copy link
Collaborator Author

Yeah haha don't worry about it. Glad it works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OS X support
3 participants