Improve Gradle Build Arguments (#699)

* Remove `uses-sdk` from AndroidManifest * Remove dependency `elementtree` * Updated Build Command Help Menu Printout * Cleanup `minSdkVersion` printout * Added `maxSdkVersion`, but not recommended to set. * Added `targetSdkVersion` * Update the `GradlePropertiesParser` & Test Spec * Always Set Overriding Changes * Update existing properties * Update configure method
apache · Apr 6, 2019 · 485e6e0 · 485e6e0
1 parent 516c341
commit 485e6e0
Show file tree

Hide file tree

Showing 15 changed files with 104 additions and 127 deletions.
diff --git a/bin/lib/create.js b/bin/lib/create.js
@@ -321,7 +321,6 @@ exports.create = function (project_path, config, options, events) {
 
                 var manifest = new AndroidManifest(path.join(project_template_dir, 'AndroidManifest.xml'));
                 manifest.setPackageId(package_name)
-                    .setTargetSdkVersion(target_api.split('-')[1])
                     .getActivity().setName(safe_activity_name);
 
                 var manifest_path = path.join(app_path, 'AndroidManifest.xml');

diff --git a/bin/templates/cordova/lib/AndroidManifest.js b/bin/templates/cordova/lib/AndroidManifest.js
@@ -18,7 +18,6 @@
 */
 
 var fs = require('fs');
-var et = require('elementtree');
 var xml = require('cordova-common').xmlHelpers;
 
 var DEFAULT_ORIENTATION = 'default';
@@ -98,31 +97,6 @@ AndroidManifest.prototype.getActivity = function () {
     };
 };
 
-['minSdkVersion', 'maxSdkVersion', 'targetSdkVersion'].forEach(function (sdkPrefName) {
-    // Copy variable reference to avoid closure issues
-    var prefName = sdkPrefName;
-
-    AndroidManifest.prototype['get' + capitalize(prefName)] = function () {
-        var usesSdk = this.doc.getroot().find('./uses-sdk');
-        return usesSdk && usesSdk.attrib['android:' + prefName];
-    };
-
-    AndroidManifest.prototype['set' + capitalize(prefName)] = function (prefValue) {
-        var usesSdk = this.doc.getroot().find('./uses-sdk');
-
-        if (!usesSdk && prefValue) { // if there is no required uses-sdk element, we should create it first
-            usesSdk = new et.Element('uses-sdk');
-            this.doc.getroot().append(usesSdk);
-        }
-
-        if (prefValue) {
-            usesSdk.attrib['android:' + prefName] = prefValue;
-        }
-
-        return this;
-    };
-});
-
 AndroidManifest.prototype.getDebuggable = function () {
     return this.doc.getroot().find('./application').attrib['android:debuggable'] === 'true';
 };
@@ -150,7 +124,3 @@ AndroidManifest.prototype.write = function (destPath) {
 };
 
 module.exports = AndroidManifest;
-
-function capitalize (str) {
-    return str.charAt(0).toUpperCase() + str.slice(1);
-}
diff --git a/bin/templates/cordova/lib/build.js b/bin/templates/cordova/lib/build.js
@@ -38,6 +38,8 @@ function parseOpts (options, resolvedTarget, projectRoot) {
         prepenv: Boolean,
         versionCode: String,
         minSdkVersion: String,
+        maxSdkVersion: String,
+        targetSdkVersion: String,
         gradleArg: [String, Array],
         keystore: path,
         alias: String,
@@ -56,6 +58,8 @@ function parseOpts (options, resolvedTarget, projectRoot) {
 
     if (options.argv.versionCode) { ret.extraArgs.push('-PcdvVersionCode=' + options.argv.versionCode); }
     if (options.argv.minSdkVersion) { ret.extraArgs.push('-PcdvMinSdkVersion=' + options.argv.minSdkVersion); }
+    if (options.argv.maxSdkVersion) { ret.extraArgs.push('-PcdvMaxSdkVersion=' + options.argv.maxSdkVersion); }
+    if (options.argv.targetSdkVersion) { ret.extraArgs.push('-PcdvTargetSdkVersion=' + options.argv.targetSdkVersion); }
     if (options.argv.gradleArg) {
         ret.extraArgs = ret.extraArgs.concat(options.argv.gradleArg);
     }
@@ -265,7 +269,9 @@ module.exports.help = function () {
     console.log('    \'--nobuild\': will skip build process (useful when using run command)');
     console.log('    \'--prepenv\': don\'t build, but copy in build scripts where necessary');
     console.log('    \'--versionCode=#\': Override versionCode for this build. Useful for uploading multiple APKs.');
-    console.log('    \'--minSdkVersion=#\': Override minSdkVersion for this build. Useful for uploading multiple APKs.');
+    console.log('    \'--minSdkVersion=#\': Override minSdkVersion for this build.');
+    console.log('    \'--maxSdkVersion=#\': Override maxSdkVersion for this build. (Not Recommended)');
+    console.log('    \'--targetSdkVersion=#\': Override targetSdkVersion for this build.');
     console.log('    \'--gradleArg=<gradle command line arg>\': Extra args to pass to the gradle command. Use one flag per arg. Ex. --gradleArg=-PcdvBuildMultipleApks=true');
     console.log('');
     console.log('Signed APK flags (overwrites debug/release-signing.proprties) :');

diff --git a/bin/templates/cordova/lib/config/GradlePropertiesParser.js b/bin/templates/cordova/lib/config/GradlePropertiesParser.js
@@ -84,10 +84,20 @@ class GradlePropertiesParser {
             let value = this.gradleFile.get(key);
 
             if (!value) {
+                // Handles the case of adding missing defaults or new properties that are missing.
                 events.emit('verbose', `[Gradle Properties] Appending configuration item: ${key}=${properties[key]}`);
                 this.gradleFile.set(key, properties[key]);
             } else if (value !== properties[key]) {
-                events.emit('info', `[Gradle Properties] Detected Gradle property "${key}" with the value of "${value}", Cordova's recommended value is "${properties[key]}"`);
+                if (this._defaults[key] && this._defaults[key] !== properties[key]) {
+                    // Since the value does not match default, we will notify the discrepancy with Cordova's recommended value.
+                    events.emit('info', `[Gradle Properties] Detected Gradle property "${key}" with the value of "${properties[key]}", Cordova's recommended value is "${this._defaults[key]}"`);
+                } else {
+                    // When the current value exists but does not match the new value or does matches the default key value, the new value it set.
+                    events.emit('verbose', `[Gradle Properties] Updating Gradle property "${key}" with the value of "${properties[key]}"`);
+                }
+
+                // We will set the new value in either case.
+                this.gradleFile.set(key, properties[key]);
             }
         });
     }

diff --git a/bin/templates/cordova/lib/prepare.js b/bin/templates/cordova/lib/prepare.js
@@ -45,9 +45,13 @@ module.exports.prepare = function (cordovaProject, options) {
 
     // Get the min SDK version from config.xml
     const minSdkVersion = this._config.getPreference('android-minSdkVersion', 'android');
+    const maxSdkVersion = this._config.getPreference('android-maxSdkVersion', 'android');
+    const targetSdkVersion = this._config.getPreference('android-targetSdkVersion', 'android');
 
     let gradlePropertiesUserConfig = {};
     if (minSdkVersion) gradlePropertiesUserConfig.cdvMinSdkVersion = minSdkVersion;
+    if (maxSdkVersion) gradlePropertiesUserConfig.cdvMaxSdkVersion = maxSdkVersion;
+    if (targetSdkVersion) gradlePropertiesUserConfig.cdvTargetSdkVersion = targetSdkVersion;
 
     let gradlePropertiesParser = new GradlePropertiesParser(this.locations.root);
     gradlePropertiesParser.configure(gradlePropertiesUserConfig);
@@ -205,9 +209,6 @@ function updateProjectAccordingTo (platformConfig, locations) {
     manifest.setVersionName(platformConfig.version())
         .setVersionCode(platformConfig.android_versionCode() || default_versionCode(platformConfig.version()))
         .setPackageId(androidPkgName)
-        .setMinSdkVersion(platformConfig.getPreference('android-minSdkVersion', 'android'))
-        .setMaxSdkVersion(platformConfig.getPreference('android-maxSdkVersion', 'android'))
-        .setTargetSdkVersion(platformConfig.getPreference('android-targetSdkVersion', 'android'))
         .write();
 
     // Java file paths shouldn't be hard coded

diff --git a/bin/templates/project/AndroidManifest.xml b/bin/templates/project/AndroidManifest.xml
@@ -44,6 +44,4 @@
             </intent-filter>
         </activity>
     </application>
-
-    <uses-sdk android:minSdkVersion="19" android:targetSdkVersion="__APILEVEL__"/>
 </manifest>
diff --git a/bin/templates/project/app/build.gradle b/bin/templates/project/app/build.gradle
@@ -64,6 +64,14 @@ ext {
     if (!project.hasProperty('cdvMinSdkVersion')) {
         cdvMinSdkVersion = null
     }
+    // Sets the maxSdkVersion to the given value.
+    if (!project.hasProperty('cdvMaxSdkVersion')) {
+        cdvMaxSdkVersion = null
+    }
+    // The value for android.targetSdkVersion.
+    if (!project.hasProperty('cdvTargetSdkVersion')) {
+        cdvTargetSdkVersion = null;
+    }
     // Whether to build architecture-specific APKs.
     if (!project.hasProperty('cdvBuildMultipleApks')) {
         cdvBuildMultipleApks = null
@@ -103,10 +111,12 @@ if (hasBuildExtras2) {
 }
 
 // Set property defaults after extension .gradle files.
-if (ext.cdvCompileSdkVersion == null) {
-    ext.cdvCompileSdkVersion = privateHelpers.getProjectTarget()
-    //ext.cdvCompileSdkVersion = project.ext.defaultCompileSdkVersion
-}
+ext.cdvCompileSdkVersion = cdvCompileSdkVersion == null ? (
+    defaultCompileSdkVersion == null 
+        ? privateHelpers.getProjectTarget()
+        : defaultCompileSdkVersion 
+) : Integer.parseInt('' + cdvCompileSdkVersion);
+
 if (ext.cdvBuildToolsVersion == null) {
     ext.cdvBuildToolsVersion = privateHelpers.findLatestInstalledBuildTools()
     //ext.cdvBuildToolsVersion = project.ext.defaultBuildToolsVersion
@@ -121,7 +131,14 @@ if (ext.cdvReleaseSigningPropertiesFile == null && file('../release-signing.prop
 // Cast to appropriate types.
 ext.cdvBuildMultipleApks = cdvBuildMultipleApks == null ? false : cdvBuildMultipleApks.toBoolean();
 ext.cdvVersionCodeForceAbiDigit = cdvVersionCodeForceAbiDigit == null ? false : cdvVersionCodeForceAbiDigit.toBoolean();
+
+// minSdkVersion, maxSdkVersion and targetSdkVersion
 ext.cdvMinSdkVersion = cdvMinSdkVersion == null ? defaultMinSdkVersion : Integer.parseInt('' + cdvMinSdkVersion)
+if (cdvMaxSdkVersion != null) {
+    ext.cdvMaxSdkVersion = Integer.parseInt('' + cdvMaxSdkVersion)
+}
+ext.cdvTargetSdkVersion = cdvTargetSdkVersion == null ? defaultTargetSdkVersion  : Integer.parseInt('' + cdvTargetSdkVersion)
+
 ext.cdvVersionCode = cdvVersionCode == null ? null : Integer.parseInt('' + cdvVersionCode)
 
 def computeBuildTargetName(debugBuild) {
@@ -151,6 +168,8 @@ task cdvPrintProps {
         println('cdvVersionCode=' + cdvVersionCode)
         println('cdvVersionCodeForceAbiDigit=' + cdvVersionCodeForceAbiDigit)
         println('cdvMinSdkVersion=' + cdvMinSdkVersion)
+        println('cdvMaxSdkVersion=' + cdvMaxSdkVersion)
+        println('cdvTargetSdkVersion=' + cdvTargetSdkVersion)
         println('cdvBuildMultipleApks=' + cdvBuildMultipleApks)
         println('cdvReleaseSigningPropertiesFile=' + cdvReleaseSigningPropertiesFile)
         println('cdvDebugSigningPropertiesFile=' + cdvDebugSigningPropertiesFile)
@@ -170,6 +189,14 @@ android {
         if (cdvMinSdkVersion != null) {
             minSdkVersion cdvMinSdkVersion
         }
+
+        if (cdvMaxSdkVersion != null) {
+            maxSdkVersion cdvMaxSdkVersion
+        }
+
+        if(cdvTargetSdkVersion != null) {
+            targetSdkVersion cdvTargetSdkVersion
+        }
     }
 
     lintOptions {

diff --git a/bin/templates/project/legacy/build.gradle b/bin/templates/project/legacy/build.gradle
@@ -66,6 +66,14 @@ ext {
     if (!project.hasProperty('cdvMinSdkVersion')) {
         cdvMinSdkVersion = null
     }
+    // Sets the maxSdkVersion to the given value.
+    if (!project.hasProperty('cdvMaxSdkVersion')) {
+        cdvMaxSdkVersion = null
+    }
+    // The value for android.targetSdkVersion.
+    if (!project.hasProperty('cdvTargetSdkVersion')) {
+        cdvTargetSdkVersion = null;
+    }
     // Whether to build architecture-specific APKs.
     if (!project.hasProperty('cdvBuildMultipleApks')) {
         cdvBuildMultipleApks = null
@@ -112,6 +120,11 @@ if (ext.cdvReleaseSigningPropertiesFile == null && file('release-signing.propert
 // Cast to appropriate types.
 ext.cdvBuildMultipleApks = cdvBuildMultipleApks == null ? false : cdvBuildMultipleApks.toBoolean();
 ext.cdvMinSdkVersion = cdvMinSdkVersion == null ? null : Integer.parseInt('' + cdvMinSdkVersion)
+
+if(cdvMaxSdkVersion != null) {
+    ext.cdvMaxSdkVersion = Integer.parseInt('' + cdvMaxSdkVersion)
+}
+
 ext.cdvVersionCode = cdvVersionCode == null ? null : Integer.parseInt('' + cdvVersionCode)
 
 def computeBuildTargetName(debugBuild) {
@@ -139,6 +152,8 @@ task cdvPrintProps << {
     println('cdvBuildToolsVersion=' + cdvBuildToolsVersion)
     println('cdvVersionCode=' + cdvVersionCode)
     println('cdvMinSdkVersion=' + cdvMinSdkVersion)
+    println('cdvMaxSdkVersion=' + cdvMaxSdkVersion)
+    println('cdvTargetSdkVersion=' + cdvTargetSdkVersion)
     println('cdvBuildMultipleApks=' + cdvBuildMultipleApks)
     println('cdvReleaseSigningPropertiesFile=' + cdvReleaseSigningPropertiesFile)
     println('cdvDebugSigningPropertiesFile=' + cdvDebugSigningPropertiesFile)
@@ -170,6 +185,14 @@ android {
         if (cdvMinSdkVersion != null) {
             minSdkVersion cdvMinSdkVersion
         }
+
+        if (cdvMaxSdkVersion != null) {
+            maxSdkVersion cdvMaxSdkVersion
+        }
+
+        if(cdvTargetSdkVersion != null) {
+            targetSdkVersion cdvTargetSdkVersion
+        }
     }
 
     lintOptions {

diff --git a/framework/AndroidManifest.xml b/framework/AndroidManifest.xml
@@ -19,5 +19,4 @@
 -->
 <manifest xmlns:android="http://schemas.android.com/apk/res/android"
       package="org.apache.cordova" android:versionName="1.0" android:versionCode="1">
-    <uses-sdk android:minSdkVersion="19" />
 </manifest>
diff --git a/framework/build.gradle b/framework/build.gradle
@@ -53,6 +53,11 @@ android {
         targetCompatibility JavaVersion.VERSION_1_8
     }
 
+    // For the Android Cordova Lib, we will hardcode the minSdkVersion and not allow changes.
+    defaultConfig {
+        minSdkVersion 19
+    }
+
     sourceSets {
         main {
             manifest.srcFile 'AndroidManifest.xml'

diff --git a/package.json b/package.json
@@ -31,7 +31,6 @@
   "dependencies": {
     "android-versions": "^1.3.0",
     "cordova-common": "^3.1.0",
-    "elementtree": "^0.1.7",
     "nopt": "^4.0.1",
     "properties-parser": "^0.3.1",
     "q": "^1.4.1",

diff --git a/spec/unit/AndroidManifest.spec.js b/spec/unit/AndroidManifest.spec.js
@@ -29,9 +29,6 @@ describe('AndroidManifest', () => {
     const ACTIVITY_LAUNCH_MODE = 'singleTop';
     const ACTIVITY_NAME = 'MainActivity';
     const ACTIVITY_ORIENTATION = 'portrait';
-    const MIN_SDK_VERSION = '12';
-    const MAX_SDK_VERSION = '88';
-    const TARGET_SDK_VERSION = '27';
 
     const DEFAULT_MANIFEST = `<?xml version='1.0' encoding='utf-8'?>
 <manifest android:hardwareAccelerated="true" android:versionCode="${VERSION_CODE}" android:versionName="${VERSION_NAME}"
@@ -51,7 +48,6 @@ describe('AndroidManifest', () => {
             </intent-filter>
         </activity>
     </application>
-    <uses-sdk android:minSdkVersion="${MIN_SDK_VERSION}" android:maxSdkVersion="${MAX_SDK_VERSION}" android:targetSdkVersion="${TARGET_SDK_VERSION}" />
 </manifest>`;
 
     const manifestPath = path.join(os.tmpdir(), `AndroidManifest${Date.now()}.xml`);
@@ -190,78 +186,6 @@ describe('AndroidManifest', () => {
         });
     });
 
-    describe('minSdkVersion', () => {
-        it('should get minSdkVersion', () => {
-            expect(manifest.getMinSdkVersion()).toBe(MIN_SDK_VERSION);
-        });
-
-        it('should set minSdkVersion', () => {
-            const newMinSdkVersion = `${MIN_SDK_VERSION}111`;
-            manifest.setMinSdkVersion(newMinSdkVersion);
-            expect(manifest.getMinSdkVersion()).toBe(newMinSdkVersion);
-        });
-
-        it('should create the uses-sdk node if it does not exist when setting minSdkVersion', () => {
-            const root = manifest.doc.getroot();
-            root.remove(root.find('./uses-sdk'));
-
-            expect(root.find('./uses-sdk')).toBe(null);
-
-            manifest.setMinSdkVersion(1);
-
-            expect(root.find('./uses-sdk')).not.toBe(null);
-            expect(manifest.getMinSdkVersion()).toBe(1);
-        });
-    });
-
-    describe('maxSdkVersion', () => {
-        it('should get maxSdkVersion', () => {
-            expect(manifest.getMaxSdkVersion()).toBe(MAX_SDK_VERSION);
-        });
-
-        it('should set maxSdkVersion', () => {
-            const newMaxSdkVersion = `${MAX_SDK_VERSION}999`;
-            manifest.setMaxSdkVersion(newMaxSdkVersion);
-            expect(manifest.getMaxSdkVersion()).toBe(newMaxSdkVersion);
-        });
-
-        it('should create the uses-sdk node if it does not exist when setting maxSdkVersion', () => {
-            const root = manifest.doc.getroot();
-            root.remove(root.find('./uses-sdk'));
-
-            expect(root.find('./uses-sdk')).toBe(null);
-
-            manifest.setMaxSdkVersion(1);
-
-            expect(root.find('./uses-sdk')).not.toBe(null);
-            expect(manifest.getMaxSdkVersion()).toBe(1);
-        });
-    });
-
-    describe('targetSdkVersion', () => {
-        it('should get targetSdkVersion', () => {
-            expect(manifest.getTargetSdkVersion()).toBe(TARGET_SDK_VERSION);
-        });
-
-        it('should set targetSdkVersion', () => {
-            const newTargetSdkVersion = `${TARGET_SDK_VERSION}555`;
-            manifest.setTargetSdkVersion(newTargetSdkVersion);
-            expect(manifest.getTargetSdkVersion()).toBe(newTargetSdkVersion);
-        });
-
-        it('should create the uses-sdk node if it does not exist when setting targetSdkVersion', () => {
-            const root = manifest.doc.getroot();
-            root.remove(root.find('./uses-sdk'));
-
-            expect(root.find('./uses-sdk')).toBe(null);
-
-            manifest.setTargetSdkVersion(1);
-
-            expect(root.find('./uses-sdk')).not.toBe(null);
-            expect(manifest.getTargetSdkVersion()).toBe(1);
-        });
-    });
-
     describe('debuggable', () => {
         it('should get debuggable', () => {
             expect(manifest.getDebuggable()).toBe(true);