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

Improve Gradle Build Arguments #699

Merged
merged 8 commits into from
Apr 6, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion bin/lib/create.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
30 changes: 0 additions & 30 deletions bin/templates/cordova/lib/AndroidManifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/

var fs = require('fs');
var et = require('elementtree');
var xml = require('cordova-common').xmlHelpers;

var DEFAULT_ORIENTATION = 'default';
Expand Down Expand Up @@ -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';
};
Expand Down Expand Up @@ -150,7 +124,3 @@ AndroidManifest.prototype.write = function (destPath) {
};

module.exports = AndroidManifest;

function capitalize (str) {
return str.charAt(0).toUpperCase() + str.slice(1);
}
6 changes: 6 additions & 0 deletions bin/templates/cordova/lib/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);
}
Expand Down Expand Up @@ -266,6 +270,8 @@ module.exports.help = function () {
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(' \'--maxSdkVersion=#\': Override maxSdkVersion for this build. Useful for uploading multiple APKs.');
Copy link
Member

Choose a reason for hiding this comment

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

Remove "Useful for uploading multiple APKs." from minSdkVersion, maxSdkVersion, andtargetSdkVersion.

There's also probably no value in supporting maxSdkVersion since they really really strongly discourage using it (your app will not be compatible with newer versions of Android)

Copy link
Member Author

@erisu erisu Mar 29, 2019

Choose a reason for hiding this comment

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

At best, we might only be able to add to the printout (Not Recommended).

We already supported the setting of the maxSdkVersion property through config.xml and in which sets it in AndroidManifest.xml.

This PR moves the maxSdkVersion definition into the Gradle's workflow and removes from AndroidManifest.xml.

This does add a new command line option to exposes alternative ways for defining this value other than config.xml. I thought it was a missed option because we supported min and believe there was no harm to add.

If we want to remove maxSdkVersion altogether, it then becomes a major change. As for future planning, it would be best if I don't add the new command-line option then. We probably should just add warnings that the maxSdkVersion setting will be deprecated in the next major.

Thoughts and Opinions?

console.log(' \'--targetSdkVersion=#\': Override targetSdkVersion for this build. Useful for uploading multiple APKs.');
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) :');
Expand Down
7 changes: 4 additions & 3 deletions bin/templates/cordova/lib/prepare.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions bin/templates/project/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,4 @@
</intent-filter>
</activity>
</application>

<uses-sdk android:minSdkVersion="19" android:targetSdkVersion="__APILEVEL__"/>
</manifest>
35 changes: 31 additions & 4 deletions bin/templates/project/app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 ? (
Copy link
Member

Choose a reason for hiding this comment

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

compileSdkVersion should probably default to targetSdkVersion, unless it is explicitly set

defaultCompileSdkVersion == null
? privateHelpers.getProjectTarget()
: defaultCompileSdkVersion
) : Integer.parseInt('' + cdvCompileSdkVersion);

if (ext.cdvBuildToolsVersion == null) {
ext.cdvBuildToolsVersion = privateHelpers.findLatestInstalledBuildTools()
//ext.cdvBuildToolsVersion = project.ext.defaultBuildToolsVersion
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand All @@ -170,6 +189,14 @@ android {
if (cdvMinSdkVersion != null) {
minSdkVersion cdvMinSdkVersion
}

if (cdvMaxSdkVersion != null) {
maxSdkVersion cdvMaxSdkVersion
}

if(cdvTargetSdkVersion != null) {
targetSdkVersion cdvTargetSdkVersion
}
}

lintOptions {
Expand Down
23 changes: 23 additions & 0 deletions bin/templates/project/legacy/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -170,6 +185,14 @@ android {
if (cdvMinSdkVersion != null) {
minSdkVersion cdvMinSdkVersion
}

if (cdvMaxSdkVersion != null) {
maxSdkVersion cdvMaxSdkVersion
}

if(cdvTargetSdkVersion != null) {
targetSdkVersion cdvTargetSdkVersion
}
}

lintOptions {
Expand Down
1 change: 0 additions & 1 deletion framework/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
5 changes: 5 additions & 0 deletions framework/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
76 changes: 0 additions & 76 deletions spec/unit/AndroidManifest.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand All @@ -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`);
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 1 addition & 2 deletions spec/unit/create.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,8 @@ describe('create', function () {
var default_templates = path.join(__dirname, '..', '..', 'bin', 'templates', 'project');
var fake_android_target = 'android-1337';
beforeEach(function () {
Manifest_mock.prototype = jasmine.createSpyObj('AndroidManifest instance mock', ['setPackageId', 'setTargetSdkVersion', 'getActivity', 'setName', 'write']);
Manifest_mock.prototype = jasmine.createSpyObj('AndroidManifest instance mock', ['setPackageId', 'getActivity', 'setName', 'write']);
Manifest_mock.prototype.setPackageId.and.returnValue(new Manifest_mock());
Manifest_mock.prototype.setTargetSdkVersion.and.returnValue(new Manifest_mock());
Manifest_mock.prototype.getActivity.and.returnValue(new Manifest_mock());
Manifest_mock.prototype.setName.and.returnValue(new Manifest_mock());
spyOn(create, 'validatePackageName').and.returnValue(Q());
Expand Down
3 changes: 2 additions & 1 deletion spec/unit/run.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,9 @@ describe('run', () => {
describe('help', () => {
it('should print out usage and help', () => {
const logSpy = jasmine.createSpy();
const errorSpy = jasmine.createSpy();
const procStub = { exit: _ => null, cwd: _ => '', argv: ['', ''] };
run.__set__({ console: { log: logSpy }, process: procStub });
run.__set__({ console: { log: logSpy, error: errorSpy }, process: procStub });

run.help();
expect(logSpy).toHaveBeenCalledWith(jasmine.stringMatching(/^Usage:/));
Expand Down