Conversation
mokagio
left a comment
mokagio
left a comment
There was a problem hiding this comment.
Requesting changes because of the generated file issue.
Other than that, I love what you've done and where the proposal is going.
One suggestion: What do you think of calling the folder where the demo project lives Demo/ instead of GridIconsDemo/? I think a generic name might help us in the future if we need some standardized setup or automation.
This repo doesn't follow the recommended (?) SPM folder structure, but in repos that do, calling it Demo/ would also look consistent:
Demo/
Sources/
Tests/
One question, regarding Package.resolved:
I don't think we should not pin the library's dependencies (if there is any) to certain versions during development.
Why not?
.buildkite/validate_swift_package
Outdated
| # For some reason this fixes a failure in `lib lint` | ||
| # https://github.com/Automattic/buildkite-ci/issues/7 | ||
| xcrun simctl list >> /dev/null |
There was a problem hiding this comment.
Do we need this given we don't call bundle exec pod lib lint from this script?
There was a problem hiding this comment.
Good catch! I copied the build_and_test_pod script but didn't noticed the comment. We probably don't need this extra command, I'll remove it for now and can always add it back if anything goes wrong.
| s.test_spec do |test| | ||
| test.source_files = ['Gridicons/GridiconsTests/**/*.{swift}'] | ||
| end |
There was a problem hiding this comment.
TIL about test_spec.
Super neat, we should adopt it everywhere as we migrate to SPM.
For reference, here are the CI logs that show the tests running.
| // When loaded through CocoaPods, assets reside in a separate resource bundle | ||
| bundle = assetBundle | ||
| } | ||
| #endif |
There was a problem hiding this comment.
I'm afraid there's more work to be done on this particular file.
If I run the command that bundle exec rake gen runs, /path/to/project/Gridicons-iOS/vendor/swiftgen/bin/swiftgen xcassets -p Gridicons.stencil Gridicons/Gridicons/Gridicons.xcassets > Gridicons/Gridicons/GridiconsGenerated.swift, these changes are reverted.
I never worked with SwiftGen or Stencils. I assume it should be a straightforward matter of updating the template but I don't know for sure.
I wrote "run the command..." because the rake task itself fails for me with:
rake aborted!
ArgumentError: wrong first argument
/Users/gio/Developer/a8c/Gridicons-iOS/Rakefile:65:in `swiftgen'
/Users/gio/Developer/a8c/Gridicons-iOS/Rakefile:55:in `block in <top (required)>'
/Users/gio/Developer/a8c/Gridicons-iOS/vendor/bundle/ruby/2.7.0/gems/rake-13.0.1/exe/rake:27:in `<top (required)>'
/Users/gio/.rbenv/versions/2.7.4/bin/bundle:23:in `load'
/Users/gio/.rbenv/versions/2.7.4/bin/bundle:23:in `<main>'
I think the two things are unrelated.
|
|
||
| let package = Package( | ||
| name: "Gridicons", | ||
| platforms: [.iOS(.v11)], |
There was a problem hiding this comment.
I think all our apps support iOS 13 and above, with the possibility of bumping to iOS 14. Still, iOS 11 is the minimum supported by Xcode 14 so I think this is the best value to use 👍
That is, unless we find some code improvements that might require an higher SDK, but those ought to be saved for a dedicated PR anyway.
fastlane/Fastfile
Outdated
| run_tests( | ||
| package_path: '.', | ||
| scheme: 'Gridicons', | ||
| devices: ['iPhone 11'], |
There was a problem hiding this comment.
Out of scope for this PR, but iPhone 11 is not available out of the box with Xcode 14 (that's something I didn't notice when upgrading CI, sorry about that).
| platform :ios do | ||
| desc 'Builds the project and runs tests' | ||
| desc 'Builds the Swift package and runs tests' | ||
| lane :test do |
There was a problem hiding this comment.
This fails for me locally with
[06:08:50]: ▸ xcodebuild: error: The workspace named "Gridicons" does not contain a scheme named "Gridicons". The "-list" option can be used to find the names of the schemes in the workspace.
However, it works in CI 🤔 I wonder what the difference is between the two setups. The only thing I can think of is Intel vs Apple Silicon, but that doesn't sound related to a scheme not being found.
There was a problem hiding this comment.
This lanes works on my M1 mac though. Any chance you have a xcodeproj or xcworkspace file at your root directory?
|
@mokagio I actually changed my mind regarding the |
My plan for now is keeping the existing folder structure, for two reasons
|
@crazytonyli fair enough. I don't feel strongly about the file names. Well, actually... A part of me really wishes we had Doing a blanket rename is a straightforward operation that we can do anytime in the future if a good reason for it comes up, so no worries 👍 I was curious about the
|
mokagio
left a comment
mokagio
left a comment
There was a problem hiding this comment.
thank you for following up promptly and for fixing the Rake task.
I tried it again locally and there was no diff 🎉
Yes,
So if we wanted to reorganize the files to follow SPM filesystem structure conventions, that wouldn't cause any trouble with |
|
@AliSoftware I can't remember the exact issue, but I had troubles with git blame option, maybe I wasn't using them correctly. For easier PR review, I can create a follow up PR to follow SPM's convention, what do you all think? |
|
Yeah if you want to try that doing so in a separate PR would be a good way to go imho. I wonder if it makes a difference to git blame if you used |
Sounds good to me. I am not too worried about adhering to the |
3 participants
Not just adding a
Package.swiftfile, this PR also proposes a structure that potentially can be shared among all our shared libraries.Development
During development, we should treat the library as a Swift package. There are two ways to develop the Swift package in Xcode:
Both approaches work, they don't conflict with each other. I don't see there is any need of recommending one over the other. But we may want to add steps in CI to ensure the demo/example app continues to build.
Distribution
The library can be imported as a Swift package and a CocoaPods pod. But since we no longer use the library's
podspecfile during development, we could easily overlook CocoaPods integration. One way to prevent this issue is adding "test spec" to the podspec so that thepod lib lintstep in CI not only validates if the podspec can be imported as a pod but also tests its functionality—kinda like how you can build and test targets in a Swift Package.I want to highlight one thing. Since CocoaPods and SPM package the library differently, especially on the resource bundles, we'll need to make sure there is test coverage around loading resource bundles.
Summary
To summarize all above, here are list of things that we should do in our libraries.
test_specto the podspec file. The test spec should have the same test cases that are declared inPackage.swift.validate_swift_packagescript is added in this PR, which should serve the same purpose asvalidate_podspecand should be moved to the buildkite plugin repo.xcodebuildcommand line picks up thePackage.swiftfile, not the workspace or project file(*).Ignore two files in the git repo:Update: Check inPackage.resolvedand.swiftpmdirectory. I don't think we should not pin the library's dependencies (if there is any) to certain versions during development.Package.resolvedto pin the dependencies used during development—similar to how we check inPodfile.lock.(*)
xcodebuildhas this very strange behavior when dealing withPackage.swift. To make the command run tests in thePackage.swift, we need to run the command in the directory ofPackage.swift, without passing-workspacenor-projectoption. Suppose there is an xcworkspace or xcodeproject in thePackage.swiftdirectory, they will use them instead of thePackage.swiftfile—there is no equivalent of-workspace MyApp.workspacefor Swift Package.CHANGELOG.mdif necessary.