Skip to content

uraniborg/s/p/automate_observation.py Updated copyright year and bug …#3

Open
billy-lau wants to merge 1 commit intoandroid:mainfrom
billy-lau:bug-fix/account-for-missing-prebuilt-dir
Open

uraniborg/s/p/automate_observation.py Updated copyright year and bug …#3
billy-lau wants to merge 1 commit intoandroid:mainfrom
billy-lau:bug-fix/account-for-missing-prebuilt-dir

Conversation

@billy-lau
Copy link
Copy Markdown
Member

…fix.

Fix a bug that was introduced when prebuilt binaries were removed, and the directory that the script relied on to create symlinks is now gone.

Also made sure that the Android SDK's location is known and exists before proceeding to call gradle to build Hubble APK.

Change-Id: Ia37be790e26ff352635215fd211447f8ebc0bdd3

…fix.

Fix a bug that was introduced when prebuilt binaries were removed, and
the directory that the script relied on to create symlinks is now gone.

Also made sure that the Android SDK's location is known and exists
before proceeding to call gradle to build Hubble APK.

Change-Id: Ia37be790e26ff352635215fd211447f8ebc0bdd3
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces an ensure_android_sdk function to automate Android SDK detection and configuration, updates the copyright year, and ensures symlink directories are created. Feedback focuses on increasing the robustness of the SDK detection logic by validating paths, improving the parsing and writing of local.properties to handle whitespace and missing newlines, and advising against interactive input to ensure compatibility with CI/CD environments.

Comment on lines +749 to +754
if os.environ.get("ANDROID_HOME"):
logger.debug("ANDROID_HOME is set to %s", os.environ.get("ANDROID_HOME"))
return True
if os.environ.get("ANDROID_SDK_ROOT"):
logger.debug("ANDROID_SDK_ROOT is set to %s", os.environ.get("ANDROID_SDK_ROOT"))
return True
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The environment variable check should verify that the directory actually exists, consistent with how local.properties and default locations are handled. This ensures the build doesn't fail later due to an invalid path in the environment.

Suggested change
if os.environ.get("ANDROID_HOME"):
logger.debug("ANDROID_HOME is set to %s", os.environ.get("ANDROID_HOME"))
return True
if os.environ.get("ANDROID_SDK_ROOT"):
logger.debug("ANDROID_SDK_ROOT is set to %s", os.environ.get("ANDROID_SDK_ROOT"))
return True
for env_var in ["ANDROID_HOME", "ANDROID_SDK_ROOT"]:
val = os.environ.get(env_var)
if val:
logger.debug("%s is set to %s", env_var, val)
if os.path.exists(val):
return True
logger.warning("%s points to non-existent directory: %s", env_var, val)

Comment on lines +760 to +766
if line.startswith("sdk.dir="):
sdk_dir = line.split("=")[1].strip()
logger.debug("Found sdk.dir in local.properties: %s", sdk_dir)
if os.path.exists(sdk_dir):
return True
else:
logger.warning("sdk.dir in local.properties points to non-existent directory: %s", sdk_dir)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The parsing of local.properties is brittle. It doesn't handle potential whitespace around the = sign and might fail if the value itself contains an = character.

Suggested change
if line.startswith("sdk.dir="):
sdk_dir = line.split("=")[1].strip()
logger.debug("Found sdk.dir in local.properties: %s", sdk_dir)
if os.path.exists(sdk_dir):
return True
else:
logger.warning("sdk.dir in local.properties points to non-existent directory: %s", sdk_dir)
parts = line.split("=", 1)
if len(parts) == 2 and parts[0].strip() == "sdk.dir":
sdk_dir = parts[1].strip()
logger.debug("Found sdk.dir in local.properties: %s", sdk_dir)
if os.path.exists(sdk_dir):
return True
else:
logger.warning("sdk.dir in local.properties points to non-existent directory: %s", sdk_dir)

if not sdk_path:
logger.warning("Android SDK location not found in environment or default locations.")
try:
user_input = input("Please enter the path to your Android SDK: ").strip()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using input() in a script intended for automation can cause it to hang or fail in non-interactive environments (like CI/CD). Consider allowing the SDK path to be passed as a command-line argument or strictly relying on environment variables for such cases.

Comment on lines +801 to +802
lines = [l for l in lines if not l.startswith("sdk.dir=")]
lines.append(f"sdk.dir={sdk_path}\n")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

When appending to local.properties, if the existing file does not end with a newline character, the new entry will be appended to the last line, making it invalid. Additionally, the filtering logic should be more robust against leading whitespace.

  lines = [l for l in lines if not l.strip().startswith("sdk.dir=")]
  if lines and not lines[-1].endswith("\n"):
    lines[-1] += "\n"
  lines.append(f"sdk.dir={sdk_path}\n")

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