-
Notifications
You must be signed in to change notification settings - Fork 43
Package and bundle Android SDKs into the windows toolchain installer #297
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.
Looking great! I think we should make installing the Android SDK opt-in though. Most devs out there will likely never need it.
@@ -38,6 +38,10 @@ | |||
<Variable Name="OptionsInstallRedistAMD64" bal:Overridable="yes" Persisted="yes" Value="1" /> | |||
<Variable Name="OptionsInstallSdkArm64" bal:Overridable="yes" Persisted="yes" Value="1" /> | |||
<Variable Name="OptionsInstallRedistArm64" bal:Overridable="yes" Persisted="yes" Value="1" /> | |||
<Variable Name="OptionsInstallAndroidSdkArm64" bal:Overridable="yes" Persisted="yes" Value="1" /> |
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.
Is the default value specified here? I think the Android SDK should be opt-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.
Let's make arm64 and x86_64 on by default, but not the others ?
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.
Looking good!
tested in swiftlang/swift#74220 |
@compnerd could you take another look? |
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.
Minor tweak would be nice to have.
INCLUDE_ARM64_ANDROID_SDK=$(INCLUDE_ARM64_ANDROID_SDK); | ||
INCLUDE_x86_64_ANDROID_SDK=$(INCLUDE_x86_64_ANDROID_SDK); | ||
INCLUDE_ARM_ANDROID_SDK=$(INCLUDE_ARM_ANDROID_SDK); | ||
INCLUDE_X86_ANDROID_SDK=$(INCLUDE_X86_ANDROID_SDK); |
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 would be nicer to swap this around to ANDROID_...
IMO
Package and bundle Android SDKs into the windows toolchain installer (cherry picked from commit 0ecdc1c)
This change packages the Android SDKs that can be built using build.ps1 on windows into MSIs, which are then added to the toolchain's installer.
At the moment it covers aarch64 only, I'm adding and testing other arches and will update the PR with them as well.
For future follow-up: