Skip to content
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

cargo-apk: Use min_sdk_version to select compiler target #197

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented Nov 16, 2021

Fixes #193, cc @hrydgard

According to 1 minSdkVersion is used to determine the compiler target and ultimately limit what API is available at compile-time. This is most likely because using/linking newer API will result in runtime linker errors on these older platforms.


I think we still have a couple things to do here:

  • Now that min_sdk_version is mandatory (see the unwrap), perhaps we should default it to the minimum SDK level available in the NDK (the min() of default_platform())?;
    (obviously reword the defaults in the cargo-apk docs);
  • Remove the "arbitrary" default of minimum version 23? Does anyone recall why this was chosen? NDK r24 beta still ships with support for SDK level 21, which would then be the lowest;
  • Is targetSdkVersion still mandatory? If so, does it still make sense to set it to the max available in the SDK?;
  • when we have these defaults, should we remove the arbitrary overrides in ndk-examples?

@hrydgard
Copy link
Contributor

hrydgard commented Nov 16, 2021

targetSdkVersion is very important to be able to set separately from the others, because Android uses it to decide behavior at times. For example, the new Scoped Storage restrictions have completely different behavior in Android 11 depending on if you're targeting sdk version <= 29 or >= 30. Also, from August 2021, Google Play has prohibited uploading apps targeting lower than 30 to Google Play. See https://developer.android.com/distribute/best-practices/develop/target-sdk

minSdkVersion just restricts which devices you can install on. I think just for ease of maintenance something like 23 makes perfect sense, older devices are not really relevant anymore.

@MarijnS95
Copy link
Member Author

targetSdkVersion is very important to be able to set separately from the others

Yes, merely checking if it still makes sense to set a default here, and if so, what. Not planning to remove/disallow the ability to override it from metadata in any way 😉

just for ease of maintenance

Here I have a hunch it's easier to use the minimum available in the NDK, as 23 will probably get removed from a future NDK at some point. On the other hand, this could cause minor variations in compilation depending on the environment. However, that's not an issue as long as it compiles fine (minSdkVersion should only have an effect on what's available during compile-time).
Not so much for targetSdkVersion which currently is dependent on the environment (only the default, though) and has the ability to cause much more havoc at runtime.

In other words, I think it should be safe to:

  • Default minSdkVersion to whatever is at least available in the NDK (perhaps with a lower bound of 23, to not break existing builds);
  • Default targetSdkVersion to the least required by Google Play as of writing, instead of being dependent on the maximum platform shipped with the SDK combined with what's in Android/Sdk/platforms/android-xx.

@MarijnS95 MarijnS95 marked this pull request as ready for review November 22, 2021 22:44
@MarijnS95
Copy link
Member Author

@hrydgard This is ready now. I'll open a separate PR for target_sdk_version, which is a different issue that needs its own documentation. Thanks for bringing this all to my attention!

According to [1] minSdkVersion is used to determine the compiler target
and ultimately limit what API is available at compile-time.  This is
for the most part because using/linking newer API will result in runtime
linker errors on these older platforms.

In other words, using the target sdk version which (as of writing, but
that will change in the near future too) defaults to the highest version
supported by the detected SDK makes it impossible to detect
compatibility issues (symbols that are unavailable) with your app on
older Android APIs despite setting min_sdk_version.

[1]: https://developer.android.com/ndk/guides/sdk-versions#minsdkversion
@hrydgard
Copy link
Contributor

Awesome, thanks for taking care of this!

MarijnS95 added a commit that referenced this pull request Nov 22, 2021
As discussed in [197] setting `target_sdk_version` to the "arbitrary"
highest available SDK version is nonsense.  This target version (unlike
`min_sdk_version` which defines the least set of symbols that should be
available) has real impact on the runtime of an application, in
particular the compatibility or stringency of rules Android applies to
your application.  Certain APIs may not work at all or be heavily
restricted on newer target versions because they are deemed too
dangerous, and Android expects the user has tested their app against
these limitations and is communicating this by setting
`target_sdk_version` to that particular value.  Hence this shouldn't
change purely based on the environment, even for the default.

To retain some backwards compatibility with previous `cargo-apk` we set
this to level 29 which is the least [required by Google Play] today, and
approximately what users with recent SDKs will be targeting already.
However, care has to be taken around ie. scoped storage, hence this is
considered a **Breaking** change.

[197]: #197 (comment)
[required by Google Play]: https://developer.android.com/distribute/best-practices/develop/target-sdk
MarijnS95 added a commit that referenced this pull request Nov 23, 2021
As discussed in [197] setting `target_sdk_version` to the "arbitrary"
highest available SDK version is nonsense.  This target version (unlike
`min_sdk_version` which defines the least set of symbols that should be
available) has real impact on the runtime of an application, in
particular the compatibility or stringency of rules Android applies to
your application.  Certain APIs may not work at all or be heavily
restricted on newer target versions because they are deemed too
dangerous, and Android expects the user has tested their app against
these limitations and is communicating this by setting
`target_sdk_version` to that particular value.  Hence this shouldn't
change purely based on the environment, even for the default.

To retain some backwards compatibility with previous `cargo-apk` we set
this to level 30 which is the least [required by Google Play] today, and
exactly what users will have been targeting using NDK r22 (assuming the
SDK for this `platform` was installed as well) since SDK version 31
support with NDK r23 only [arrived just last week].

[197]: #197 (comment)
[required by Google Play]: https://developer.android.com/distribute/best-practices/develop/target-sdk
[arrived just last week]: #189
@MarijnS95 MarijnS95 merged commit 39d4701 into master Nov 24, 2021
@MarijnS95 MarijnS95 deleted the min-sdk-version branch November 24, 2021 19:27
MarijnS95 added a commit that referenced this pull request Nov 24, 2021
As discussed in [197] setting `target_sdk_version` to the "arbitrary"
highest available SDK version is nonsense.  This target version (unlike
`min_sdk_version` which defines the least set of symbols that should be
available) has real impact on the runtime of an application, in
particular the compatibility or stringency of rules Android applies to
your application.  Certain APIs may not work at all or be heavily
restricted on newer target versions because they are deemed too
dangerous, and Android expects the user has tested their app against
these limitations and is communicating this by setting
`target_sdk_version` to that particular value.  Hence this shouldn't
change purely based on the environment, even for the default.

To retain some backwards compatibility with previous `cargo-apk` we set
this to level 30 which is the least [required by Google Play] today, and
exactly what users will have been targeting using NDK r22 (assuming the
SDK for this `platform` was installed as well) since SDK version 31
support with NDK r23 only [arrived just last week].

[197]: #197 (comment)
[required by Google Play]: https://developer.android.com/distribute/best-practices/develop/target-sdk
[arrived just last week]: #189
MarijnS95 added a commit that referenced this pull request Nov 24, 2021
As discussed in [197] setting `target_sdk_version` to the "arbitrary"
highest available SDK version is nonsense.  This target version (unlike
`min_sdk_version` which defines the least set of symbols that should be
available) has real impact on the runtime of an application, in
particular the compatibility or stringency of rules Android applies to
your application.  Certain APIs may not work at all or be heavily
restricted on newer target versions because they are deemed too
dangerous, and Android expects the user has tested their app against
these limitations and is communicating this by setting
`target_sdk_version` to that particular value.  Hence this shouldn't
change purely based on the environment, even for the default.

To retain some backwards compatibility with previous `cargo-apk` we set
this to level 30 which is the least [required by Google Play] today, and
exactly what users will have been targeting using NDK r22 (assuming the
SDK for this `platform` was installed as well) since SDK version 31
support with NDK r23 only [arrived just last week].

[197]: #197 (comment)
[required by Google Play]: https://developer.android.com/distribute/best-practices/develop/target-sdk
[arrived just last week]: #189
MarijnS95 added a commit that referenced this pull request Nov 24, 2021
As discussed in [197] setting `target_sdk_version` to the "arbitrary"
highest available SDK version is nonsense.  This target version (unlike
`min_sdk_version` which defines the least set of symbols that should be
available) has real impact on the runtime of an application, in
particular the compatibility or stringency of rules Android applies to
your application.  Certain APIs may not work at all or be heavily
restricted on newer target versions because they are deemed too
dangerous, and Android expects the user has tested their app against
these limitations and is communicating this by setting
`target_sdk_version` to that particular value.  Hence this shouldn't
change purely based on the environment, even for the default.

To retain some backwards compatibility with previous `cargo-apk` we set
this to level 30 which is the least [required by Google Play] today, and
exactly what users will have been targeting using NDK r22 (assuming the
SDK for this `platform` was installed as well) since SDK version 31
support with NDK r23 only [arrived just last week].

[197]: #197 (comment)
[required by Google Play]: https://developer.android.com/distribute/best-practices/develop/target-sdk
[arrived just last week]: #189
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.

target_sdk_version seems to affect the NDK build version?
3 participants