Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add caching to CLT installation check #950

Closed
wants to merge 1 commit into from

Conversation

willmcginnis
Copy link

Improve the should_install_command_line_tools function by:

  1. Caching the result in SHOULD_INSTALL_CLT to avoid redundant file existence checks.
  2. Using a local variable (ret) to capture return values for added clarity.

This change slightly increases code size but provides a more efficient
and readable approach.

Upsides:

  • Efficiency improvement by skipping redundant checks on subsequent calls.

Downsides:

  • Introduces a new global variable (SHOULD_INSTALL_CLT).
  • Increases the function length by about 10 lines.

Improve the should_install_command_line_tools function by:
1. Caching the result to avoid redundant file existence checks on subsequent calls.
2. Using local variable to capture return values for added clarity
else
! [[ -e "/Library/Developer/CommandLineTools/usr/bin/git" ]] ||
! [[ -e "/Library/Developer/CommandLineTools/usr/bin/git" ]] || \
Copy link
Contributor

Choose a reason for hiding this comment

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

The backslash is not needed.

@gromgit
Copy link
Contributor

gromgit commented Mar 4, 2025

I'm not sure calling should_install_command_line_tools is computationally expensive enough to warrant caching, but even if it is, the required change can be reduced to just 3 lines (1 new, 2 changed):

diff --git a/install.sh b/install.sh
index 71ca08b..4b588fa 100755
--- a/install.sh
+++ b/install.sh
@@ -732,6 +732,7 @@ fi
 if should_install_command_line_tools
 then
   ohai "The Xcode Command Line Tools will be installed."
+  SHOULD_INSTALL_CLT=1
 fi
 
 non_default_repos=""
@@ -834,7 +835,7 @@ then
   execute "${TOUCH[@]}" "${HOMEBREW_CACHE}/.cleaned"
 fi
 
-if should_install_command_line_tools && version_ge "${macos_version}" "10.13"
+if [[ -n "${SHOULD_INSTALL_CLT-}" ]] && version_ge "${macos_version}" "10.13"
 then
   ohai "Searching online for the Command Line Tools"
   # This temporary file prompts the 'softwareupdate' utility to list the Command Line Tools
@@ -859,7 +860,7 @@ then
 fi
 
 # Headless install may have failed, so fallback to original 'xcode-select' method
-if should_install_command_line_tools && test -t 0
+if [[ -n "${SHOULD_INSTALL_CLT-}" ]] && test -t 0
 then
   ohai "Installing the Command Line Tools (expect a GUI popup):"
   execute "/usr/bin/xcode-select" "--install"

@MikeMcQuaid
Copy link
Member

Passing on this, sorry. As @gromgit mentioned: this is not expensive enough to warrant caching. Also, there's a non-zero chance at install time that these files are present at some points and not at others.

@MikeMcQuaid MikeMcQuaid closed this Mar 4, 2025
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.

3 participants