You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by vladimir-kotikov <gi...@git.apache.org> on 2015/05/22 11:16:19 UTC
[GitHub] cordova-android pull request: CB-8954 Adds support for `requiremen...
GitHub user vladimir-kotikov opened a pull request:
https://github.com/apache/cordova-android/pull/176
CB-8954 Adds support for `requirements` command
Implemetation for https://issues.apache.org/jira/browse/CB-8954
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-android CB-8954
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-android/pull/176.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #176
----
commit 7013b1ff01efe3a666b16e358f2028c4219b286e
Author: Vladimir Kotikov <v-...@microsoft.com>
Date: 2015-04-22T11:24:26Z
Updates check_reqs API to aloow consume it in more advanced way.
* Updates all check_* methods to return information about version of tool installed.
* Introduces Requirement class
* Introduces check_all method, which return result of checks instead of resolving/failing in case of unsatisfied requirements
* Makes check_all method check all of the requirements
commit a333384d5e3f168b8866273a45bae000352279a8
Author: Vladimir Kotikov <v-...@microsoft.com>
Date: 2015-04-22T11:30:33Z
Makes error mesages a bit more descriptive.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-8954 Adds support for `requiremen...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on the pull request:
https://github.com/apache/cordova-android/pull/176#issuecomment-105632526
In general looks good. A few minor comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-8954 Adds support for `requiremen...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/176#discussion_r31065606
--- Diff: bin/lib/check_reqs.js ---
@@ -70,18 +72,23 @@ module.exports.get_target = function() {
// Returns a promise. Called only by build and clean commands.
module.exports.check_ant = function() {
- return tryCommand('ant -version', 'Failed to run "ant -version", make sure you have ant installed and added to your PATH.');
+ return tryCommand('ant -version', 'Failed to run "ant -version", make sure you have ant installed and added to your PATH.')
+ .then(function (output) {
+ // Parse Ant version from command output
+ return /version ((?:\d+\.)+(?:\d+))/i.exec(output)[1];
+ });
};
// Returns a promise. Called only by build and clean commands.
module.exports.check_gradle = function() {
var sdkDir = process.env['ANDROID_HOME'];
- var wrapperDir = path.join(sdkDir, 'tools', 'templates', 'gradle', 'wrapper');
- if (!fs.existsSync(wrapperDir)) {
- return Q.reject(new Error('Could not find gradle wrapper within android sdk. Might need to update your Android SDK.\n' +
- 'Looked here: ' + wrapperDir));
- }
- return Q.when();
+ var wrapper = path.join(sdkDir, 'tools', 'templates', 'gradle', 'wrapper', 'gradlew');
--- End diff --
If `ANDROID_HOME` is not defined, the error you will get here is:
Gradle: not installed
[TypeError: Arguments to path.join must be strings]
(since you end up passing null to `path.join()`)
While there is a previous check for `ANDROID_HOME` (as part of the Android SDK check), it would be nice to make this error more meaningful.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-8954 Adds support for `requiremen...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/176#discussion_r31066149
--- Diff: bin/lib/check_reqs.js ---
@@ -196,20 +208,22 @@ module.exports.check_android = function() {
process.env['ANDROID_HOME'] = grandParentDir;
hasAndroidHome = true;
} else {
- throw new Error('ANDROID_HOME is not set and no "tools" directory found at ' + parentDir);
+ throw new Error('Failed to find \'ANDROID_HOME\' environment variable. Try setting setting it manually.\n' +
+ 'Detected \'android\' command at ' + parentDir + ' but no \'tools\' directory found near.\n' +
+ 'Try reinstall Android SDK or update your PATH to include path to valid SDK directory.');
}
}
if (hasAndroidHome && !adbInPath) {
process.env['PATH'] += path.delimiter + path.join(process.env['ANDROID_HOME'], 'platform-tools');
}
if (!process.env['ANDROID_HOME']) {
- throw new Error('ANDROID_HOME is not set and "android" command not in your PATH. You must fulfill at least one of these conditions.');
+ throw new Error('Failed to find \'ANDROID_HOME\' environment variable. Try setting setting it manually.\n' +
+ 'Failed to find \'android\' command in your \'PATH\'. Try update your \'PATH\' to include path to valid SDK directory.');
}
if (!fs.existsSync(process.env['ANDROID_HOME'])) {
- throw new Error('ANDROID_HOME is set to a non-existant path: ' + process.env['ANDROID_HOME']);
+ throw new Error('\'ANDROID_HOME\' environment variable is leads to non-existant path: ' + process.env['ANDROID_HOME'] +
--- End diff --
"is leads to" isn't good grammar :smile: ... Either leave it as it was ('is set to'), or just 'leads to' (I actually think it worked fine how it was).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-8954 Adds support for `requiremen...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/176#discussion_r31061166
--- Diff: bin/lib/check_reqs.js ---
@@ -223,27 +237,87 @@ module.exports.check_android_target = function(valid_target) {
// android-L
// Google Inc.:Google APIs:20
// Google Inc.:Glass Development Kit Preview:20
+ if (!valid_target) valid_target = module.exports.get_target();
var msg = 'Android SDK not found. Make sure that it is installed. If it is not at the default location, set the ANDROID_HOME environment variable.';
return tryCommand('android list targets --compact', msg)
.then(function(output) {
- if (output.split('\n').indexOf(valid_target) == -1) {
- var androidCmd = module.exports.getAbsoluteAndroidCmd();
- throw new Error('Please install Android target: "' + valid_target + '".\n\n' +
- 'Hint: Open the SDK manager by running: ' + androidCmd + '\n' +
- 'You will require:\n' +
- '1. "SDK Platform" for ' + valid_target + '\n' +
- '2. "Android SDK Platform-tools (latest)\n' +
- '3. "Android SDK Build-tools" (latest)');
+ var targets = output.split('\n');
+ if (targets.indexOf(valid_target) >= 0) {
+ return targets;
}
+
+ var androidCmd = module.exports.getAbsoluteAndroidCmd();
+ throw new Error('Please install Android target: "' + valid_target + '".\n\n' +
+ 'Hint: Open the SDK manager by running: ' + androidCmd + '\n' +
+ 'You will require:\n' +
+ '1. "SDK Platform" for ' + valid_target + '\n' +
+ '2. "Android SDK Platform-tools (latest)\n' +
+ '3. "Android SDK Build-tools" (latest)');
});
};
// Returns a promise.
module.exports.run = function() {
- return Q.all([this.check_java(), this.check_android()])
+ return Q.all([this.check_java(), this.check_android(), this.check_android_target()])
.then(function() {
console.log('ANDROID_HOME=' + process.env['ANDROID_HOME']);
console.log('JAVA_HOME=' + process.env['JAVA_HOME']);
});
};
+/**
+ * Object thar represents one of requirements for current platform.
+ * @param {String} id The unique identifier for this requirements.
+ * @param {String} name The name of requirements. Human-readable field.
+ * @param {String} version The version of requirement installed. In some cases could be an array of strings
+ * (for example, check_android_target returns an array of android targets installed)
+ * @param {Boolean} installed Indicates whether the reuirement is installed or not
--- End diff --
My IDE wants you to mark `version` and `installed` as optional :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-8954 Adds support for `requiremen...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/176#discussion_r31061078
--- Diff: bin/lib/check_reqs.js ---
@@ -196,20 +208,22 @@ module.exports.check_android = function() {
process.env['ANDROID_HOME'] = grandParentDir;
hasAndroidHome = true;
} else {
- throw new Error('ANDROID_HOME is not set and no "tools" directory found at ' + parentDir);
+ throw new Error('Failed to find \'ANDROID_HOME\' environment variable. Try setting setting it manually.\n' +
+ 'Detected \'android\' command at ' + parentDir + ' but no \'tools\' directory found near.\n' +
+ 'Try reinstall Android SDK or update your PATH to include path to valid SDK directory.');
}
}
if (hasAndroidHome && !adbInPath) {
process.env['PATH'] += path.delimiter + path.join(process.env['ANDROID_HOME'], 'platform-tools');
}
if (!process.env['ANDROID_HOME']) {
- throw new Error('ANDROID_HOME is not set and "android" command not in your PATH. You must fulfill at least one of these conditions.');
+ throw new Error('Failed to find \'ANDROID_HOME\' environment variable. Try setting setting it manually.\n' +
+ 'Failed to find \'android\' command in your \'PATH\'. Try update your \'PATH\' to include path to valid SDK directory.');
}
if (!fs.existsSync(process.env['ANDROID_HOME'])) {
- throw new Error('ANDROID_HOME is set to a non-existant path: ' + process.env['ANDROID_HOME']);
+ throw new Error('\'ANDROID_HOME\' environment variable is leads to non-existant path: ' + process.env['ANDROID_HOME'] +
--- End diff --
Typo - 'existent' (I know - not new code, but good to fix while you're here)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-8954 Adds support for `requiremen...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-android/pull/176
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-8954 Adds support for `requiremen...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/176#discussion_r31061124
--- Diff: bin/lib/check_reqs.js ---
@@ -223,27 +237,87 @@ module.exports.check_android_target = function(valid_target) {
// android-L
// Google Inc.:Google APIs:20
// Google Inc.:Glass Development Kit Preview:20
+ if (!valid_target) valid_target = module.exports.get_target();
var msg = 'Android SDK not found. Make sure that it is installed. If it is not at the default location, set the ANDROID_HOME environment variable.';
return tryCommand('android list targets --compact', msg)
.then(function(output) {
- if (output.split('\n').indexOf(valid_target) == -1) {
- var androidCmd = module.exports.getAbsoluteAndroidCmd();
- throw new Error('Please install Android target: "' + valid_target + '".\n\n' +
- 'Hint: Open the SDK manager by running: ' + androidCmd + '\n' +
- 'You will require:\n' +
- '1. "SDK Platform" for ' + valid_target + '\n' +
- '2. "Android SDK Platform-tools (latest)\n' +
- '3. "Android SDK Build-tools" (latest)');
+ var targets = output.split('\n');
+ if (targets.indexOf(valid_target) >= 0) {
+ return targets;
}
+
+ var androidCmd = module.exports.getAbsoluteAndroidCmd();
+ throw new Error('Please install Android target: "' + valid_target + '".\n\n' +
+ 'Hint: Open the SDK manager by running: ' + androidCmd + '\n' +
+ 'You will require:\n' +
+ '1. "SDK Platform" for ' + valid_target + '\n' +
+ '2. "Android SDK Platform-tools (latest)\n' +
+ '3. "Android SDK Build-tools" (latest)');
});
};
// Returns a promise.
module.exports.run = function() {
- return Q.all([this.check_java(), this.check_android()])
+ return Q.all([this.check_java(), this.check_android(), this.check_android_target()])
.then(function() {
console.log('ANDROID_HOME=' + process.env['ANDROID_HOME']);
console.log('JAVA_HOME=' + process.env['JAVA_HOME']);
});
};
+/**
+ * Object thar represents one of requirements for current platform.
+ * @param {String} id The unique identifier for this requirements.
+ * @param {String} name The name of requirements. Human-readable field.
+ * @param {String} version The version of requirement installed. In some cases could be an array of strings
+ * (for example, check_android_target returns an array of android targets installed)
+ * @param {Boolean} installed Indicates whether the reuirement is installed or not
--- End diff --
Typo - 'requirement'
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org