-
Notifications
You must be signed in to change notification settings - Fork 114
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
Unify android and ios setup scripts #1177
Conversation
Oh right I guess this conflicts with our current CI |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1177 +/- ##
=======================================
Coverage 30.13% 30.13%
=======================================
Files 118 118
Lines 5164 5164
Branches 1157 1108 -49
=======================================
Hits 1556 1556
- Misses 3604 3606 +2
+ Partials 4 2 -2
Flags with carried forward coverage won't be shown. Click here to find out more. |
@catarial can you edit the current CI as well? The files are in |
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.
LGTM! I have some nits/suggestions for future polishing
brew install ruby@$RUBY_VERSION | ||
else | ||
if [ $OSX_MAJOR_VERSION -ge $OSX_EXP_VERSION ]; then | ||
echo "No brew installation found, but OSX major version "$OSX_MAJOR_VERSION" and expected version "$OSX_EXP_VERSION" so CocoaPods should work" |
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.
nit; future fix: I think we should remove this check. Per @iantei, it doesn't actually work on recent versions of android. We should just say that brew is a requirement.
if [ $OSX_MAJOR_VERSION -ge $OSX_EXP_VERSION ]; then | ||
echo "No brew installation found, but OSX major version "$OSX_MAJOR_VERSION" and expected version "$OSX_EXP_VERSION" so CocoaPods should work" | ||
else | ||
echo "No brew installation found, but OSX major version "$OSX_MAJOR_VERSION" != expected version "$OSX_EXP_VERSION" CocoaPods install will likely fail" |
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.
nit, future fix: we might as well remove this as well, if we are going to make brew
required
General cleanup suggested by @shankari
Android and ios builds can be toggled with SETUP_ANDROID and SETUP_IOS environment variables, both are setup if they aren't set.