-
Notifications
You must be signed in to change notification settings - Fork 295
Provision SwiftLint via SwiftPM instead of CocoaPods #1712
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
Conversation
Notice no source changes were required. All the code already complies with the configuration and any new rule from the newer version.
|
App Name | Simplenote Prototype Build | |
Build Number | 1285 | |
Version | PR #1712 | |
Bundle ID | com.codality.NotationalFlow.Alpha | |
Commit | 70de778 | |
Installation URL | 6qvpgbvottb8g |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock and because it worked on my machine, but left a note about running rake lint
from the Xcode shell script build phase and its potential issues depending on developers' setup
runOnlyForDeploymentPostprocessing = 0; | ||
shellPath = /bin/sh; | ||
shellScript = "[ $CI ] && exit 0\n./Pods/SwiftLint/swiftlint\n"; | ||
shellScript = "[ $CI ] && exit 0\nrake lint\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wary of if running rake
from an Xcode build phase would work as expected, given that:
- When Xcode runs shell scripts it runs them in a separate shell that might not have the same env / settings as your Terminal.app's shell (e.g.
$PATH
and whererake
is installed) - That shell script runs in
/bin/sh
, while macOS users are usingzsh
, so all their customization and setup of their shell is in.zshrc
and not.bashrc
. So again, any custom$PATH
orrbenv
andbundle
-related config etc might be set up properly in.zshrc
but not in.bashrc
for the/bin/sh
shell to pick up, which could lead to thisrake lint
command to fail - Notice how you run
rake lint
, notbundle exec rake lint
. Which means one might accidentally run this command with a different version ofrake
than the one in theGemfile.lock
bundle of this repo. Probably not a big deal (the most important thing is forswiftlint
to be using the right version, andrake
doesn't have that many updates anyway, and if it does, nothing likely breaking anything) but still inconsistent.
It still worked when I tested building the project with Xcode on my Mac:
But I think that might be a happy coincidence in how my Mac is set up, and that might not be the case for every developer? I wonder if switching to shellPath = /bin/zsh;
and using bundle exec rake lint
wouldn't be better (maybe with a need to source ~/.zshrc
explicitly first, in case Xcode running this shell in non-interactive mode would prevent that from loading… not sure, always confused with the rc files and when they're loaded wrt to interactive vs non-interactive shells etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember looking into this with Jeremy a while back and coming to the conclusion that calling rake
from Xcode works because of a number of happy coincidences in how Apple set up Ruby in macOS.
In the long run, consistently with our goal of removing Ruby from day-to-day DX, I think we'd be better off using make
which we know would be consistent without depending on Ruby setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this discussion internally to paaHJt-8yZ-p2
See https://linear.app/a8c/issue/AINFRA-449
Test
Checkout locally and run
bundle install && bundle exec pod install && bundle exec rake lint
You should see something like, possibly with additional logs such as removing SwiftLint from your pods.
Note
The UI tests failure is a long standing issue and unrelated with the changes in this PR.
Review
(Required) Add instructions for reviewers. For example:
Release