[
https://issues.apache.org/jira/browse/CB-14201?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16671107#comment-16671107
]
ASF GitHub Bot commented on CB-14201:
-------------------------------------
dpogue closed pull request #465: CB-14201: (android) Removes Gradle property
in-line command arguments…
URL: https://github.com/apache/cordova-android/pull/465
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/bin/templates/cordova/lib/builders/ProjectBuilder.js
b/bin/templates/cordova/lib/builders/ProjectBuilder.js
index b000de3e0..24aef07cd 100644
--- a/bin/templates/cordova/lib/builders/ProjectBuilder.js
+++ b/bin/templates/cordova/lib/builders/ProjectBuilder.js
@@ -46,20 +46,15 @@ class ProjectBuilder {
} else if (cmd === 'debug') {
cmd = 'cdvBuildDebug';
}
- var args = [cmd, '-b', path.join(this.root, 'build.gradle')];
+
+ let args = [cmd, '-b', path.join(this.root, 'build.gradle')];
+
if (opts.arch) {
args.push('-PcdvBuildArch=' + opts.arch);
}
- // 10 seconds -> 6 seconds
- args.push('-Dorg.gradle.daemon=true');
- // to allow dex in process
- args.push('-Dorg.gradle.jvmargs=-Xmx2048m');
- // allow NDK to be used - required by Gradle 1.5 plugin
- // args.push('-Pandroid.useDeprecatedNdk=true');
args.push.apply(args, opts.extraArgs);
- // Shaves another 100ms, but produces a "try at own risk" warning. Not
worth it (yet):
- // args.push('-Dorg.gradle.parallel=true');
+
return args;
}
diff --git a/bin/templates/cordova/lib/config/GradlePropertiesParser.js
b/bin/templates/cordova/lib/config/GradlePropertiesParser.js
new file mode 100644
index 000000000..ac7f38634
--- /dev/null
+++ b/bin/templates/cordova/lib/config/GradlePropertiesParser.js
@@ -0,0 +1,97 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/
+
+let fs = require('fs');
+let path = require('path');
+let propertiesParser = require('properties-parser');
+let events = require('cordova-common').events;
+
+class GradlePropertiesParser {
+ /**
+ * Loads and Edits Gradle Properties File.
+ *
+ * @param {String} platformDir is the path of the Android platform directory
+ */
+ constructor (platformDir) {
+ this._defaults = {
+ // 10 seconds -> 6 seconds
+ 'org.gradle.daemon': 'true',
+
+ // to allow dex in process
+ 'org.gradle.jvmargs': '-Xmx2048m',
+
+ // allow NDK to be used - required by Gradle 1.5 plugin
+ 'android.useDeprecatedNdk': 'true'
+
+ // Shaves another 100ms, but produces a "try at own risk" warning.
Not worth it (yet):
+ // 'org.gradle.parallel': 'true'
+ };
+
+ this.gradleFilePath = path.join(platformDir, 'gradle.properties');
+ }
+
+ configure () {
+ events.emit('verbose', '[Gradle Properties] Preparing Configuration');
+
+ this._initializeEditor();
+ this._configureDefaults();
+ this._save();
+ }
+
+ /**
+ * Initialize the properties editor for parsing, setting, etc.
+ */
+ _initializeEditor () {
+ // Touch empty gradle.properties file if missing.
+ if (!fs.existsSync(this.gradleFilePath)) {
+ events.emit('verbose', '[Gradle Properties] File missing, creating
file with Cordova defaults.');
+ fs.writeFileSync(this.gradleFilePath, '', 'utf-8');
+ }
+
+ // Create an editor for parsing, getting, and setting configurations.
+ this.gradleFile = propertiesParser.createEditor(this.gradleFilePath);
+ }
+
+ /**
+ * Validate that defaults are set and set the missing defaults.
+ */
+ _configureDefaults () {
+ // Loop though Cordova default properties and set only if missing.
+ Object.keys(this._defaults).forEach(key => {
+ let value = this.gradleFile.get(key);
+
+ if (!value) {
+ events.emit('verbose', `[Gradle Properties] Appended missing
default: ${key}=${this._defaults[key]}`);
+ this.gradleFile.set(key, this._defaults[key]);
+ } else if (value !== this._defaults[key]) {
+ events.emit('info', `[Gradle Properties] Detected Gradle
property "${key}" with the value of "${value}", Cordova's recommended value is
"${this._defaults[key]}"`);
+ }
+ });
+ }
+
+ /**
+ * Saves any changes that has been made to the properties file.
+ */
+ _save () {
+ events.emit('verbose', '[Gradle Properties] Updating and Saving File');
+ this.gradleFile.save();
+ }
+}
+
+module.exports = GradlePropertiesParser;
diff --git a/bin/templates/cordova/lib/prepare.js
b/bin/templates/cordova/lib/prepare.js
index 9dc85ff3a..54aa97d1e 100644
--- a/bin/templates/cordova/lib/prepare.js
+++ b/bin/templates/cordova/lib/prepare.js
@@ -33,6 +33,8 @@ var PlatformJson = require('cordova-common').PlatformJson;
var PlatformMunger = require('cordova-common').ConfigChanges.PlatformMunger;
var PluginInfoProvider = require('cordova-common').PluginInfoProvider;
+const GradlePropertiesParser = require('./config/GradlePropertiesParser');
+
module.exports.prepare = function (cordovaProject, options) {
var self = this;
@@ -41,6 +43,9 @@ module.exports.prepare = function (cordovaProject, options) {
this._config = updateConfigFilesFrom(cordovaProject.projectConfig, munger,
this.locations);
+ let gradlePropertiesParser = new
GradlePropertiesParser(this.locations.root);
+ gradlePropertiesParser.configure();
+
// Update own www dir with project's www assets and plugins' assets and
js-files
return Q.when(updateWww(cordovaProject, this.locations)).then(function () {
// update project according to config.xml changes.
diff --git a/spec/unit/config/GradlePropertiesParser.spec.js
b/spec/unit/config/GradlePropertiesParser.spec.js
new file mode 100644
index 000000000..a9f78905f
--- /dev/null
+++ b/spec/unit/config/GradlePropertiesParser.spec.js
@@ -0,0 +1,167 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/
+
+const rewire = require('rewire');
+let GradlePropertiesParser =
rewire('../../../bin/templates/cordova/lib/config/GradlePropertiesParser');
+
+describe('Gradle Builder', () => {
+ describe('_initializeEditor method', () => {
+ let parser;
+ let writeFileSyncSpy;
+ let existsSyncSpy;
+ let createEditorSpy;
+ let emitSpy;
+
+ beforeEach(() => {
+ createEditorSpy = jasmine.createSpy('createEditor');
+ emitSpy = jasmine.createSpy('emit');
+
+ GradlePropertiesParser.__set__('propertiesParser', {
+ createEditor: createEditorSpy
+ });
+
+ GradlePropertiesParser.__set__('events', {
+ emit: emitSpy
+ });
+
+ parser = new GradlePropertiesParser('/root');
+ });
+
+ it('should not detect an existing gradle.properties file and create
new file', () => {
+ existsSyncSpy =
jasmine.createSpy('existsSync').and.returnValue(false);
+ writeFileSyncSpy = jasmine.createSpy('writeFileSync');
+ GradlePropertiesParser.__set__('fs', {
+ existsSync: existsSyncSpy,
+ writeFileSync: writeFileSyncSpy
+ });
+
+ parser._initializeEditor();
+
+ expect(emitSpy).toHaveBeenCalled();
+ expect(emitSpy.calls.argsFor(0)[1]).toContain('File missing,
creating file with Cordova defaults');
+ expect(writeFileSyncSpy).toHaveBeenCalled();
+ expect(createEditorSpy).toHaveBeenCalled();
+ });
+
+ it('should detect an existing gradle.properties file', () => {
+ existsSyncSpy =
jasmine.createSpy('existsSync').and.returnValue(true);
+ writeFileSyncSpy = jasmine.createSpy('writeFileSync');
+ GradlePropertiesParser.__set__('fs', {
+ existsSync: existsSyncSpy,
+ writeFileSync: writeFileSyncSpy
+ });
+
+ parser._initializeEditor();
+
+ expect(writeFileSyncSpy).not.toHaveBeenCalled();
+ expect(createEditorSpy).toHaveBeenCalled();
+ });
+ });
+
+ describe('_configureDefaults method', () => {
+ let parser;
+ let emitSpy;
+
+ beforeEach(() => {
+ emitSpy = jasmine.createSpy('emit');
+
+ GradlePropertiesParser.__set__('events', {
+ emit: emitSpy
+ });
+
+ parser = new GradlePropertiesParser('/root');
+
+ parser._defaults = {'org.gradle.jvmargs': '-Xmx2048m'};
+ });
+
+ it('should detect missing default property and sets the property.', ()
=> {
+ let setSpy = jasmine.createSpy('set');
+ let getSpy = jasmine.createSpy('get').and.returnValue(false);
+
+ parser.gradleFile = {
+ set: setSpy,
+ get: getSpy
+ };
+
+ parser._configureDefaults();
+
+ expect(getSpy).toHaveBeenCalled();
+ expect(setSpy).toHaveBeenCalled();
+ expect(emitSpy.calls.argsFor(0)[1]).toContain('Appended missing
default');
+ });
+
+ it('should not detect missing defaults and not call set.', () => {
+ let setSpy = jasmine.createSpy('set');
+ let getSpy = jasmine.createSpy('get').and.returnValue(true);
+
+ parser.gradleFile = {
+ set: setSpy,
+ get: getSpy
+ };
+
+ parser._configureDefaults();
+
+ expect(getSpy).toHaveBeenCalled();
+ expect(setSpy).not.toHaveBeenCalled();
+ });
+
+ it('should detect default with changed value.', () => {
+ let setSpy = jasmine.createSpy('set');
+ let getSpy = jasmine.createSpy('get').and.returnValue('-Xmx512m');
+
+ parser.gradleFile = {
+ set: setSpy,
+ get: getSpy
+ };
+
+ parser._configureDefaults();
+
+ expect(getSpy).toHaveBeenCalled();
+ expect(setSpy).not.toHaveBeenCalled();
+ expect(emitSpy.calls.argsFor(0)[1]).toContain('Cordova\'s
recommended value is');
+ });
+ });
+
+ describe('_save method', () => {
+ let parser;
+ let emitSpy;
+ let saveSpy;
+
+ beforeEach(() => {
+ emitSpy = jasmine.createSpy('emit');
+ GradlePropertiesParser.__set__('events', {
+ emit: emitSpy
+ });
+
+ parser = new GradlePropertiesParser('/root');
+
+ saveSpy = jasmine.createSpy('save');
+ parser.gradleFile = {
+ save: saveSpy
+ };
+ });
+
+ it('should detect save being called.', () => {
+ parser._save();
+
+ expect(saveSpy).toHaveBeenCalled();
+ expect(emitSpy.calls.argsFor(0)[1]).toContain('Updating and Saving
File');
+ });
+ });
+});
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Remove Gradle property in-line command arguments for gradle.properties
> ----------------------------------------------------------------------
>
> Key: CB-14201
> URL: https://issues.apache.org/jira/browse/CB-14201
> Project: Apache Cordova
> Issue Type: Improvement
> Components: cordova-android
> Reporter: Bryan Ellis
> Priority: Minor
>
> By passing in properties as an argument to the Gradle's command, it prevents
> users from being able to override these values.
> In one example, a user reported
> ([CB-14139|https://issues.apache.org/jira/browse/CB-14139]) the need to be
> able to set the jvmargs so builds works on 32bit Java.
> Some other values Cordova passes by default are org.gradle.daemon and
> android.useDeprecatedNdk.
> By removing the command line argument usage and migrating to the usage of
> gradle.properties, it gives users the flexibility to override defaults if
> desired.
> This ticket will also resolve
> [CB-14139|https://issues.apache.org/jira/browse/CB-14139] but also remove the
> other argument usage.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For additional commands, e-mail: issues-h...@cordova.apache.org
