Skip to content

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

Merged
merged 11 commits into from
Jun 10, 2024

Conversation

hyp
Copy link
Contributor

@hyp hyp commented May 22, 2024

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:

Copy link
Contributor

@tristanlabelle tristanlabelle left a 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" />
Copy link
Contributor

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.

Copy link
Contributor Author

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 ?

lxbndr added a commit to readdle/swift-installer-scripts that referenced this pull request May 25, 2024
hyp pushed a commit that referenced this pull request May 28, 2024
Copy link
Contributor

@tristanlabelle tristanlabelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@hyp
Copy link
Contributor Author

hyp commented Jun 7, 2024

tested in swiftlang/swift#74220

@hyp
Copy link
Contributor Author

hyp commented Jun 7, 2024

@compnerd could you take another look?

Copy link
Member

@compnerd compnerd left a 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.

Comment on lines 11 to 14
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);
Copy link
Member

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

@hyp hyp merged commit 0ecdc1c into main Jun 10, 2024
@hyp hyp deleted the eng/android-sdk branch June 10, 2024 15:39
compnerd pushed a commit that referenced this pull request Jun 10, 2024
hyp added a commit that referenced this pull request Nov 18, 2024
Package and bundle Android SDKs into the windows toolchain installer

(cherry picked from commit 0ecdc1c)
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.

4 participants