Skip to content

Conversation

@StephenHodgson
Copy link
Member

  • fix android sdk license acceptance input

- fix android sdk license acceptance input
Copilot AI review requested due to automatic review settings October 17, 2025 15:57
@StephenHodgson StephenHodgson merged commit 37b66b8 into main Oct 17, 2025
3 of 4 checks passed
@StephenHodgson StephenHodgson deleted the fix/android-sdk-license-accept branch October 17, 2025 15:58
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes Android SDK license acceptance by improving how input is provided to the SDK manager process. The fix addresses platform-specific handling of the JAVA_HOME environment variable on Windows and ensures license acceptance prompts are properly handled by pre-writing acceptance responses to stdin.

Key changes:

  • Fixed JAVA_HOME path quoting for Windows platform compatibility
  • Moved license acceptance buffer creation and stdin writing to occur before output handling begins
  • Used OS-specific line endings (os.EOL) instead of hardcoded newlines for license acceptance

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/android-sdk.ts Fixed JAVA_HOME Windows path handling and improved Android SDK license acceptance by pre-writing responses using OS-specific line endings
package.json Bumped version to 1.3.2

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

env: { ...process.env, JAVA_HOME: javaPath }
env: {
...process.env,
JAVA_HOME: process.platform === 'win32' ? `"${javaPath}"` : javaPath
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding quotes around the JAVA_HOME path on Windows can cause issues. Environment variable values should not include quotes - the operating system handles paths with spaces correctly without them. This could break Java execution if the JDK tools don't expect quoted paths.

Suggested change
JAVA_HOME: process.platform === 'win32' ? `"${javaPath}"` : javaPath
JAVA_HOME: javaPath

Copilot uses AI. Check for mistakes.
function handleDataStream(data: Buffer) {
const chunk = data.toString();
output += chunk;
process.stderr.write(chunk);
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handleDataStream function writes all output to stderr, but it's being used for both stdout and stderr streams (lines 154-155). Output from child.stdout should be written to process.stdout, not process.stderr.

Copilot uses AI. Check for mistakes.
Comment on lines +152 to +155
const acceptBuffer = Buffer.from(Array(10).fill('y').join(os.EOL), 'utf8');
child.stdin.write(acceptBuffer);
child.stdout.on('data', (data: Buffer) => handleDataStream(data));
child.stderr.on('data', (data: Buffer) => handleDataStream(data));
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing all license acceptances to stdin immediately before the SDK manager prompts may cause issues. The previous implementation waited for the 'Accept? (y/N):' prompt before responding. Pre-writing responses could lead to the input being consumed before prompts appear or cause the SDK manager to behave unexpectedly if it doesn't buffer stdin properly.

Suggested change
const acceptBuffer = Buffer.from(Array(10).fill('y').join(os.EOL), 'utf8');
child.stdin.write(acceptBuffer);
child.stdout.on('data', (data: Buffer) => handleDataStream(data));
child.stderr.on('data', (data: Buffer) => handleDataStream(data));
// Respond to license prompts only when detected in output
const licensePromptRegex = /Accept\?\s*\(y\/N\):/i;
let stdoutBuffer = '';
let stderrBuffer = '';
function checkAndRespond(buffer: string) {
if (licensePromptRegex.test(buffer)) {
child.stdin.write('y' + os.EOL);
}
}
child.stdout.on('data', (data: Buffer) => {
const chunk = data.toString();
stdoutBuffer += chunk;
handleDataStream(data);
checkAndRespond(stdoutBuffer);
// Keep buffer from growing indefinitely
if (stdoutBuffer.length > 1000) stdoutBuffer = stdoutBuffer.slice(-1000);
});
child.stderr.on('data', (data: Buffer) => {
const chunk = data.toString();
stderrBuffer += chunk;
handleDataStream(data);
checkAndRespond(stderrBuffer);
if (stderrBuffer.length > 1000) stderrBuffer = stderrBuffer.slice(-1000);
});

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant