-
Notifications
You must be signed in to change notification settings - Fork 529
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
Updated .gitignore and automated setup procedure #3943
Conversation
scripts/setup.sh
Outdated
arch_name="$(uname -m)" | ||
if [ "${arch_name}" = "x86_64" ]; then | ||
if [ "$(sysctl -in sysctl.proc_translated)" = "1" ]; then | ||
echo '\nprotobuf_platform=osx-x86_64' >> ../oppia-android/local.properties |
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.
#3891 for context
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.
Suggest adding this context as a comment if it's important to make the association (it's more important to have that as a permanent record in code vs. a PR comment that might be hard to find in the future).
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.
It seems my comment is no longer relevant--the latest version of this is now using a shell script.
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.
Thanks @FareesHussain! Just had one comment--PTAL.
Sorry, will need to review this tomorrow. |
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 thanks Farees!
Unassigning @vinitamurthi since they have already approved the PR. |
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.
Apologies; I probably could have followed up yesterday (I didn't realize you were just looking for clarification). PTAL @FareesHussain.
Unassigning @anandwana001 since the review is done. |
Unassigning @FareesHussain since a re-review was requested. @FareesHussain, please make sure you have addressed all review comments. Thanks! |
Got it, PTAL at my one last comment for comment update, else all LGTM |
@anandwana001 can you resolve the other comments I'm unable to understand which one (all of them are related to comment) |
Unassigning @FareesHussain since a re-review was requested. @FareesHussain, please make sure you have addressed all review comments. Thanks! |
Co-authored-by: Akshay Nandwana <akshaynandwana001@gmail.com>
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
Unassigning @anandwana001 since they have already approved the PR. |
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.
Thanks @FareesHussain. Just had two more comments, PTAL.
Hi @FareesHussain, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
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.
Thanks @FareesHussain. I think this looks great! Approved for codeowners.
scripts/setup.sh
Outdated
arch_name="$(uname -m)" | ||
if [ "${arch_name}" = "x86_64" ]; then | ||
if [ "$(sysctl -in sysctl.proc_translated)" = "1" ]; then | ||
echo '\nprotobuf_platform=osx-x86_64' >> ../oppia-android/local.properties |
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.
It seems my comment is no longer relevant--the latest version of this is now using a shell script.
Merging since there don't seem to be any outstanding comments to address, CI checks are passing, and this should be a big help with contributor PRs (since we keep seeing people changing unrelated .idea files). |
Updated scripts for setup.sh to automatically update the local.properties for M1 mac (to use protoc using gradle)
Added misc.xml and runconfigurations.xml to .gitignore