Fix preparation script for macOS for shell check #79
Conversation
Replaced `if [ $? -eq 0 ]; then` with `if softwareupdate --install-rosetta --agree-to-license; then` in the `check_and_install_rosetta` function to check the exit status directly.
WalkthroughThe changes in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I probably applied an IDE suggestion a while ago. Pushing it here so that little rabbit can review and suggest changes 🪄 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/scripts/macos/prepare-env.sh (4)
14-14: LGTM! Command status check improvement.The direct command status check is a better approach than using
$?. This change follows shell scripting best practices and eliminates potential race conditions.Consider capturing error output for better debugging:
- if softwareupdate --install-rosetta --agree-to-license; then + if softwareupdate --install-rosetta --agree-to-license 2> rosetta_error.log; then
91-97: Consider enhancing error messages.While the blank line removal improves readability, the error messages could be more specific about what exactly failed during serial extraction (e.g., base64 decoding, regex matching, etc.).
Example improvement:
- printf '%s\n' "Failed to parse the serial from the Unity license." + printf '%s\n' "Failed to parse the serial from the Unity license: ${error_step:-Unknown error}"
Line range hint
127-131: Consider improving the license extraction robustness.The current implementation could benefit from better error handling and more robust string manipulation.
Suggested improvements:
- encoded_serial="$(cut -c 5- <<< "$developer_data")" - - decoded_unity_serial="$(base64 --decode <<< "$encoded_serial")" - readonly decoded_unity_serial - - if [ -n "$decoded_unity_serial" ]; then return 0; else return 1; fi + # Skip first 4 characters using parameter expansion instead of cut + encoded_serial="${developer_data:4}" + + if ! decoded_unity_serial="$(base64 --decode <<< "$encoded_serial" 2>/dev/null)"; then + printf '%s\n' "Failed to decode base64 serial" >&2 + return 1 + fi + + readonly decoded_unity_serial + + if [ -z "$decoded_unity_serial" ]; then + printf '%s\n' "Decoded serial is empty" >&2 + return 1 + fi + + return 0
Based on the gathered information, I can now provide the final response:
Environment variables and dependencies need documentation updates
The script uses several undocumented environment variables and external dependencies:
Required environment variables:
UNITY_EDITOR_VERSION: Documented in executors but not in the scriptunity_username,unity_password: Used for Unity activation but undocumentedunity_serialorunity_encoded_license: Used for licensing but undocumentedBASH_ENV: Used for persisting environment variablesRequired external commands:
- System tools:
softwareupdate,arch,base64,cut,mkdir,chmod- Third-party tools:
brew,npm,perl- Unity-specific: Unity Hub, Unity Editor
🔗 Analysis chain
Line range hint
1-167: Verify environment variables and dependencies.The script relies on several environment variables and external commands that should be documented and verified.
Run this script to check for undocumented environment variables and required commands:
Let me gather more information about the environment variables and their documentation.
Let me check the documentation files for information about the required environment variables.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for environment variables used in the script echo "Checking for environment variables..." grep -o '\$[A-Z_][A-Z0-9_]*' src/scripts/macos/prepare-env.sh | sort -u # Check for external commands echo -e "\nChecking for required commands..." commands=( "softwareupdate" "brew" "npm" "perl" "base64" "cut" "mkdir" "chmod" ) for cmd in "${commands[@]}"; do echo "Command '$cmd' is referenced in the script" doneLength of output: 1426
Script:
#!/bin/bash # Check for environment variables documentation in README or docs echo "Checking documentation for environment variables..." rg -i "BASH_ENV|UNITY_EDITOR_VERSION" -A 5 -B 5 # Check for additional environment variables used in string comparisons echo -e "\nChecking for additional environment variables..." rg '\[ -n "\$\w+" \]' src/scripts/macos/prepare-env.sh # Check for any setup or documentation files echo -e "\nChecking for setup documentation..." fd -t f "README|CONTRIBUTING|SETUP|\.md$|\.env"Length of output: 4419
Script:
#!/bin/bash # Check documentation for environment variables echo "Checking README.md for environment variables..." cat README.md echo -e "\nChecking CONTRIBUTING.md for environment variables..." cat CONTRIBUTING.md # Check for any environment variable documentation in the orb source echo -e "\nChecking for environment variables in orb documentation..." rg -i "environment|variables" -A 5 -B 5 src/executors/Length of output: 8912
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/scripts/macos/prepare-env.sh(5 hunks)
🔇 Additional comments (1)
src/scripts/macos/prepare-env.sh (1)
118-118: LGTM! Formatting improvement.
Removed trailing space in LC_ALL export.
Replaced
if [ $? -eq 0 ]; thenwithif softwareupdate --install-rosetta --agree-to-license; thenin thecheck_and_install_rosettafunction to check the exit status directly.Summary by CodeRabbit
LC_ALLvariable for consistency.