-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Added android/build-toolchain #9785
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
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.
Looks great! This should be useful for anyone experimenting with Swift on Android, thanks for writing this!
I have one suggestion, though: how about writing this in Python? I think that would be improvement for three reasons:
- By using the Python
argparse
module, users of the script can see all the required parameters by invokingutils/android/build-toolchain --help
. As is, on the other hand, they would have to invoke the script, then they'd seePlease set the Android NDK directory in the ANDROID_NDK_DIR environment variable
, then invoke the script again... and so on. - I think using Python could prevent some errors if this script is modified in the future. For example, the line
rm -rf ${SWIFT_TOOLCHAIN_DIR}
in this script worries me... Python makes it a little harder to accidentally delete a directory. Or, if you'd like to keep using Bash, please useset -e
at the top of this script to prevent programmer errors from causing too much chaos :) - Many of the scripts in this repository are written in Python, and personally I think Python is easier to read than Bash.
What do you think? I'm open to keeping this in Bash, if you'd like to do so, please let me know why.
Hi modocache, You can suggest a python version of it if you like, and we can check it. Regards, |
I'm aware
Anyway, that's just my personal preference; Bash is fine, too. Let me know if you'd like to add |
Don't mind me, just passing through @swift-ci please smoke test |
⛵️ |
No description provided.