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

feat: support custom compileSdk setting #1431

Merged
merged 6 commits into from May 18, 2022

Conversation

erisu
Copy link
Member

@erisu erisu commented May 13, 2022

Motivation and Context

closes #1310
closes #1373

Allow setting a custom compileSdkversion, different from targetSdk.

Some libraries might require a higher compileSdk while it can still target a lower Sdk.

Generally it is recommended that the targetSdk and compileSdk match.

If a user decides to increase the targetSdk, they will now need to also manually bump the compileSdk or it will remain behind.

Description

Can set custom compileSdk version with the follow preference:

<preference name="android-compileSdkVersion" value="31" />

A warning will display during the platform add and prepare step when the targetSdk is higher then the compileSdk.

Even if the build passes, there could be potential errors further down.

Testing

npm t

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@erisu erisu added this to the 11.0.0 milestone May 13, 2022
@erisu erisu requested a review from breautek May 13, 2022 16:20
@erisu erisu added this to To Do in Release Plan - 11.0.0 via automation May 13, 2022
@erisu erisu requested a review from dpogue May 13, 2022 16:20
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2022

Codecov Report

Merging #1431 (30fbc85) into master (e730000) will increase coverage by 0.02%.
The diff coverage is 76.19%.

@@            Coverage Diff             @@
##           master    #1431      +/-   ##
==========================================
+ Coverage   73.20%   73.22%   +0.02%     
==========================================
  Files          21       21              
  Lines        1646     1666      +20     
==========================================
+ Hits         1205     1220      +15     
- Misses        441      446       +5     
Impacted Files Coverage Δ
lib/prepare.js 44.47% <0.00%> (-0.14%) ⬇️
lib/check_reqs.js 71.05% <76.47%> (+0.68%) ⬆️
lib/create.js 94.32% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e730000...30fbc85. Read the comment docs.

@erisu
Copy link
Member Author

erisu commented May 13, 2022

Downside of allowing custom compileSdk..

  • Plugins can take control of defining the version and users may not notice it changed.. Might lead to issues
  • If multiple plugins define the version, the compileSdk version from the last installed plugin is used. If the version is lower then another plugins defined version, errors might appear.
    • Usually it is recommended that the app developer sets the value in the config.xml and not controlled by plugin developers in plugin.xml. Plugin developers should only document the requirement.
  • Some plugins may not be well maintained and if they defined the compileSdk it would lock the app from upgrades.
  • If the user bumps the targetSdk, the compileSdk will no longer be in sync. The users will now have to manually control the compileSdk.

@breautek
Copy link
Contributor

breautek commented May 15, 2022

If the user bumps the targetSdk, the compileSdk will no longer be in sync. The users will now have to manually control the compileSdk.

This might be difficult since we use a JSON file to set our defaults... but wonder if there's a way to default compileSdk to the value of targetSdk.

Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

I've made a collection of suggestions that if accepted... probably should be applied together.

The attempt is rather than explicitly declaring the COMPILE_SDK_VERSION default to a hard-coded value, is to make it's default the same as SDK_VERSION.

The suggested changes I believe will work (but untested) so that unless explicitly declared, compile SDK will be whatever what target SDK is set to (even if the user changes the target SDK).

e.g.:

Use Case 1

User does not declare either target or compile

Target defaults to 32, as defined in defaults json.
compile will default to target, which is set to 32.

Use Case 2

User explicitly sets target sdk to 33

Target will be set to 33 from config preferences.
Compile will default to target, which is set to 33.

Use Case 3

User explicitly sets compile sdk to 33

Target sdk will be set to 32, which comes from the defaults json.
Compile sdk will be set to 33, which comes from config preferences.

Use Case 4

User explicitly sets both target to 30 & compile sdk to 32

Target sdk will be set to 30, which comes from config preferences.
compile sdk will be set to 32, which comes from the config preferences.

Let me know of your thoughts, or whether if this is even a good or bad idea.

framework/cdv-gradle-config-defaults.json Outdated Show resolved Hide resolved
lib/check_reqs.js Outdated Show resolved Hide resolved
lib/check_reqs.js Outdated Show resolved Hide resolved
lib/check_reqs.js Outdated Show resolved Hide resolved
Co-authored-by: Norman Breau <norman@nbsolutions.ca>
erisu and others added 2 commits May 16, 2022 12:02
@erisu erisu force-pushed the feat/support-compilesdk-config branch from 5d1c9fd to 42b971e Compare May 16, 2022 03:51
Release Plan - 11.0.0 automation moved this from To Do to In Progress May 16, 2022
Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

LGTM

@erisu
Copy link
Member Author

erisu commented May 18, 2022

Thank you for the review & votes!

@erisu erisu merged commit 4744bfe into apache:master May 18, 2022
Release Plan - 11.0.0 automation moved this from In Progress to Done May 18, 2022
@erisu erisu deleted the feat/support-compilesdk-config branch May 18, 2022 14:18
@erisu erisu mentioned this pull request May 18, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants