Skip to content

Commit

Permalink
fix: cordova requirements consider the android-targetSdkVersion (#849)
Browse files Browse the repository at this point in the history 
* Made cordova requirements consider the android-targetSdkVersion preference
* refator: get_target method
Added comments.
Added JSDoc block
Reduced error exit point to one spot

Co-authored-by: エリス <erisu@users.noreply.github.com>
  • Loading branch information
@breautek @erisu
breautek and erisu committed Jan 24, 2020
1 parent 0924654 commit 92268b2
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 20 deletions.
55 changes: 37 additions & 18 deletions bin/templates/cordova/lib/check_reqs.js
View file
Expand Up @@ -26,8 +26,9 @@ var fs = require('fs');
var os = require('os');
var REPO_ROOT = path.join(__dirname, '..', '..', '..', '..');
var PROJECT_ROOT = path.join(__dirname, '..', '..');
var CordovaError = require('cordova-common').CordovaError;
const { CordovaError, ConfigParser, events } = require('cordova-common');
var android_sdk = require('./android_sdk');
const { createEditor } = require('properties-parser');

function forgivingWhichSync (cmd) {
try {
Expand All @@ -45,26 +46,44 @@ module.exports.isDarwin = function () {
return (os.platform() === 'darwin');
};

// Get valid target from framework/project.properties if run from this repo
// Otherwise get target from project.properties file within a generated cordova-android project
/**
* @description Get valid target from framework/project.properties if run from this repo
* Otherwise get target from project.properties file within a generated cordova-android project
* @returns {string} The android target in format "android-${target}"
*/
module.exports.get_target = function () {
function extractFromFile (filePath) {
var target = shelljs.grep(/\btarget=/, filePath);
if (!target) {
throw new Error('Could not find android target within: ' + filePath);
}
return target.split('=')[1].trim();
}
var repo_file = path.join(REPO_ROOT, 'framework', 'project.properties');
if (fs.existsSync(repo_file)) {
return extractFromFile(repo_file);
const projectPropertiesPaths = [
path.join(REPO_ROOT, 'framework', 'project.properties'),
path.join(PROJECT_ROOT, 'project.properties')
];

// Get the minimum required target API from the framework.
let target = projectPropertiesPaths.filter(filePath => fs.existsSync(filePath))
.map(filePath => createEditor(filePath).get('target'))
.pop();

if (!target) {
throw new Error(`We could not locate the target from the "project.properties" at either "${projectPropertiesPaths.join('", "')}".`);
}
var project_file = path.join(PROJECT_ROOT, 'project.properties');
if (fs.existsSync(project_file)) {
// if no target found, we're probably in a project and project.properties is in PROJECT_ROOT.
return extractFromFile(project_file);

// If the repo config.xml file exists, find the desired targetSdkVersion.
const configFile = path.join(REPO_ROOT, 'config.xml');
if (!fs.existsSync(configFile)) return target;

const configParser = new ConfigParser(configFile);
const desiredAPI = parseInt(configParser.getPreference('android-targetSdkVersion', 'android'), 10);

if (!isNaN(desiredAPI)) {
const minimumAPI = parseInt(target.split('-').pop(), 10);

if (desiredAPI >= minimumAPI) {
target = `android-${desiredAPI}`;
} else {
events.emit('warn', `android-targetSdkVersion should be greater than or equal to ${minimumAPI}.`);
}
}
throw new Error('Could not find android target in either ' + repo_file + ' nor ' + project_file);

return target;
};

// Returns a promise. Called only by build and clean commands.
Expand Down
83 changes: 81 additions & 2 deletions spec/unit/check_reqs.spec.js
View file
Expand Up @@ -17,11 +17,16 @@
under the License.
*/

var check_reqs = require('../../bin/templates/cordova/lib/check_reqs');
var rewire = require('rewire');
var check_reqs = rewire('../../bin/templates/cordova/lib/check_reqs');
var android_sdk = require('../../bin/templates/cordova/lib/android_sdk');
var shelljs = require('shelljs');
var fs = require('fs');
var path = require('path');
var events = require('cordova-common').events;

// This should match /bin/templates/project/build.gradle
const DEFAULT_TARGET_API = 29;

describe('check_reqs', function () {
var original_env;
Expand Down Expand Up @@ -176,13 +181,87 @@ describe('check_reqs', function () {
});
});
});

describe('get_target', function () {
var ConfigParser;
var getPreferenceSpy;
beforeEach(function () {
getPreferenceSpy = jasmine.createSpy();
ConfigParser = jasmine.createSpy().and.returnValue({
getPreference: getPreferenceSpy
});
check_reqs.__set__('ConfigParser', ConfigParser);
});

it('should retrieve target from framework project.properties file', function () {
var target = check_reqs.get_target();
expect(target).toBeDefined();
expect(target).toContain('android-');
expect(target).toContain('android-' + DEFAULT_TARGET_API);
});

it('should throw error if target cannot be found', function () {
spyOn(fs, 'existsSync').and.returnValue(false);
expect(function () {
check_reqs.get_target();
}).toThrow();
});

it('should override target from config.xml preference', () => {
var realExistsSync = fs.existsSync;
spyOn(fs, 'existsSync').and.callFake(function (path) {
if (path.indexOf('config.xml') > -1) {
return true;
} else {
return realExistsSync.call(fs, path);
}
});

getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API + 1);

var target = check_reqs.get_target();

expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
expect(target).toBe('android-' + (DEFAULT_TARGET_API + 1));
});

it('should fallback to default target if config.xml has invalid preference', () => {
var realExistsSync = fs.existsSync;
spyOn(fs, 'existsSync').and.callFake(function (path) {
if (path.indexOf('config.xml') > -1) {
return true;
} else {
return realExistsSync.call(fs, path);
}
});

getPreferenceSpy.and.returnValue(NaN);

var target = check_reqs.get_target();

expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
expect(target).toBe('android-' + DEFAULT_TARGET_API);
});

it('should warn if target sdk preference is lower than the minimum required target SDK', () => {
var realExistsSync = fs.existsSync;
spyOn(fs, 'existsSync').and.callFake(function (path) {
if (path.indexOf('config.xml') > -1) {
return true;
} else {
return realExistsSync.call(fs, path);
}
});

getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API - 1);

var target = check_reqs.get_target();

expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
expect(target).toBe('android-' + DEFAULT_TARGET_API);
expect(events.emit).toHaveBeenCalledWith('warn', 'android-targetSdkVersion should be greater than or equal to ' + DEFAULT_TARGET_API + '.');
});
});

describe('check_android_target', function () {
it('should should return full list of supported targets if there is a match to ideal api level', () => {
var fake_targets = ['you are my fire', 'my one desire'];
Expand Down

0 comments on commit 92268b2

Please sign in to comment.