Carthage compatibility #6

fortmarek · 2018-12-21T16:48:50Z

I added Carthage compatibility and bumped up CryptoSwift version since the compiler complained the version it was there previously had errors.

MnemonicKit.podspec

MnemonicKit/MnemonicKit.h

keefertaylor · 2018-12-23T18:31:02Z

Podfile

@@ -6,9 +6,5 @@ platform :ios, '12.0'
 target 'MnemonicKitExampleApp' do
 end



My idea here was that there were three targets:

An example app, good for local develoment / messing about

A static library, for consumption of dependent projects

A test target, contains tests for static library.

I see that you're moving the static library target to a framework? What is the rationale there? Where does the dependency for CryptoSwift get declared now?

I think it's a standard to ship libraries like this as frameworks, at least from what I understand (I'm interested in your rationale why you chose static library).

Three targets are still there, I just deleted it from the pod, since, well, there are no pods 😄
I should probably return it there, so it's not forgotten about.

CryptoSwift is installed via Carthage and declared as a dependency. 🙂

Ok, I did some research. Thanks for being patient, I'm not overly familiar with the tooling.

Both Carthage and Cocoapods ultimately build shared libraries into frameworks. Also, on iOS it seems that frameworks are a bit more flexible in that they can bundle additional resources with code and they are easier to include in project files.

I agree that it is then best to develop locally as a framework since that is what it is being shipped as.

It is also worth noting that this is a change in the Podfile which declares the settings for the local xcode project. The .podspec file declares the settings to build a shared library. The CryptoSwift dependency still exists in the podspec, so I suspect that the pod will build and ship fine.

However, I do not think that opening the project locally and building with Cocoapods will work as the Podfile no longer declares a CryptoSwift dependency. I think you may need to run pod install again to pick up changes after you've modified this file. Can you verify that you can still build the tests with CocoaPods?

Finally (and maybe this is obvious at this point) but Carthage just has the one cartfile which looks fine from here.

No, running pod install will not work locally - but is that something that we would want to have? I do not see any benefits to it and looking at popular projects in the Swift community, most of them do not declare this kind of dependency twice. (declaring it in podspec should be more than fine).

I didn't realize that but after poking around a bit more I agree with your sentiment.

My understanding then is that to do development locally you would run:

$ carthage bootstrap --platform iOS --cache-builds $ open open MnemonicKit.xcodeproj/

Correct?

Yes, that is correct. Probably should add it to the manual.

keefertaylor · 2018-12-23T18:37:47Z

MnemonicKit.xcodeproj/xcshareddata/xcschemes/MnemonicKit.xcscheme

@@ -28,26 +28,7 @@
      selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
      shouldUseLaunchSchemeArgsEnv = "YES">
      <Testables>


This PR fails on CI: https://travis-ci.org/keefertaylor/MnemonicKit/builds/471070508

The command being used is:
xcodebuild -enableCodeCoverage=YES -scheme MnemonicKitTests -workspace MnemonicKit.xcworkspace -destination platform\=iOS\ Simulator,OS\=12.0,name\=iPhone\ X test

and fails with:
: error: Unable to find a destination matching the provided destination specifier: { platform:iOS Simulator, OS:12.0, name:iPhone X }

I think the missing destination might be a red herring since the MnemonicKitTests scheme is deleted here.

Should work now.

keefertaylor · 2019-01-04T00:59:13Z

.travis.yml

+deploy:
+  provider: releases
+  api_key:
+    secure: fLdK+WsGAXajnI6BVNjthNyXx+v+YihXAtZNO85LdA2p8o4OthlXCivdpby4VbFwMxI/JXAnNJSygYNDULwkdower43MwBurXenj45MWRyJ3dkSbG2Aqd0Qtb1CEu2Djm/MuOJ2Nz60MqWtC2blqZf7XbxDH5Bbq2wNGYnrktxWGDhUaNHJumSLdg5y6xbC+H5NGeXONXHfTJhVOrvzvYEAkYoKPk34P01BPUF6HPyCqcwzixpkhYcnsgckgBu1JMUlTJR5aiTNwLjPxbyI5Q/2VuUX8Ma7W7Wv0rVcwGAQae/eJHE5cuNNLJOxvX9CnhxbOhmf5Vq7dLy94aKdDhcSujcPt3ue7jJMuqf2kKR1IlURVYEFBp8wi49d8W+J/WeFEzPeL6ZsURsg6N2IvNjGjcnIgf6oIwUmWVPFlFWar2/QN03N1KVS+Ihn8qhu5BxtV9dm6teYaYdVU4v5rHniwg+TbxsalqkHG3JubP+WutSm9QHz/cFERWEh6PAbnVask2Nmho8h9NQOomv/1zJzu/QaAZcjZ6OxzYKDdVuem71dyqi5cO8AzD9HGbDo6RkRjTjVjTkCgnc2GjmfgElp8fhuLGdJzQS/o7vQgSIY9iYiFHb4XT122rzHJf91RoLNNhUQw+K6NgBBuhSYVAgYV6e0KnbgxgJWZi+fP3QI=


Just OOC, what is this API key used for?

It's used for encrypting variables that are necessary for project to be successfully tested, but should be deleted in this case, I forgot to do so, sorry.

keefertaylor · 2019-01-04T01:01:28Z

.travis.yml

+- brew update
+- brew outdated carthage || brew upgrade carthage
+- carthage bootstrap --platform iOS --cache-builds
+- pod install || pod install --repo-update


This command now fails on CI: https://travis-ci.org/keefertaylor/MnemonicKit/builds/474772677

(Obviously there is no longer a Podfile)

Makes sense, my bad.

keefertaylor · 2019-01-04T01:02:03Z

.travis.yml

+before_deploy:
+- carthage build --no-skip-current --platform iOS --cache-builds
+- carthage archive $FRAMEWORK_NAME
+- pod trunk push


I don't think we want to push to Cocoapods trunk here. Besides that Cocoapods won't have an API key available, we shouldn't publish the effects of any build.

Likewise, you should consider if carthage archive is really what you want to do here (I'm less familiar)

That's what the api_key was for, but I obviously do not have it.

before_deploy and deploy only occur on a new release pushed to repository's master, so I would argue that is something we would want to do.

My proposal is to leave the carthage there and I'll leave the decision about pod on you since I can not implement it anyway.

keefertaylor · 2019-01-04T01:03:42Z

.travis.yml

 script:
-  - xcodebuild -enableCodeCoverage=YES -scheme MnemonicKitTests -workspace MnemonicKit.xcworkspace -destination platform\=iOS\ Simulator,OS\=12.0,name\=iPhone\ X test
-
+- set -o pipefail && xcodebuild test -scheme MnemonicKit -destination 'platform=iOS Simulator,name=iPhone XS,OS=12.0' ONLY_ACTIVE_ARCH=YES | xcpretty


I think you want to run the MnemonicKitTests scheme. MnemonicKit is only going to build the framework IIUC.

I added MnemonicKitTests as the framework's test target, so it should be fine, I think.

keefertaylor · 2019-01-04T01:05:49Z

MnemonicKit.xcodeproj/xcshareddata/xcschemes/MnemonicKitTests.xcscheme

@@ -10,7 +10,6 @@
      buildConfiguration = "Debug"
      selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
      selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
-      codeCoverageEnabled = "YES"


I think this line is important because it generates the CodeCoverage file that is used by Slather and codecov.io

Yeah, you're right, I have deleted and re-added the test target to fix some issue and have not noticed this had got deleted, so thanks for noticing 🎊

keefertaylor · 2019-01-04T01:08:09Z

MnemonicKit.podspec

  s.license      = { :type => "MIT", :file => "LICENSE" }
  s.author             = { "Keefer Taylor" => "[email protected]" }
-  s.source       = { :git => "https://github.com/fortmarek/MnemonicKit.git", :tag => "1.2.0" }
+  s.source       = { :git => "https://github.com/keefertaylor/MnemonicKit.git", :tag => "1.2.0" }
  s.source_files  = "Sources/**/*.swift",


Below you've moved the source files from Sources/MnemonicKit to MnemonicKit so you'll need to update this here as well.

OOC why move those to be in a different folder? Swift libraries seem to use either a Source or Sources folder, with either a Playground or an Example folder that has an example app in it. Of the top 15 repositories, only 5 seem to use this style [1]. To be clear, I don't really care about what the folder is named so don't feel the need to revert, I am just genuinely curious if you had a strong feeling about this.

What do you think is the most idiomatic thing to do with the example app bit? Should I move it to an examples folder? Replace with a Playground? I can follow up on this in a separate PR so as not to create more work for yourself.

[1] https://github.com/search?l=Swift&o=desc&q=swift&s=stars&type=Repositories

It's something that I find more readable to the user who has never seen the project before. I think the Source folder should be now deleted and the App folder contents should be moved to Example folder. Anyway, I'd leave it to different PR, I do not mind doing it myself, but I think we should focus now to finish this one and not make it too huge 🙂

keefertaylor · 2019-01-04T01:37:01Z

Podfile

@@ -6,9 +6,5 @@ platform :ios, '12.0'
 target 'MnemonicKitExampleApp' do
 end



I didn't realize that but after poking around a bit more I agree with your sentiment.

My understanding then is that to do development locally you would run:

$ carthage bootstrap --platform iOS --cache-builds $ open open MnemonicKit.xcodeproj/

Correct?

codecov-io · 2019-01-07T09:37:33Z

Codecov Report

Merging #6 into master will increase coverage by 1.48%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
+ Coverage   91.24%   92.72%   +1.48%     
==========================================
  Files           3        3              
  Lines         137      110      -27     
==========================================
- Hits          125      102      -23     
+ Misses         12        8       -4

Impacted Files	Coverage Δ
MnemonicKit/String+MnemonicData.swift	`100% <ø> (ø)`
MnemonicKit/Data+BitArray.swift	`100% <ø> (ø)`
MnemonicKit/Mnemonic.swift	`89.61% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da90398...5d778de. Read the comment docs.

Re-integrate swiftlint support (which was accidentally removed in #6). In addition, de-integrate swiftformat. Fix style violations throughout code.

Marek Fořt added 5 commits December 21, 2018 16:43

Podspec changes.

d14d275

Add shared scheme.

b20e18f

Added cartfile

7caf85a

Edited gitignore.

0f9b5b9

CryptoSwift imported with Carthage.

4fa3f66

keefertaylor self-requested a review December 23, 2018 18:25

keefertaylor reviewed Dec 23, 2018

View reviewed changes

MnemonicKit.podspec Outdated Show resolved Hide resolved

keefertaylor reviewed Dec 23, 2018

View reviewed changes

MnemonicKit.podspec Outdated Show resolved Hide resolved

keefertaylor reviewed Dec 23, 2018

View reviewed changes

MnemonicKit/MnemonicKit.h Outdated Show resolved Hide resolved

keefertaylor reviewed Dec 23, 2018

View reviewed changes

Make travis setup work, deintegrate cocoapods.

ec0e307

keefertaylor reviewed Jan 4, 2019

View reviewed changes

Marek Fořt added 3 commits January 4, 2019 08:54

Change travis config, edit README.

50f1e2d

Add space to README.

92ee844

Change source_files, remove pod lib lint.

5d778de

keefertaylor merged commit 85776c0 into keefertaylor:master Jan 8, 2019

keefertaylor added a commit that referenced this pull request Jan 27, 2019

Integrate linter

3722358

Re-integrate swiftlint support (which was accidentally removed in #6). In addition, de-integrate swiftformat. Fix style violations throughout code.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Carthage compatibility #6

Carthage compatibility #6

fortmarek commented Dec 21, 2018

keefertaylor Dec 23, 2018

fortmarek Dec 23, 2018

keefertaylor Dec 25, 2018

fortmarek Jan 3, 2019

keefertaylor Jan 4, 2019

fortmarek Jan 4, 2019

keefertaylor Dec 23, 2018

fortmarek Jan 3, 2019

keefertaylor Jan 4, 2019

fortmarek Jan 4, 2019

keefertaylor Jan 4, 2019

fortmarek Jan 4, 2019

keefertaylor Jan 4, 2019

keefertaylor Jan 4, 2019

fortmarek Jan 4, 2019

keefertaylor Jan 4, 2019

fortmarek Jan 4, 2019

keefertaylor Jan 4, 2019

fortmarek Jan 4, 2019

keefertaylor Jan 4, 2019

fortmarek Jan 4, 2019

keefertaylor Jan 4, 2019

codecov-io commented Jan 7, 2019

		@@ -6,9 +6,5 @@ platform :ios, '12.0'
		target 'MnemonicKitExampleApp' do
		end

Carthage compatibility #6

Carthage compatibility #6

Conversation

fortmarek commented Dec 21, 2018

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

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

codecov-io commented Jan 7, 2019

Codecov Report