fix: Gradle Args parsing (apache#1606)

* fix: Gradle Args parsing

* refactor: Applied ARGVParser.parseArgsStringToArgv -> parseArgsStringToArgv suggestion

* test: Added deeper testing for gradle argument parsing

lib/build.js

@@ -21,6 +21,7 @@ const path = require('path');
 const fs = require('fs');
 const nopt = require('nopt');
 const untildify = require('untildify');
+const { parseArgsStringToArgv } = require('string-argv');
 
 const Adb = require('./Adb');
 
@@ -36,7 +37,11 @@ function parseOpts (options, resolvedTarget, projectRoot) {
         minSdkVersion: String,
         maxSdkVersion: String,
         targetSdkVersion: String,
+
+        // This needs to be an array since nopts will parse its entries as further options for this process
+        // It will be an array of 1 string: [ "string args" ]
         gradleArg: [String, Array],
+
         keystore: path,
         alias: String,
         storePassword: String,
@@ -58,7 +63,8 @@ function parseOpts (options, resolvedTarget, projectRoot) {
     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);
+        const gradleArgs = parseArgsStringToArgv(options.argv.gradleArg[0]);
+        ret.extraArgs = ret.extraArgs.concat(gradleArgs);
     }
 
     const packageArgs = {};

lib/builders/ProjectBuilder.js

@@ -85,31 +85,33 @@ class ProjectBuilder {
     }
 
     getArgs (cmd, opts) {
-        let args;
+        let args = [
+            '-b', path.join(this.root, 'build.gradle')
+        ];
+        if (opts.extraArgs) {
+            args = args.concat(opts.extraArgs);
+        }
+
         let buildCmd = cmd;
         if (opts.packageType === PackageType.BUNDLE) {
             if (cmd === 'release') {
                 buildCmd = ':app:bundleRelease';
             } else if (cmd === 'debug') {
                 buildCmd = ':app:bundleDebug';
             }
-
-            args = [buildCmd, '-b', path.join(this.root, 'build.gradle')];
         } else {
             if (cmd === 'release') {
                 buildCmd = 'cdvBuildRelease';
             } else if (cmd === 'debug') {
                 buildCmd = 'cdvBuildDebug';
             }
 
-            args = [buildCmd, '-b', path.join(this.root, 'build.gradle')];
-
             if (opts.arch) {
                 args.push('-PcdvBuildArch=' + opts.arch);
             }
         }
 
-        args.push.apply(args, opts.extraArgs);
+        args.push(buildCmd);
 
         return args;
     }
@@ -318,6 +320,8 @@ class ProjectBuilder {
         const wrapper = path.join(this.root, 'gradlew');
         const args = this.getArgs(opts.buildType === 'debug' ? 'debug' : 'release', opts);
 
+        events.emit('verbose', `Running Gradle Build: ${wrapper} ${args.join(' ')}`);
+
         try {
             return await execa(wrapper, args, { stdio: 'inherit', cwd: path.resolve(this.root) });
         } catch (error) {

package.json

@@ -34,6 +34,7 @@
     "nopt": "^7.1.0",
     "properties-parser": "^0.3.1",
     "semver": "^7.3.8",
+    "string-argv": "^0.3.1",
     "untildify": "^4.0.0",
     "which": "^3.0.0"
   },

spec/unit/build.spec.js

@@ -0,0 +1,116 @@
+/*
+       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');
+const builders = require('../../lib/builders/builders');
+
+describe('build', () => {
+    let build;
+    const builder = builders.getBuilder('FakeRootPath');
+
+    beforeEach(() => {
+        build = rewire('../../lib/build');
+        build.__set__({
+            events: jasmine.createSpyObj('eventsSpy', ['emit'])
+        });
+
+        // run needs `this` to behave like an Api instance
+        build.run = build.run.bind({
+            _builder: builder
+        });
+
+        spyOn(builder, 'build').and.returnValue(Promise.resolve({
+            paths: ['fake.apk'],
+            buildtype: 'debug'
+        }));
+    });
+
+    describe('argument parsing', () => {
+        let prepEnvSpy;
+
+        beforeEach(() => {
+            prepEnvSpy = spyOn(builder, 'prepEnv').and.returnValue(Promise.resolve());
+        });
+
+        describe('gradleArg', () => {
+            const baseOptions = {
+                packageType: 'apk',
+                arch: undefined,
+                prepEnv: undefined,
+                buildType: 'debug'
+            };
+
+            it('can parse single gradle argument', async () => {
+                await build.run({
+                    argv: [
+                        'node',
+                        '--gradleArg=--stacktrace'
+                    ]
+                });
+
+                expect(prepEnvSpy).toHaveBeenCalledWith({
+                    ...baseOptions,
+                    extraArgs: ['--stacktrace']
+                });
+            });
+
+            it('can parse multiple gradle arguments', async () => {
+                await build.run({
+                    argv: [
+                        'node',
+                        '--gradleArg=--stacktrace --info'
+                    ]
+                });
+
+                expect(prepEnvSpy).toHaveBeenCalledWith({
+                    ...baseOptions,
+                    extraArgs: ['--stacktrace', '--info']
+                });
+            });
+
+            it('can parse multiple gradle arguments with strings', async () => {
+                await build.run({
+                    argv: [
+                        'node',
+                        '--gradleArg=--testArg="hello world"'
+                    ]
+                });
+
+                expect(prepEnvSpy).toHaveBeenCalledWith({
+                    ...baseOptions,
+                    extraArgs: ['--testArg="hello world"']
+                });
+            });
+
+            it('gradle args will split when necessary', async () => {
+                await build.run({
+                    argv: [
+                        'node',
+                        '--gradleArg=--warning-mode all'
+                    ]
+                });
+
+                expect(prepEnvSpy).toHaveBeenCalledWith({
+                    ...baseOptions,
+                    extraArgs: ['--warning-mode', 'all']
+                });
+            });
+        });
+    });
+});

spec/unit/builders/ProjectBuilder.spec.js

@@ -53,40 +53,40 @@ describe('ProjectBuilder', () => {
 
         it('should set release argument', () => {
             const args = builder.getArgs('release', {});
-            expect(args[0]).toBe('cdvBuildRelease');
+            expect(args[args.length - 1]).toBe('cdvBuildRelease');
         });
 
         it('should set debug argument', () => {
             const args = builder.getArgs('debug', {});
-            expect(args[0]).toBe('cdvBuildDebug');
+            expect(args[args.length - 1]).toBe('cdvBuildDebug');
         });
 
         it('should set apk release', () => {
             const args = builder.getArgs('release', {
                 packageType: 'apk'
             });
-            expect(args[0]).withContext(args).toBe('cdvBuildRelease');
+            expect(args[args.length - 1]).withContext(args).toBe('cdvBuildRelease');
         });
 
         it('should set apk debug', () => {
             const args = builder.getArgs('debug', {
                 packageType: 'apk'
             });
-            expect(args[0]).withContext(args).toBe('cdvBuildDebug');
+            expect(args[args.length - 1]).withContext(args).toBe('cdvBuildDebug');
         });
 
         it('should set bundle release', () => {
             const args = builder.getArgs('release', {
                 packageType: 'bundle'
             });
-            expect(args[0]).withContext(args).toBe(':app:bundleRelease');
+            expect(args[args.length - 1]).withContext(args).toBe(':app:bundleRelease');
         });
 
         it('should set bundle debug', () => {
             const args = builder.getArgs('debug', {
                 packageType: 'bundle'
             });
-            expect(args[0]).withContext(args).toBe(':app:bundleDebug');
+            expect(args[args.length - 1]).withContext(args).toBe(':app:bundleDebug');
         });
 
         it('should add architecture if it is passed', () => {
@@ -100,14 +100,14 @@ describe('ProjectBuilder', () => {
             const args = builder.getArgs('clean', {
                 packageType: 'apk'
             });
-            expect(args[0]).toBe('clean');
+            expect(args[args.length - 1]).toBe('clean');
         });
 
         it('should clean bundle', () => {
             const args = builder.getArgs('clean', {
                 packageType: 'bundle'
             });
-            expect(args[0]).toBe('clean');
+            expect(args[args.length - 1]).toBe('clean');
         });
 
         describe('should accept extra arguments', () => {
@@ -176,6 +176,7 @@ describe('ProjectBuilder', () => {
         it('should reject if the spawn fails', () => {
             const errorMessage = 'Test error';
             execaSpy.and.rejectWith(new Error(errorMessage));
+            builder.getArgs.and.returnValue([]);
 
             return builder.build({}).then(
                 () => fail('Unexpectedly resolved'),
@@ -192,6 +193,7 @@ describe('ProjectBuilder', () => {
             ProjectBuilder.__set__('check_reqs', checkReqsSpy);
             checkReqsSpy.check_android_target.and.resolveTo();
             execaSpy.and.rejectWith(testError);
+            builder.getArgs.and.returnValue([]);
 
             return builder.build({}).then(
                 () => fail('Unexpectedly resolved'),