This is an automated email from the ASF dual-hosted git repository. normanbreau pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/cordova-android.git
The following commit(s) were added to refs/heads/master by this push: new 3343c3bb fix: Gradle Args parsing (#1606) 3343c3bb is described below commit 3343c3bb3490563d96740df84c60fa06d3662970 Author: Norman Breau <nor...@nbsolutions.ca> AuthorDate: Sat Apr 22 17:00:51 2023 -0300 fix: Gradle Args parsing (#1606) * fix: Gradle Args parsing * refactor: Applied ARGVParser.parseArgsStringToArgv -> parseArgsStringToArgv suggestion * test: Added deeper testing for gradle argument parsing --- lib/build.js | 8 ++- lib/builders/ProjectBuilder.js | 16 +++-- package-lock.json | 14 ++++ package.json | 1 + spec/unit/build.spec.js | 116 ++++++++++++++++++++++++++++++ spec/unit/builders/ProjectBuilder.spec.js | 18 ++--- 6 files changed, 158 insertions(+), 15 deletions(-) diff --git a/lib/build.js b/lib/build.js index ad71093b..7e42f280 100644 --- a/lib/build.js +++ b/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 = {}; diff --git a/lib/builders/ProjectBuilder.js b/lib/builders/ProjectBuilder.js index 4410cd5b..815d08f3 100644 --- a/lib/builders/ProjectBuilder.js +++ b/lib/builders/ProjectBuilder.js @@ -85,7 +85,13 @@ 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') { @@ -93,8 +99,6 @@ class ProjectBuilder { } else if (cmd === 'debug') { buildCmd = ':app:bundleDebug'; } - - args = [buildCmd, '-b', path.join(this.root, 'build.gradle')]; } else { if (cmd === 'release') { buildCmd = 'cdvBuildRelease'; @@ -102,14 +106,12 @@ class ProjectBuilder { 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) { diff --git a/package-lock.json b/package-lock.json index 380b0067..a8f9873d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -18,6 +18,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" }, @@ -4358,6 +4359,14 @@ "integrity": "sha512-D9cPgkvLlV3t3IzL0D0YLvGA9Ahk4PcvVwUbN0dSGr1aP0Nrt4AEnTUbuGvquEC0mA64Gqt1fzirlRs5ibXx8g==", "dev": true }, + "node_modules/string-argv": { + "version": "0.3.1", + "resolved": "https://registry.npmjs.org/string-argv/-/string-argv-0.3.1.tgz", + "integrity": "sha512-a1uQGz7IyVy9YwhqjZIZu1c8JO8dNIe20xBmSS6qu9kv++k3JGzCVmprbNN5Kn+BgzD5E7YYwg1CcjuJMRNsvg==", + "engines": { + "node": ">=0.6.19" + } + }, "node_modules/string-width": { "version": "4.2.3", "resolved": "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz", @@ -8118,6 +8127,11 @@ "integrity": "sha512-D9cPgkvLlV3t3IzL0D0YLvGA9Ahk4PcvVwUbN0dSGr1aP0Nrt4AEnTUbuGvquEC0mA64Gqt1fzirlRs5ibXx8g==", "dev": true }, + "string-argv": { + "version": "0.3.1", + "resolved": "https://registry.npmjs.org/string-argv/-/string-argv-0.3.1.tgz", + "integrity": "sha512-a1uQGz7IyVy9YwhqjZIZu1c8JO8dNIe20xBmSS6qu9kv++k3JGzCVmprbNN5Kn+BgzD5E7YYwg1CcjuJMRNsvg==" + }, "string-width": { "version": "4.2.3", "resolved": "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz", diff --git a/package.json b/package.json index 699450de..c73e1d88 100644 --- a/package.json +++ b/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" }, diff --git a/spec/unit/build.spec.js b/spec/unit/build.spec.js new file mode 100644 index 00000000..9e249eca --- /dev/null +++ b/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'] + }); + }); + }); + }); +}); diff --git a/spec/unit/builders/ProjectBuilder.spec.js b/spec/unit/builders/ProjectBuilder.spec.js index 7d954910..f2cb633a 100644 --- a/spec/unit/builders/ProjectBuilder.spec.js +++ b/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'), --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cordova.apache.org For additional commands, e-mail: commits-h...@cordova.apache.org