Skip to content

Conversation

@FrederickEngelhardt
Copy link
Contributor


name: Pull Request
about: depot tools for chromium
title: ''
labels: ''
assignees: ''


Description

Type of Change

  • πŸ› Bug fix (patch)
  • ✨ New feature (minor)
  • πŸ’₯ Breaking change (major)
  • πŸ“š Documentation update
  • πŸ”’ Security fix
  • πŸ”§ Maintenance/Refactoring

Release Management

For automatic release on merge, add one of these labels:

  • release:major - Breaking changes, incompatible API changes (bumps 1.x.x β†’ 2.0.0)
  • release:minor - New features, backwards-compatible (bumps x.1.x β†’ x.2.0)

No label = No automatic release (manual release via GitHub Actions later)

Testing

  • Tested on macOS
  • Tested on Linux
  • Tested on WSL
  • Ran ./osa-cli.zsh --scan-secrets (if touching constructors)
  • Ran tests: ./tests/run-tests.zsh

Checklist

  • My code follows the zsh style of this project
  • I have added #!/usr/bin/env zsh shebang to new scripts
  • I have updated documentation (if applicable)
  • No hardcoded secrets (use osa-secret-set instead)
  • All files use .zsh extension (unless intentionally .sh for bash)
  • I have added appropriate release label (release:major or release:minor)

Additional Context

Copilot AI review requested due to automatic review settings December 28, 2025 04:59
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 adds support for installing and initializing depot_tools, a set of utilities required for Chromium development. The implementation registers depot_tools as a new optional component that can be enabled/disabled through configuration files. However, the PR also includes two unrelated utility installation scripts (tree and cmake) and removes a user-specific path from source control.

  • Adds depot_tools component with installation script and shell initialization
  • Updates all configuration YAML files to include the new depot_tools option
  • Modifies CLI to register and handle depot_tools component installation

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
src/zsh/snippets Removes accidentally committed file containing hardcoded user path
src/zsh/plugin-init/depot-tools.zsh Adds shell initialization for depot_tools PATH setup (but not integrated into base.zsh)
src/setup/install-depot-tools.zsh Implements depot_tools installation/update script
src/setup/install-tree.zsh Unrelated: adds tree utility installer (missing shebang, not registered)
src/setup/install-cmake.zsh Unrelated: adds cmake installer (missing shebang, not registered)
osa-cli.zsh Registers depot-tools component and adds to component_keys arrays
configs/*.yaml Adds depot_tools flag to all configuration presets
Comments suppressed due to low confidence (1)

src/zsh/snippets:1

  • This file should not be tracked in git. According to .gitignore line 10, the entire src/zsh/snippets/ directory is intentionally ignored because it contains downloaded snippets installed during setup. This file appears to contain a hardcoded user-specific path (/Users/fre/dev/osa-snippets) which should never be committed. The deletion is correct, but verify this wasn't accidentally committed in a previous change.

@@ -0,0 +1,3 @@
# Install cmake for building C/C++ projects
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

Missing shebang at the beginning of the file. All zsh script files must start with #!/usr/bin/env zsh according to the project's coding guidelines.

Copilot generated this review using guidance from repository custom instructions.
@@ -0,0 +1,3 @@
# Install cmake for building C/C++ projects
echo "Installing cmake "
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

Trailing space after "cmake ". Remove the trailing space for clean code formatting.

Suggested change
echo "Installing cmake "
echo "Installing cmake"

Copilot uses AI. Check for mistakes.
osa-cli.zsh Outdated
# Development tools with install scripts
register_component "git" "Configure Git (version control)" "all" "src/setup/git.zsh"
register_component "cocoapods" "Install CocoaPods for iOS development" "macos" "src/setup/install-cocoapods.zsh"
register_component "depot-tools" "Install depot_tools (Chromium development utilities)" "all" "src/setup/install-depot-tools.zsh"
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The README.md should be updated to document the new depot-tools component. According to coding guideline #4, when modifications/updates/changes are added to the osa-cli tool scope that change/add/remove usage/description/args/options, the README.md must be updated. The new depot-tools component should be added to the "Optional Components" section around line 193, similar to how CocoaPods is documented.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +1 to +3
# Install tree which is a useful utility for displaying directory structures
echo "Installing tree utility..."
brew install tree No newline at end of file
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

This file appears to be unrelated to the PR's stated purpose of adding depot-tools for Chromium development. It's not registered as a component in osa-cli.zsh and is not referenced anywhere in the codebase. If this is intended to be a new component, it should be registered and included in the configuration files. Otherwise, it should be removed from this PR or submitted separately with proper integration.

Copilot uses AI. Check for mistakes.
Comment on lines 6 to 13
DEPOT_TOOLS_HOME="${DEPOT_TOOLS_HOME:-$HOME/.depot_tools}"

if [[ -d "$DEPOT_TOOLS_HOME" ]]; then
export PATH="$DEPOT_TOOLS_HOME:$PATH"
else
# Inform user depot_tools is not installed
if [[ -z "$DEPOT_TOOLS_SKIP_INIT_MESSAGE" ]]; then
echo "Note: depot_tools not found at: $DEPOT_TOOLS_HOME"
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

Inconsistent variable naming between the install script and plugin initialization. The install script uses DEPOT_TOOLS_DIR while the plugin init uses DEPOT_TOOLS_HOME for the same directory path. This inconsistency could confuse users trying to customize the installation location. Use the same variable name (DEPOT_TOOLS_DIR) in both files for consistency.

Suggested change
DEPOT_TOOLS_HOME="${DEPOT_TOOLS_HOME:-$HOME/.depot_tools}"
if [[ -d "$DEPOT_TOOLS_HOME" ]]; then
export PATH="$DEPOT_TOOLS_HOME:$PATH"
else
# Inform user depot_tools is not installed
if [[ -z "$DEPOT_TOOLS_SKIP_INIT_MESSAGE" ]]; then
echo "Note: depot_tools not found at: $DEPOT_TOOLS_HOME"
DEPOT_TOOLS_DIR="${DEPOT_TOOLS_DIR:-${DEPOT_TOOLS_HOME:-$HOME/.depot_tools}}"
if [[ -d "$DEPOT_TOOLS_DIR" ]]; then
export PATH="$DEPOT_TOOLS_DIR:$PATH"
else
# Inform user depot_tools is not installed
if [[ -z "$DEPOT_TOOLS_SKIP_INIT_MESSAGE" ]]; then
echo "Note: depot_tools not found at: $DEPOT_TOOLS_DIR"

Copilot uses AI. Check for mistakes.
Comment on lines 1 to 18
#!/usr/bin/env zsh
# depot_tools plugin initialization
# Add depot_tools to PATH if installed
# https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/depot_tools_tutorial.html

DEPOT_TOOLS_HOME="${DEPOT_TOOLS_HOME:-$HOME/.depot_tools}"

if [[ -d "$DEPOT_TOOLS_HOME" ]]; then
export PATH="$DEPOT_TOOLS_HOME:$PATH"
else
# Inform user depot_tools is not installed
if [[ -z "$DEPOT_TOOLS_SKIP_INIT_MESSAGE" ]]; then
echo "Note: depot_tools not found at: $DEPOT_TOOLS_HOME"
echo "Install with: ./osa-cli.zsh --enable depot-tools"
echo "Or manually: git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git ~/.depot_tools"
export DEPOT_TOOLS_SKIP_INIT_MESSAGE=1
fi
fi
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The depot-tools plugin initialization file is created but never loaded in src/zsh/constructors/base.zsh. Similar to how android-sdk.zsh is conditionally loaded (line 46-48 in base.zsh), you need to add a conditional load for depot-tools.zsh. Without this, the depot-tools PATH setup will never take effect during shell initialization.

Copilot uses AI. Check for mistakes.
mise: true
git: true
cocoapods: true # iOS and React Native
depot_tools: true
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The new depot_tools component lacks test coverage. Looking at tests/test_cli_components.bats, other components like cocoapods and android have tests verifying their presence and correct configuration across config files. Consider adding tests to verify that depot_tools is correctly defined in all config files and has the expected value in everything.yaml (true) and other configs (false).

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +3
# Install cmake for building C/C++ projects
echo "Installing cmake "
brew install cmake No newline at end of file
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

This file appears to be unrelated to the PR's stated purpose of adding depot-tools for Chromium development. It's not registered as a component in osa-cli.zsh and is not referenced anywhere in the codebase. If this is intended to be a new component, it should be registered and included in the configuration files. Otherwise, it should be removed from this PR or submitted separately with proper integration.

Copilot uses AI. Check for mistakes.
Comment on lines 12 to 16
if [[ -z "$DEPOT_TOOLS_SKIP_INIT_MESSAGE" ]]; then
echo "Note: depot_tools not found at: $DEPOT_TOOLS_HOME"
echo "Install with: ./osa-cli.zsh --enable depot-tools"
echo "Or manually: git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git ~/.depot_tools"
export DEPOT_TOOLS_SKIP_INIT_MESSAGE=1
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The informational messages in the else block will be printed every time a new shell session starts if depot_tools is not installed. This could become annoying for users who don't need depot_tools. Consider using the DEPOT_TOOLS_SKIP_INIT_MESSAGE environment variable check before printing the messages, not just for preventing repeated messages within the same session. Alternatively, follow the pattern in android-sdk.zsh which only prints messages once per session by checking the environment variable first.

Suggested change
if [[ -z "$DEPOT_TOOLS_SKIP_INIT_MESSAGE" ]]; then
echo "Note: depot_tools not found at: $DEPOT_TOOLS_HOME"
echo "Install with: ./osa-cli.zsh --enable depot-tools"
echo "Or manually: git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git ~/.depot_tools"
export DEPOT_TOOLS_SKIP_INIT_MESSAGE=1
if [[ -z "$DEPOT_TOOLS_SKIP_INIT_MESSAGE" && -z "$DEPOT_TOOLS_INIT_MESSAGE_SHOWN" ]]; then
echo "Note: depot_tools not found at: $DEPOT_TOOLS_HOME"
echo "Install with: ./osa-cli.zsh --enable depot-tools"
echo "Or manually: git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git ~/.depot_tools"
export DEPOT_TOOLS_INIT_MESSAGE_SHOWN=1

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,3 @@
# Install tree which is a useful utility for displaying directory structures
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

Missing shebang at the beginning of the file. All zsh script files must start with #!/usr/bin/env zsh according to the project's coding guidelines.

Copilot generated this review using guidance from repository custom instructions.
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.

2 participants