mirror of
https://github.com/game-ci/unity-builder.git
synced 2026-09-29 12:07:05 -07:00
fix: glue --flag=value instead of --flag value, avoiding argv ambiguity
Real bug caught by live CI (not the release-timing gap, which was separately expected and has since resolved): "--flag value" as two argv tokens is ambiguous when value itself starts with "-" - e.g. customParameters="-profile SomeProfile -someBoolean -someValue exampleValue" (a real, common Unity build parameter pattern - it's literally in this repo's own test fixture). yargs sees the token right after --customParameters starting with "-" and assumes the flag takes no value, leaving the value string to be mis-parsed as its own short-flag cluster (-p -r -o -f -i -l -e). Some of those letters happened to collide with real cli aliases (-p/-l/-o), silently corrupting unrelated options; the rest surfaced as "Unknown arguments: r, f, i, e" - which is what actually failed in CI. Verified against the real cli binary locally (bun run src/index.ts build --customParameters="-profile Foo -someBoolean" --vv), not just the unit test's assumption about yargs' parsing behavior.
This commit is contained in:
+8
-2
@@ -80,7 +80,7 @@ function buildCliArgs({ getInput }) {
|
||||
if (!targetPlatform) {
|
||||
throw new Error('targetPlatform is required.');
|
||||
}
|
||||
args.push('--targetPlatform', targetPlatform);
|
||||
args.push(`--targetPlatform=${targetPlatform}`);
|
||||
const providerStrategy = getInput('providerStrategy') || 'local';
|
||||
if (providerStrategy !== 'local') {
|
||||
throw new Error(`Provider strategy "${providerStrategy}" is not supported by this thin wrapper. ` +
|
||||
@@ -89,8 +89,14 @@ function buildCliArgs({ getInput }) {
|
||||
}
|
||||
for (const [input, flag] of STRING_FLAGS) {
|
||||
const value = getInput(input);
|
||||
// "--flag value" as two argv tokens is ambiguous when value itself
|
||||
// starts with "-" (e.g. customParameters="-profile Foo -someBoolean"):
|
||||
// yargs sees the next token starting with "-" and assumes the flag
|
||||
// takes no value, leaving the value string to be mis-parsed as its own
|
||||
// (partly alias-colliding) short-flag cluster. "--flag=value" glues
|
||||
// them into one token, which is unambiguous.
|
||||
if (value)
|
||||
args.push(`--${flag}`, value);
|
||||
args.push(`--${flag}=${value}`);
|
||||
}
|
||||
for (const [input, flag] of BOOLEAN_FLAGS) {
|
||||
const value = getInput(input);
|
||||
|
||||
+1
-1
File diff suppressed because one or more lines are too long
+31
-23
@@ -13,15 +13,14 @@ describe('buildCliArgs', () => {
|
||||
it('builds the minimal command for a bare targetPlatform', () => {
|
||||
expect(buildCliArgs(inputsOf({ targetPlatform: 'StandaloneLinux64' }))).toStrictEqual([
|
||||
'build',
|
||||
'--targetPlatform',
|
||||
'StandaloneLinux64',
|
||||
'--targetPlatform=StandaloneLinux64',
|
||||
]);
|
||||
});
|
||||
|
||||
it('puts projectPath as the positional argument right after "build"', () => {
|
||||
expect(
|
||||
buildCliArgs(inputsOf({ targetPlatform: 'StandaloneLinux64', projectPath: 'game' })),
|
||||
).toStrictEqual(['build', 'game', '--targetPlatform', 'StandaloneLinux64']);
|
||||
).toStrictEqual(['build', 'game', '--targetPlatform=StandaloneLinux64']);
|
||||
});
|
||||
|
||||
it('throws for a non-local providerStrategy, matching the base action without @game-ci/orchestrator', () => {
|
||||
@@ -30,7 +29,7 @@ describe('buildCliArgs', () => {
|
||||
).toThrow(/aws/);
|
||||
});
|
||||
|
||||
it('passes string inputs through as their mapped flag', () => {
|
||||
it('passes string inputs through as their mapped flag, glued with = to avoid value/flag ambiguity', () => {
|
||||
const args = buildCliArgs(
|
||||
inputsOf({
|
||||
targetPlatform: 'StandaloneLinux64',
|
||||
@@ -40,12 +39,25 @@ describe('buildCliArgs', () => {
|
||||
}),
|
||||
);
|
||||
|
||||
expect(args).toContain('--buildName');
|
||||
expect(args).toContain('MyGame');
|
||||
expect(args).toContain('--buildMethod');
|
||||
expect(args).toContain('Foo.Bar');
|
||||
expect(args).toContain('--dockerCpuLimit');
|
||||
expect(args).toContain('4');
|
||||
expect(args).toContain('--buildName=MyGame');
|
||||
expect(args).toContain('--buildMethod=Foo.Bar');
|
||||
expect(args).toContain('--dockerCpuLimit=4');
|
||||
});
|
||||
|
||||
it('keeps a value starting with "-" as one unambiguous token', () => {
|
||||
// A value like this ("-profile Foo -someBoolean") would otherwise be
|
||||
// misread as a separate flag by yargs if passed as "--flag value"
|
||||
// (two argv tokens) - see the regression this covers.
|
||||
const args = buildCliArgs(
|
||||
inputsOf({
|
||||
targetPlatform: 'StandaloneLinux64',
|
||||
customParameters: '-profile SomeProfile -someBoolean -someValue exampleValue',
|
||||
}),
|
||||
);
|
||||
|
||||
expect(args).toContain(
|
||||
'--customParameters=-profile SomeProfile -someBoolean -someValue exampleValue',
|
||||
);
|
||||
});
|
||||
|
||||
it('remaps android inputs to their current (non-deprecated) cli flag names', () => {
|
||||
@@ -58,24 +70,20 @@ describe('buildCliArgs', () => {
|
||||
}),
|
||||
);
|
||||
|
||||
expect(args).toContain('--androidKeystorePassword');
|
||||
expect(args).toContain('keystore-secret');
|
||||
expect(args).toContain('--androidKeyAlias');
|
||||
expect(args).toContain('my-alias');
|
||||
expect(args).toContain('--androidKeyAliasPassword');
|
||||
expect(args).toContain('alias-secret');
|
||||
expect(args).toContain('--androidKeystorePassword=keystore-secret');
|
||||
expect(args).toContain('--androidKeyAlias=my-alias');
|
||||
expect(args).toContain('--androidKeyAliasPassword=alias-secret');
|
||||
// The deprecated cli flag names should never be emitted.
|
||||
expect(args).not.toContain('--androidKeystorePass');
|
||||
expect(args).not.toContain('--androidKeyAliasName');
|
||||
expect(args).not.toContain('--androidKeyAliasPass');
|
||||
expect(args.some((arg) => arg.startsWith('--androidKeystorePass='))).toBe(false);
|
||||
expect(args.some((arg) => arg.startsWith('--androidKeyAliasName='))).toBe(false);
|
||||
expect(args.some((arg) => arg.startsWith('--androidKeyAliasPass='))).toBe(false);
|
||||
});
|
||||
|
||||
it('renames versioning to versioningStrategy', () => {
|
||||
const args = buildCliArgs(inputsOf({ targetPlatform: 'StandaloneLinux64', versioning: 'Tag' }));
|
||||
|
||||
expect(args).toContain('--versioningStrategy');
|
||||
expect(args).toContain('Tag');
|
||||
expect(args).not.toContain('--versioning');
|
||||
expect(args).toContain('--versioningStrategy=Tag');
|
||||
expect(args.some((arg) => arg.startsWith('--versioning='))).toBe(false);
|
||||
});
|
||||
|
||||
it('emits boolean flags only when truthy, without a value', () => {
|
||||
@@ -96,6 +104,6 @@ describe('buildCliArgs', () => {
|
||||
it('omits flags for empty/unset inputs, leaving cli defaults in effect', () => {
|
||||
const args = buildCliArgs(inputsOf({ targetPlatform: 'StandaloneLinux64' }));
|
||||
|
||||
expect(args).toStrictEqual(['build', '--targetPlatform', 'StandaloneLinux64']);
|
||||
expect(args).toStrictEqual(['build', '--targetPlatform=StandaloneLinux64']);
|
||||
});
|
||||
});
|
||||
|
||||
+8
-2
@@ -79,7 +79,7 @@ export function buildCliArgs({ getInput }: BuildArgsOptions): string[] {
|
||||
if (!targetPlatform) {
|
||||
throw new Error('targetPlatform is required.');
|
||||
}
|
||||
args.push('--targetPlatform', targetPlatform);
|
||||
args.push(`--targetPlatform=${targetPlatform}`);
|
||||
|
||||
const providerStrategy = getInput('providerStrategy') || 'local';
|
||||
if (providerStrategy !== 'local') {
|
||||
@@ -92,7 +92,13 @@ export function buildCliArgs({ getInput }: BuildArgsOptions): string[] {
|
||||
|
||||
for (const [input, flag] of STRING_FLAGS) {
|
||||
const value = getInput(input);
|
||||
if (value) args.push(`--${flag}`, value);
|
||||
// "--flag value" as two argv tokens is ambiguous when value itself
|
||||
// starts with "-" (e.g. customParameters="-profile Foo -someBoolean"):
|
||||
// yargs sees the next token starting with "-" and assumes the flag
|
||||
// takes no value, leaving the value string to be mis-parsed as its own
|
||||
// (partly alias-colliding) short-flag cluster. "--flag=value" glues
|
||||
// them into one token, which is unambiguous.
|
||||
if (value) args.push(`--${flag}=${value}`);
|
||||
}
|
||||
|
||||
for (const [input, flag] of BOOLEAN_FLAGS) {
|
||||
|
||||
Reference in New Issue
Block a user