Skip to content

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

Merged
merged 9 commits into from
Jun 7, 2017
Merged

Added android/build-toolchain #9785

merged 9 commits into from
Jun 7, 2017

Conversation

amraboelela
Copy link
Contributor

No description provided.

@jrose-apple jrose-apple requested a review from modocache May 20, 2017 01:53
Copy link
Contributor

@modocache modocache left a 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:

  1. By using the Python argparse module, users of the script can see all the required parameters by invoking utils/android/build-toolchain --help. As is, on the other hand, they would have to invoke the script, then they'd see Please set the Android NDK directory in the ANDROID_NDK_DIR environment variable, then invoke the script again... and so on.
  2. 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 use set -e at the top of this script to prevent programmer errors from causing too much chaos :)
  3. 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.

@amraboelela
Copy link
Contributor Author

Hi modocache,
I am glad you liked it.
I don't know python, and I am sure lots of developers are like me, so for us, bash is easier to understand. Also swift/utils/build-toolchain is also written in bash, so probably we should keep it consistent.
I am not sure what set -e does, but I can add it, np.

You can suggest a python version of it if you like, and we can check it.

Regards,

@modocache
Copy link
Contributor

set -e makes the script exit with a non-zero code if any of the commands within it exit with non-zero: https://serverfault.com/a/416097

I'm aware utils/build-toolchain is in Bash, but:

  1. utils/build-toolchain doesn't take any required arguments, so the benefit of supporting --help is smaller than with this script.
  2. It only runs one sub-command, and it doesn't rm -rf anything, so it's pretty safe, even if a programmer made a mistake editing it in the future.
  3. I think it would be better in Python, too! :)

Anyway, that's just my personal preference; Bash is fine, too. Let me know if you'd like to add set -e before someone merges this.

@CodaFi
Copy link
Contributor

CodaFi commented May 30, 2017

Don't mind me, just passing through

@swift-ci please smoke test

@CodaFi
Copy link
Contributor

CodaFi commented Jun 7, 2017

⛵️

@CodaFi CodaFi merged commit 81ebb7a into swiftlang:master Jun 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants